Redefine phpunit rules with reusable references

This first commit adds reusable references for the six phpunit jobs. The original plain phpunit job just has *phpunit-base-rules and the five newer variants can reuse this reference plus their own unique one.

Another subtble but important change is that instead of having

   - if: '$SKIP_PHPUNIT == "1"'
     when: never
   - if: '$OPT_IN_TEST_MAX_PHP == "1"'
<code>
we effectively have
<code>
   - if: '$SKIP_PHPUNIT == "1"'
     when: never
   - if: '$OPT_IN_TEST_MAX_PHP != "1"'
     when: never
<code>

This is important because with the existing rule, as soon as <code>if: '$OPT_IN_TEST_MAX_PHP == "1"'

is detected as being true the job fires, and no further rules are checked. This makes extending the rule impossible. So it is better to say 'if opt_in is not true then dont run the job' which in the vanilla, non-customised template has exactly the same effect, but it alllows additional rules to be added after it, and these will not be ignored.

Here's an example of this MR in use
https://git.drupalcode.org/project/scheduler/-/pipelines/72799

I made a couple of small comments, mostly about moving the code within the file.
The MR also needs rebasing.

Back to "Needs work".

Ready for review
Tested on https://git.drupalcode.org/project/scheduler/-/pipelines/74114 which uses all six phpunit variants. (ignore the fail on Next Major). But it proves that the rules are being used ok with no typos.

I think it's a good refactoring and it makes it all more readable and extendable.
RTBC (will probably merge later today).

Do you think it could do with a comment in the snippets, reminding us (and any future maintainers) that the rules should always be extended with conditions that end in when: never, ie test for the conditions when to run? They all have that now, and it's important for extendability that they, and any other new reusable rules, keep this same pattern.

Yeah that’s a good point. A comment right before all the reusable rules won’t hurt.

Its a bit wordy. Let me know if you can think of a clearer way to say it.

Changed it slightly, but it's mostly the same.

Merged and marked this issue as fixed. Thanks so much for this improvement.

This broke contrib testing: https://git.drupalcode.org/project/config_inspector/-/pipelines/74407 is no longer running the phpunit job!

@fjgarlin Can you trigger a test of keycdn to confirm this? 🙏

https://git.drupalcode.org/project/keycdn/-/pipelines/74413
Confirmed. Reverting.

I did check the downstream pipelines were green, but not that all jobs were there. My bad, apologies. And thanks for finding and reporting!!

Working on a fix.

@jonathan1055 - based on the above (thanks again Wim!), I reverted the commit.

I created a new MR with the changes https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/105, which we know is still not 100%.

I only added the "on_success" which we discussed a while back.

Without it: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/74419 (no phpunit job)
With it: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/74424 (phpunit job)

So the issue seems to be around that rule, as all other phpunit jobs extend from "phpunit".

Are you happy to continue working on this?

I created this issue for us to test in module where the regression was found: 📌 [ignore] Rules tests in GitLab CI Active

We can see the pipeline for the issue (already connected to the fork): https://git.drupalcode.org/issue/config_inspector-3413545/-/pipelines/74482
Vs the normal pipeline: https://git.drupalcode.org/project/config_inspector/-/pipelines/74433

jonathan1055 → changed the visibility of the branch 3412915-references-for-rules to hidden.

After adding 'when: on success' to the max php variant, we do now get the two phpunit jobs
https://git.drupalcode.org/issue/config_inspector-3413545/-/pipelines/74492

Now we need to work out if that can be added to either of the reusable snippets or if it has to be in the full definition. The docs also mentioned that you can specify 'when' at the job level, to provide a default for the rules. I will try that next.

After adding 'when: on success' to the max php variant, we do now get the two phpunit jobs
https://git.drupalcode.org/issue/config_inspector-3413545/-/pipelines/74492

Now we need to work out if that can be added to either of the reusable snippets or if it has to be in the full definition. The docs also mentioned that you can specify 'when' at the job level, to provide a default for the rules. I will try that next.

I tried all sorts of way to only specify when: on_success once, and not have to repeat it in every redefinition of the rules for the five extra variants. But none of them worked, so it looks like we do need to have it in each set of rules overrides.

https://git.drupalcode.org/issue/config_inspector-3413545/-/pipelines/74752 now shows this working, and has six phpunit jobs.

https://git.drupalcode.org/project/scheduler/-/pipelines/74770 also works, with just the five phpunit jobs.

That's great! I think it's ok to have the on_success repeated as the main goal is to make the rules more reusable and chainable, and that'll still be the case.

Have you tried turning off some of those jobs? For the sake of testing and to make sure that they don't run when they shouldn't?

The scheduler pipeline in #24 had opt_in_next_major 0 and it did not run. I wil do another with all of them off.

The reason why my scheduler testing jobs were all created (when working on the original mr) and I missed the problem, is that Scheduler has a custon "insert" of additional rules to re-define _PHPUNIT_EXTRA based on whether concurrent or not. The parameters passed to phpunit are diffeent to what run-tests.sh needs.

phpunit-scheduler-rules: &phpunit-scheduler-rules
  # Re-define _PHPUNIT_EXTRA depending on _PHPUNIT_CONCURRENT
  - if: '$_PHPUNIT_CONCURRENT == "0"'
    variables:
      # Specify parameters passed to PHPUNIT (needs --group)
      _PHPUNIT_EXTRA: $_PHPUNIT_PARAMETERS --group $_MATRIX_VALUE
  - if: '$_PHPUNIT_CONCURRENT == "1"'
    variables:
      # Specify parameters passed to RUN-TESTS.SH (without --group)
      _PHPUNIT_EXTRA: $_PHPUNIT_PARAMETERS $_MATRIX_VALUE

The presence of a positive match on either if: '$_PHPUNIT_CONCURRENT == "0"' or if: '$_PHPUNIT_CONCURRENT == "1"' was sufficient to invoke the implicit default when:on_success, so the jobs were added. In config_inspector, the phpunit jobs are entirely vanilla with no customisations.

The processing behind the rules section seems to be doubly prescribed, if thats the right word. In sequence, when you match a condition which triggers 'when:never' the job is not added. But the implication that if none of the 'when:never' conditions are met then the processing 'falls through' to a default 'when:on_success' is not the case.

skip_phpunit=1 and all opt-in values 0 - no jobs
https://git.drupalcode.org/project/scheduler/-/pipelines/74784

with previous_major and next_minor (but still with skip_phpunit=1) we get no jobs
https://git.drupalcode.org/project/scheduler/-/pipelines/74788

now changing skip_phpunit=1 we get three jobs
https://git.drupalcode.org/project/scheduler/-/pipelines/74792

setting previous_major and next_minor back to 0 we get just the base job
https://git.drupalcode.org/project/scheduler/-/pipelines/74800

It would be nice also to force a failure in one of the composer jobs, to make sure that the corresponding phpunit job run. I am unsure how "needs:" interacts with "when:on_success" and it would be good to make sure.

Forced failure in Composer previous major, and as expected the phpunit previous major does not run. The main phpunit did run ok.
https://git.drupalcode.org/project/scheduler/-/pipelines/74816

This is ready for your review. Do you have any more ideas for testing?

Just wondering if it would be worth adding config_inspector and scheduler to the manual downstream tests? config_inspector has a complex pipeline and scheduler has some customisatins. Both might throw useful light on any problem accidentally introduced. I presume the overhead is not too much, as you only run the downstream jobs just prior to committing.

Another intended composer failure: https://git.drupalcode.org/issue/config_inspector-3413545/-/pipelines/74863
It all looks really good, MR and tests.

@wim-leers - would you like to do a final review of the code? See all the different tests above.
The code is here: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/105 and the only difference with the reverted commit is the "when: on_success".

Let us know if you can review it or at least if we can coordinate when we merge this change.
It's RTBC from my side but it'd be great to have your sign off too.

--

About adding config_inspector and scheduler as downstream projects: not a problem for me, but I would need to be made maintainer of those projects so I can trigger the pipelines when needed. Maybe this can be done in a follow-up issue.

Each time we extract code into a reference we make it harder to just read and understand whats going on. I'm not necessarily opposed to this MR, but how often will modules need to override this? And how bad is it if they override the rules section when they do. I might prioritize understandability in this case and forego the references.

Sorry to mention this now after a lot of work has happenned.

That's a good point @moshe.

I thought something similar when I first saw the initial MR, but I also like the idea of having a set of rules that can be reused and chained, as opposed to a full copy/paste.

In fact, #3412308: Consider providing support for dependencies in "next major" tests → was just merged and #3397270: Nightwatch testing against all opted in versions → would introduce new variants too. We could and should reuse all these templates instead of repeating the rules over and over.

This also allows that if we change the criteria for a job to be run in the future, people with the reference will benefit from it, whereas people with a full copy/paste won't.

Back to needs review based on the above.

Sorry, I meant "Needs work".

I also see Moshe's point, but you could also look at it from the other way round. The abstracted re-usable references make the whole job step shorter and thus easier to read at a glance. Also, when you see the re-used references you know that they are common and standard, so you don't worry about what they do and your attention can be focussed on the specifc custom parts of the job. And I like flgarlin's realisation that these rules can also be used for other jobs not just phpunit. I had just seen those other two issues, and noticed the repeated coding.

I left the base rules name at phpunit-base-rules because that one specific to phpunit. But the name could still change, maybe phpunit-rules, or we could also make them all singular, i.e. next-major-rule, phpunit-rules, etc. When the reference is used in a job it is fine to see the singular name, even if we later add extra conditions into the reference, it is still a single rule component.

Also, should we use them in the composer variant jobs? If we are going to re-use, we should re-use everywhere we can.

I'm ok with both rule and rules, they are individual rules that we add, but a rule could actually be "a == 1 && b == 2".
I'm happy with sticking to singular for the argument you give.

I'd also make the names more verbose, as in ".skip-phpunit-rule" or ".opt-in-previous-minor-rule".

As we are working on reducing the duplicates and make them reusable, this one appears twice, so it can be abstracted as ".skip-phpstan-rules".

    - if: '$SKIP_PHPSTAN == "1"'
      when: never

I guess the rule for abstracting can be that the exact same rule is used in two separate places for the same purpose. So only do for now the ones that are "duplicated".

Re should we use them in the composer variant jobs? 100% agree, this issue has morphed a bit into making rules more consistent, chainable and reusable, so let's apply that everywhere we can.

I have address some of your points in #35 but not everything. Better to do this in discreet steps.
- Changed the existing 'rules' to singular 'rule'.
- Added 'composer-base-rule' and 'phpstan-base-rule' and implemented them.

This raised a couple of questions, so I have stopped here to ask

1. The original plain composer job, after filtering for skip_composer, had 'when:always' so I have used that instead of 'on_success' for the final line of the combined rules. As we discovered earlier, if all the rules are negative conditions for when not to run, then we need to add a specific on_success or always
2. The composer variants previously did check for skip_composer, they only had the conditional for the opt_in phpunit. I have added the composer-base rule to them all, which means that skip_composer=1 affects all of the composer jobs, not just the default original one. Checking that is what we want? Currently you could have skip_composer=1 but opt_in_next_major=1 and the composer variant would run. Maybe that was OK, and the opt-in should override the skip_composer? Are there scenarios where someone would want to test one of the opt-ins without doing the main composer job? what do you think? maybe I should revert that change.

Happy to leave the "when: always" as it was there already.
I also agree on 2. If we have SKIP_COMPOSER=1, that should skip it for all variants. I cannot think of scenarios where people won't even want composer at all!! So I'd leave the change there.

I think we still have to do

Make the names more verbose, as in ".skip-phpunit-rule" or ".opt-in-previous-minor-rule"

. ie: Does "phpunit-base-rule" means the rule to run it or to skip it?

Thanks for the feedback. Yes I'm happy you agree with both in #36/37 that's what I was hoping.

First two commits rename the rules as requested.

Third commit removes the 'skip-phpunit-rule' from all the composer variants, now that we have skip-composer-rule in all of those instead. There will be times when other jobs such as nightwatch or phpstan are needed to be tested with the opt-in variants, but phpunit might not be needed, either temporarily skipped or not required at all.

Three phpunit jobs, customised .gitlab-ci.yml uses the new rule names
https://git.drupalcode.org/project/scheduler/-/pipelines/75639

skip_phpunit set to 1, so get no phpunit, but we do get the composer variants required for phpstan
https://git.drupalcode.org/project/scheduler/-/pipelines/75643

Ready for review.

The code looks good to me and the right jobs seem to be running in the right places.

The rules can be reused (they are already reused throughout the template) and chained; and I think we've also fixed some inconsistencies around some jobs and the rules that were being used too.

The code looks good to me. RTBC.

I have just started a pipeline on the config_inspector testing MR
https://git.drupalcode.org/issue/config_inspector-3413545/-/pipelines/75731
All seems good. It had the forced error in Composer (next major), and the dependent jobs did not run, as expected.

I removed the forced error and all jobs ran as expected
https://git.drupalcode.org/issue/config_inspector-3413545/-/pipelines/75738

Thanks for the additional testing. I'll probably merge it on Monday.

We want to implement semver releases #3404175: Adopt (semver) versioning for gitlab_templates → and I might tackle this on Monday too and have this issue be the first to go through that release cycle.

I'll just merge this now as the other issue might still need more time to agree on the full format of tags.

Merged and marked as fixed. Thanks for the refactoring, I'm already mentioning this in other issues where we can reuse the rules.

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