Add coverage for test-only changes job

We could somehow alter a file using the before_script and maybe we also need to override the rules section for the "test-only job" on one of the files so it always runs as well.

Yes we can alter files in before_script but even if we changed the rules to trigger the job, I am not sure that this "diff" would pick up the changes when the pipeline is not running from a Merge Request. It might do, so worth a try. But also altering the rules kind-of invalidates the overall testing of the job.

I will try the above, but it might end up that we create a special MR here, marked as draft, which remains open and we run it in a new downstream pipeline (with other jobs skipped so we don't duplicate and hence waste resource each time)

We can only do it on one branch, or just do it for the parent issue (the one of failing curl) and then revert the change.
Or we can add a new rule that will trigger it, like setting a variable, which we could set in the gitlab_templates project when calling the downstream pipelines.

Something like this:

  rules:
    - if: '$FORCE_TEST_ONLY_JOB == "1"'
       when: always
    - *opt-in-current-rule
    - *skip-test-only-changes-rule
    - changes:
        - '**/tests/**/*Test.php'
      when: manual

Now that ✨ Allow users to specify custom files that Test-only changes Active provides the ability to run Test-Only changes on non-MR pipelines this provides a great way for us to add test coverge in GTD. I have opened a second Merge Request MR396 to remove the restriction on pipelines via the web UI, to see if we can use the functionality here in a downstream pipeline.

For the base branch to compare against, I can think of three possibilities initially

1. Try to use another of the existing branches, or main. This would likely give unhelpful differences, but it may be enough to at least demonstrate the job.
2. Manually create a new (static) copy of the branch we are going to use, called say d10-plus-duplicate, or something like that. This is simple to do, if we are OK with having another branch in this project? We could keep it in line with d10-plus quite easily as the branches do not change that often, but we'd have full control over which files are different.
3. We could get fancy and do a dynamic git clone in a test-only changes: before_script to create the branch to compare against. This is harder, and it would also require a dynamic commit to make a changed file, otherwise there would be nothing to revert

I think option 2 is probably the way to tackle this.

I am using this issue/gitlab_templates_downstream-3523139 branch to test the changes in the second MR on ✨ Allow users to specify custom files that Test-only changes Active - MR396. When the pipeline runs in MR29 here the Test-only job is added as expected. But when I run a pipeline via the branch UI then no test-only job is added. The branch contains the two curl_tempates variables in the .gitlab-ci.yml and that is not the the problem, the correct MR396 code is being used. I have tried setting _TEST_ONLY_TARGET_BRANCH to d9-basic and also refs/heads/d9-basic but it seems that the changes: compare_to always returns false. Locally the path '**/tests/**/*Test.php' does give changes, when I run git diff d9-basic **/tests/**/*Test.php.

I presume the "changes: compare_to" can use any branch, e.g. a sibling branch, it does not have to compare to a direct ancestor? I'd like to know what the scenario was that @codebymikey used, when getting this to work via UI. Then I can try to replicate that.

Based off the documentation

Using compare_to with merged results pipelines can cause unexpected results, because the comparison base is an internal commit that GitLab creates.

It appears due to how Gitlab checks out the repo, the compare only works for clean commits where the commit hash it's comparing to is within its commit history.

In this case:
1. 3523139-test-only-changes -> 0603fba1 gives you a valid test-only changes pipeline.
2. The latest commit on the d9-basic branch (3cbc82f1) doesn't exist on the 3523139-test-only-changes branch, so it's unable to provide you with a test-only change diff for it.

Was able to trigger it on the 3523139-test-only-changes branch with:

_GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
_GITLAB_TEMPLATES_REF: 3539542-test-only-part-2
# The test-only pipeline isn't available because the branches need rebasing so that they share similar commit references.
#_TEST_ONLY_TARGET_BRANCH: d9-basic
# This works because the commit reference exists relative to the current branch.
_TEST_ONLY_TARGET_BRANCH: 0603fba103772e05c2c1357e55af2186aaeb4bd6
# Mainly for debugging purposes.
CI_DEBUG_TRACE: true

tldr; due to how Gitlab internally checks out the code, the branch the pipeline is running from needs to have the target branch's commit ref somewhere in its commit history, so a rebase is most likely needed.

... due to how Gitlab internally checks out the code, the branch the pipeline is running from needs to have the target branch's commit ref somewhere in its commit history,

Ah, that is what I was trying to ask about in my last comment. Thanks for clarifying.

In your pipeline above I triggerd the test-only job. Here is where it starts running the test-only.sh script
https://git.drupalcode.org/issue/gitlab_templates_downstream-3523139/-/j...
The log says that the only changed file was composer.json, which is strange because the commitref you gave 0603fba103772e05c2c1357e55af2186aaeb4bd6 is six commits back in this 3523139-test-only-changes branch, and there are changes to .module, a test file and .gitlab-ci.yml

The composer.json is modified at runtime, and that is the only change detected. So more investigation is needed. Any ideas?

Re #6: if we need to create another branch to make this easier (option 2), we can do it.

The composer.json is modified at runtime, and that is the only change detected. So more investigation is needed. Any ideas?

It was because the original git ls-remote --sort="v:refname" origin "$TARGET_BRANCH" | tail -n1 | cut -f 1 call was written to pick up branch/tag references, but wasn't made to handle scenarios where the _TEST_ONLY_TARGET_BRANCH is set to a commit hash.

I've pushed an update to cater for it. The updated pipeline's available here.

Thanks for the enhancement, but I think that was not actually my problem. The error was caused by a missing _ at the start of the variable name. This was in the original MR and not spotted until now. I pushed this commit yesterday before yours, and it fixed my problem. I could then use

_TEST_ONLY_TARGET_BRANCH = d10-plus
_TEST_ONLY_BASELINE = 520125b0b88cbbbefd581174aaf254a0a98639c2

which is the last commit in the d10-plus branch that is common with this issue fork. It worked as expected, and just like when it is run in the MR pipeline. The log shows

Changes from d10-plus branch (SHA 520125b0b88cbbbefd581174aaf254a0a98639c2)

and the job ended amber (which is right) because the updated test fails when the .module has been reverted.

The test-only job you linked ended green, which implies it is not doing what we expected. It has

Changes from 0603fba103772e05c2c1357e55af2186aaeb4bd6 branch (SHA 0603fba103772e05c2c1357e55af2186aaeb4bd6)

i.e. the same on both sides. This is because the commit ref was entered into the _TEST_ONLY_TARGET_BRANCH variable which then also gets used for the BASELINE, because there was no value entered for _TEST_ONLY_BASELINE.

We need to decide how to treat the various combinations of input of the two variables, because running the job when target branch and baseline SHA are identical is just confusing.

Thanks for updating the d7 script, but let's not waste any time on doing that right now. The scripts are so similar that I have a plan to pass in a 'this is d7' argument to the main script, and then do away with the -d7 file altogether.

The test-only job you linked ended green, which implies it is not doing what we expected. It has

Ooh, I wasn't actually testing for failures, more that it was now able to pick up the appropriate Test.php file as being changed. The diff between the commits doesn't seem to have any changes that'd trigger an actual failure, hence why it passes.

We need to decide how to treat the various combinations of input of the two variables, because running the job when target branch and baseline SHA are identical is just confusing.

Yup, most users shouldn't use the hash reference as the _TEST_ONLY_TARGET_BRANCH, which is why it initially defaulted to the CI_DEFAULT_BRANCH.

I just erroneously did it as a quick test since 520125b0b wasn't the head of a referenceable branch name and was curious how it'd work.

I think it should work as was originally planned:
1. _TEST_ONLY_TARGET_BRANCH - should be a branch (or a commit hash - it should work, but is not officially supported)
2. _TEST_ONLY_BASELINE - should be an optional commit hash. If it's not declared, it should attempt to resolve it from _TEST_ONLY_TARGET_BRANCH which should be a git ref that shares a common history as the current one.

I have a plan to pass in a 'this is d7' argument to the main script, and then do away with the -d7 file altogether.

That sounds like a good idea!

The diff between the commits doesn't seem to have any changes that'd trigger an actual failure, hence why it passes.

Ah, yes you are right, well spotted. The .module did have a change which was reverted, but we need to go one further commit back, to 8080f53 to get the failure, as that is where I added the new function, to cause the job to end amber. That's good, all is explained :-)

I removed from the variables definition file the default value of $CI_DEFAULT_BRANCH for target branch, as that can be derived in the script. I think it is better for the script to be able to determine if the user has entered a value or not. It was already defaulting to that value anyway, so it was overkill to have it in the variables file too.

📌 Pass a "d7" parameter into scripts/test-only.sh and discard scripts/test-only-d7.sh Active will deal with the d7 file, so we could revert all the d7 changes here, to save effort.

In preparation for the MR downstream test, here a a few changes
https://git.drupalcode.org/project/gitlab_templates_downstream/-/merge_r...

All phpunit tests (current and next minor) and the test-only job are green, using current latest release of templates, which is 1.11.2
https://git.drupalcode.org/issue/gitlab_templates_downstream-3523139/-/p...

So I'm going to commit this, then re-work the MR29 which is the branch we will use in the downstream test.

Actually this needs a bit more checking before commit. The clean green run was a second test I did, via the web UI, to use _TEST_ONLY_FILE_PATTERN: "\.module" so that the .module was not reverted in the test-only job. That worked ok, and all the phpunit jobs (current and next minor) are also green.
https://git.drupalcode.org/issue/gitlab_templates_downstream-3523139/-/p...

But running the entire pipeline via a push to the MR branch - we get a different result for 'phpunit next minor - group 1'. It fails amber with a deprecation, this is probably because that test now installs the gitlab_templates_
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

But I do not understand why when running from the UI that phpunit job ended green. I'm not sure we want to commit to the real d10-plus branch when 'phpunit next minor' ends amber, as that will just cause us extra work and investigation when we see that in the GTD downstream pipelines.

I have worked out where the difference is coming from. The phpunit 'next minor' job is defined in the .gitlab-ci.yml with _PHPUNIT_CONCURRENT=1 so that we use run-tests.sh and can detect deprecations via SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=$CI_PROJECT_DIR/$_WEB_ROOT/core/.deprecation-ignore.txt" as intended, and this is what we see in the full MR pipeline phpunit next minor job

But I think the reason that the phpunit next minor job initiated via the UI passed green is that the .gitlab-ci.yml file does define any _PHPUNIT_CONCURRENT at the top level, so this value appears in the form, and it has its default value of 0. This value is applied to all the phpunit variants, and even overrides the customised value of 1 in the 'next minor' job. So 'next minor' therefore uses the phpunit binary, and does not display the deprecations. Is that because it does not use the SYMFONY_DEPRECATIONS_HELPER variable? Our help page implies that it should work in either concurrency setting, but we need to verify that.

The two current MRs as similar and against the same branch, which one should I review? (if any).

Still working on this. MR32 will be committed first. MR29 might remain in draft, for use in the downstream testing, not sure yet.

Current problem should be really simple but I've not found a solution. It works in local bash terminal but not in the pipeline. In words, I want to add the string \.deprecation\-ignore\.txt to the variable $_TEST_ONLY_FILE_PATTERN but if there is already a value in that variable, it needs a pipe | before the additional text. Simple. Or to make it short and readable, use $A as the variable, and extra as the text.

A+=$([[ $A == "" ]] && echo "" || echo "|")+"extra"

A+=$([[ $A == "" ]] && echo "" || echo "|")"extra"

A+=$([[ $A == "" ]] && echo "" || echo "|")extra

The above ways to add 'extra' all fail the pipe syntax, even though OK locally. I have also tried it inside a piped section. The pipeline fails to be built - eg https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

I always struggle with these too 😅
Maybe just use an "if".

pseudocode:

a = '';
if (sa != '')
  a = a + '|extra'
endif

or I just repeat the "extra"

A+=$([[ $A == "" ]] && echo "extra" || echo "|extra")

The conditional is working fine, it is the final concatenation that isn't. It just bugs me that I can't work out the correct way. The problem is that bash in the pipeline script does not behave exactly like the bash in my local terminal, so I'm taking guesses and pushing to see what happens :-)

It was not that line that was causing the pipe syntax error. The new comment I added at the same time started with a - which I had not noticed. That is what the problem was, and it all works correctly now. I've rebased to remove all the intermediate commits.
With no pre-existing value in the pattern
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/64...
Using UI and setting the pattern to '.module'
https://git.drupalcode.org/issue/gitlab_templates_downstream-3523139/-/j...

Oh, I totally missed that comment. Well spotted.

I want to add the string \.deprecation\-ignore\.txt to the variable $_TEST_ONLY_FILE_PATTERN but if there is already a value in that variable, it needs a pipe | before the additional text

For future reference, this can be achieved using the :+ parameter expansion without using a subshell, so it'd look something like this:

# Add deprecation ignore text file to the extra file pattern.
_TEST_ONLY_FILE_PATTERN="${_TEST_ONLY_FILE_PATTERN:+$_TEST_ONLY_FILE_PATTERN|}\.deprecation\-ignore\.txt"

Example script showcasing this: https://www.onlinegdb.com/9hWM_JbBE

However it might be easier for people not as familiar with bash to review if the if statement version is used instead - entirely up to your discretions.

Thanks, that's really good to know, and thanks for taking the time to write the demo script.

The version that is in the MR at present is

 # Add .deprecation-ignore.txt to the file pattern to be preserved. Include | if there is already a value for the pattern.
_TEST_ONLY_FILE_PATTERN+=$([[ $_TEST_ONLY_FILE_PATTERN == "" ]] && echo "" || echo "|")"\.deprecation\-ignore\.txt"

It's longer, and still not that readable. So I might use your new shorter one. I like to use new things that I've learned, and the comment should explain it adequately.

The pattern manipulation works fine now. Test via UI with \.module specified in the test pattern, so .module is not reverted (and neither is .deprecation-ignore.txt) and the test passes green as intended.
https://git.drupalcode.org/issue/gitlab_templates_downstream-3523139/-/j...

One thing I noticed is that, when running via the web UI, the derived SHA to compare back to has two more commits than when run just in the MR. Here is the job direct from MR
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/64...

d10-plus SHA 5bb804582959db2377cbd37980211a23e4d2f61e
5 files changes

But the job from web UI
https://git.drupalcode.org/issue/gitlab_templates_downstream-3523139/-/j...

d10-plus SHA 520125b0b88cbbbefd581174aaf254a0a98639c2
6 files changed, new script

Attached is an image of the git history, highighting these two commits. But it might be working fine and does not indicate a problem in the recent test-only work we did to allow it to run from the UI. You can see the info on the history. Maybe the issue fork is not up-to-date and that's why we get the extra two commits? Also I had to recreate this branch, and it may not be typical.

So and apart from the above question, ansd restoring the skipped jobs, this is ready for review. The purpose is to make sure the test-only jobs works correctly (hence the deprecation stuff) and also to add a simple function that can be modified in the real branch where we will do the downstream testing.