Gitlab CI: Broaden Tests coverage and require jobs to pass

Created on 4 April 2025, 3 days ago

Problem/Motivation

The module currently exploits a limited set of Testing variables provided by the GitLab CI project templates.

Additionally, it does not enforce validation of certain "basic" jobs to ensure certain types are not introduced in the code base, such as typos, or coding standards errors.

Steps to reproduce

Run build pipeline on the 2.x branch at:
https://git.drupalcode.org/project/structure_sync/-/pipelines/new

Proposed resolution

Enable all OPT_IN_% variables.
Require jobs to pass.

📌 Task
Status

Active

Version

2.0

Component

Code

Created by

🇫🇷France dydave

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @dydave
  • @dydave opened merge request.
  • 🇫🇷France dydave

    Quick follow-up on this issue:

    Created initial merge request MR!47 above at #2 which should enable complete Tests coverage and code validation on Gitlab CI.

    These changes should allow the module to exploit and benefit from all of the current features and configuration options provided by the Drupal templates for Gitlab CI, available at this moment.

    The MR currently fails on PHPCS validation job, see parent issue 📌 Fix PHPCS validation job errors Active .
    So, once the changes from the other MR are merged, this MR should pass all jobs and Tests correctly.

    Since all Tests are passing 🟢, moving issue to Needs review:
    https://git.drupalcode.org/issue/structure_sync-3517495/-/pipelines/465983

    The configuration in this MR is really only a suggestion, it is left at the maintainers appreciation to comment/uncomment/remove any sections added in the changes.

    Feel free to let us know if you have any questions or concerns on any of the changes in the merge request or this issue in general, we would surely be glad to help.
    Thanks in advance!

  • 🇨🇦Canada mparker17 UTC-4

    @dydave, thank you!

    1. I'm open to enabling OPT_IN_TEST_MAX_PHP if you think that it is necessary.
      1. In the past, I've run into more issues with new code not being compatible with old versions of PHP than issues with new versions of PHP not being compatible with existing code. I'm also mindful of the fact that every test costs the Drupal Association (a little bit of) money.
    2. I'm open to enabling OPT_IN_TEST_PREVIOUS_MINOR, and OPT_IN_TEST_NEXT_MINOR if you think they are necessary.
      1. Generally, I've found that testing the major versions is good enough, because trying to maintain backwards compatibility typically prevents me from being able to use new APIs for a while anyway. I'm also mindful of the fact that every test costs the Drupal Association (a little bit of) money.

    That being said, I am respectfully pushing back on a few of the changes in the merge request...

    1. OPT_IN_TEST_NEXT_MAJOR will always fail, because that currently refers to Drupal 12, and structure_sync doesn't support Drupal 12 yet.
      1. We could change the core_version_requirement to start supporting D12, in order to make those tests pass - but until Drupal 12 reaches alpha, D12 tests will start/stop failing randomly as D12 changes. I've found this discourages new contributors who don't understand that...
        1. the failing D12 test is not their fault, and,
        2. they don't have to fix D12 failures until D12 reaches alpha.

        ... I've found contributors will sometimes abandon an issue if they don't understand how to fix a test fail.

      2. My best practice is to only start caring about compatibility with the next major version of Drupal core when it reaches alpha, and to work on compatibility in an issue branch, so it doesn't affect the mainline until all the bugs have been worked out.
    2. Based on experience with other projects, _PHPUNIT_CONCURRENT (or rather, the thing that variable controls, paratest Run phpunit tests from a single job in parallel Fixed ) often results in random test fails, especially when running Functional and FunctionalJavascript tests, due to the nature of those tests.
      1. As mentioned in the previous point, new contributors sometimes misunderstand random test fails as something that they did incorrectly, which discourages them from contributing (or makes them abandon the issue).
    3. I generally don't require all jobs to pass, because - as a maintainer - I haven't found that to be necessary:
      1. Most contributors will already try to make all the tests pass, i.e.: without me asking.
      2. Any contributors who don't will usually fix it if I ask.
      3. If I really feel strongly that an issue needs to get merged right away, I will fix the code on my own.
      4. Requiring all jobs to pass in .gitlab-ci.yml will also enforce that on the mainline branches, and on security team branches (i.e.: and prevent me from merging to those branches if those lints fail). If I was to be contacted by the security team , I have a duty to the Drupal community to get a fix out as soon as possible. If necessary, I want the option to be able to fix security issues right away, and worry about phpcs, eslint, phpstan, cspell, etc. in follow-up issue(s).

    In summary, if you can change the merge request so that it only sets OPT_IN_TEST_MAX_PHP, OPT_IN_TEST_PREVIOUS_MINOR, and OPT_IN_TEST_NEXT_MINOR, I'll merge it for sure.

    I'm currently hesitant about merging OPT_IN_TEST_NEXT_MAJOR, _PHPUNIT_CONCURRENT, and requiring the linters to pass; but I'm open to new ideas: if you have compelling reasons why I should use those options, I'd be interested to hear them!

Production build 0.71.5 2024