(Try to) fix the GitLab CI

Fix IS

What has been done.

• I’ve converted all deprecated code, otherwise the tests are failing. But this is normal and the CI should always fail on deprecated code. This guarantees that any new MR will not add deprecated code.
• In order to support older Drupal core versions I’ve added a DeprecationdHelper utility class, which is a forward compatibility to the one implemented in Drupal 10.1 (see https://www.drupal.org/node/3379306 → ). This could be replaced with the one provided by core once 10.1 is the minimum supported version.
• I had to convert the LocalTasksTest Unit test to Kernel but I think is better as too much mocking is not always the right way. I didn’t had time to dig into the failures, converting was straight.
• By using concurrency, I’ve reduced the PHPUnit test duration from ~20mins (see https://git.drupalcode.org/issue/search_api-3396064/-/jobs/212824) to ~4 mins (see https://git.drupalcode.org/issue/search_api-3396064/-/jobs/215735)>
• Tests
  • Default (on commit) runs PHP 8.1, Drupal 10.1.x-dev, MySQL 5.7. See https://git.drupalcode.org/issue/search_api-3396064/-/pipelines/37331
  • Scheduled runs PHP 8.1, Drupal 9.5, SqlLite. See https://git.drupalcode.org/issue/search_api-3396064/-/pipelines/37332
  • Legacy Drupal CI. See https://www.drupal.org/pift-ci-job/2793116 →

Caveats

• As now the test dependencies are downloaded, a developer working on the module should manually get the two modules
• With Drupal 9.5, there are 2 deprecation errors that are not because of Search API (spotted also in #3292775: Drupal 10 compatibility → ). There’s nothing we can do about it that, so in the Drupal 9.5 scheduled test, we need disable check of deprecations by setting value of _PHPUNIT_EXTRA env var to --suppress-deprecations. Also in legacy Drupal CI, deprecations were silenced.

What's next

After committing, scheduled tests needs to be setup (needs proper permissions)

Interesting ran the tests locally just to see what is depricated. And I only get one Javascript Depreciation error for AjaxTest::testAjax. This was tested on 9.5.11.

That said it would be amazing if this gets merged and fully set up soon.

Thanks for your work on this! I agree with @admirlju, would be amazing to finally get this fixed. (Though working with MRs on d.o still seems like a lot more work than with patches, unfortunately.)

A few notes, though:

• As deprecations didn’t fail the tests before, I don’t think we should change that. Especially, I do not want to remove testing for deprecated code (i.e., hooks) that a lot of people are still using. Seems we should just always set _PHPUNIT_EXTRA: --suppress-deprecations?
• Agreed, converting LocalTasksTest to a Kernel test does make sense.
• PHPUnit added several unrelated list(…) = ---> [] = changes.
• DeprecationHelper looks great, wasn’t aware of that but looks like a great tool for the future. However, as it seems it was only introduced in 10.1.3, I don’t think we can use it before we depend on 10.2, actually, not 10.1 as you wrote in the @todo comment. Also, I don’t quite understand why you can’t use an import for the Drupal Core class in DeprecationHelper::backwardsCompatibleCall()?
• I think I’m against removing all the comments from .gitlab-ci.yml. Is there a specific reason you did that? I think it’s always great when config files are properly documented. Additionally, it would be great to get an explanatory comment for _PHPUNIT_CONCURRENT. I surmise this tells PhpUnit to run concurrently, which a) improves speed by a lot and b) luckily doesn’t pose any problems for our specific test suite? The comment should just explain that.

Getting test run times down would, in any case, be great.

Attempted a different fix based on drumm’s comment in #3190024-11: Problem with test dependencies when testing issue forks → . Let’s see how it goes.

OK, this did help GitLab CI, but failed to do anything for DrupalCI.
So, maybe it could work in combination with the MR by @claudiu.cristea? I’ll give it a try.

Hm, no, couldn’t figure out how to do it. I didn’t find a way to set the COMPOSER_ROOT_VERSION environment variable for DrupalCI, but if we don’t use the fix for both CI systems then we can’t keep the dev dependencies in the composer.json file.

Does anyone know whether/how we can set custom environment variable values in DrupalCI?

Sorry, I didn't find some time to look at your remarks. I'm trying to take a look next week

OK, this did help GitLab CI, but failed to do anything for DrupalCI.

Is there a reason to keep both? DrupalCI is essentially dead now and we really should be trying to just run everything in gitlab CI.

I'm happy to help get this over the line as I've opened a couple of MRs over the past week but noticed gitlab CI is broken.

Is there a reason to keep both? DrupalCI is essentially dead now and we really should be trying to just run everything in gitlab CI.

No, you’re probably right – if we get GitLab CI working reliably (and with a minimum of seemingly unrelated changes) that’s probably good enough.

Alright, looking great!
While Drupal CI still refuses to busge, GitLab CI now passes flawlessly, so if we are prepared to just disable DrupalCI completely (except for patches, for the next few months, I guess) this might already be ready to go.

All the code style problems reported by stylelint and phpstan can and should be addressed in a follow-up, in my opinion.

Please test/review and we can finally get this resolved!

Also, once again many thanks to @claudiu.cristea for his amazing work on this! While I’ve discarded most of your changes in my MR, your branch was still a valuable resource for resolving all the little problems I encountered.

Would be great if someone could review.

Looking great, nice work

OK, I now activated this in the Search API Saved Searches module → and it’s working pretty well. So, while it would have really been great to get tests of issue forks working in DrupalCI as well, I think I’ll just go ahead and merge this now so people can finally use issue forks in this module.

I’d like to still keep support for testing patches a while longer, as there are a lot of existing ones in the queue, but let’s see whether I manage to do that (without confusing people with failing DrupalCI tests of their MRs).

In any case, thanks a lot again to everyone in here, especially of course Claudiu!
(Unfortunately, when merging an MR it now doesn’t seem to be possible to specify a different author, so I guess going forward the attribution will again only be in the commit message – and of course stored here in the issue itself.)

drunken monkey → closed merge request !96

Automatically closed - issue fixed for 2 weeks with no activity.