Allow users to specify custom files that Test-only changes

Created on 4 August 2025, 2 months ago

Problem/Motivation

The test-only script has a hard-coded pattern of file paths that it allows to be exempt from reverts.

PATTERN='(tests/|composer\.json|\.info\.yml|phpunit\.xml|\.gitlab-ci\.yml)'

However there are some specific cases where a particular change requires a specific file to not be reverted in order to test the behaviour properly, so should be supported.

e.g. #3521812-7: Take non-Clean URL requests into account

Proposed resolution

We should either:
1. Export the existing the PATTERN variable as a global CI variable like _TEST_ONLY_FILE_PATTERN.
2. Introduce a new optional _TEST_ONLY_FILE_PATTERN_EXTRA variable that adds additional file patterns to the existing pattern list.

Remaining tasks

Feature request
Status

Active

Component

gitlab-ci

Created by

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

Merge Requests

Comments & Activities

  • Issue created by @codebymikey
  • 🇪🇸Spain fjgarlin

    I would just go as far as step 1. Do we really need step 2?

    It has a default value, so if somebody wants to alter it, the whole variable can be overwriten. This is something that you would only change for some particular tests.

  • I was weighing the need for it, the main advantage of the additional variable is that it makes it a bit easier to add specific patterns to your project/test while also leveraging the default global pattern set on the template.

    Personally would find it easier to simply add a dedicated stage_file_proxy\.services\.yml value to the pipeline variable, and have it be a bit more obvious as to why its being included rather than copying and updating the default variables from upstream.

    However I don't have a strong opinion either way as its an edge case variable and will be an improvement over the current behaviour.

  • 🇪🇸Spain fjgarlin

    Yeah, I can see how that second variable will make things more readable and it'd have the added benefit that if we make changes upstream, you'd still get them.

    Note that the same changes would be needed for test-only-d7.sh (we know D7 is EOL but if the changes are easy to port we are trying our best).

    Would you be willing to produce an MR for this?

  • I wasn't aware that you could actually modify the manual pipeline variables before running them, so initially worked on supporting test-only changes via the web pipeline.

    This can be tested on the https://git.drupalcode.org/issue/stage_file_proxy-3521812/-/pipelines/new issue fork.

    Test case 1: Merge request
    Previous behaviour: Tests can't be ran
    New behaviour: Test runs and fails gracefully.

    Test case 2: Manual pipeline with no tests
    Branch: 3.1.x
    Variables:

    _GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
    _GITLAB_TEMPLATES_REF: 3539542-test-only-patterns
    _TEST_ONLY_FILE_PATTERN: stage_file_proxy\.services\.yml
    CI_DEBUG_TRACE: true
    

    Result - doesn't create the "Test-only changes" job, because there are no test file changes.

    Test case 2: Manual pipeline with tests
    Branch: 3521812-test-web-pipeline
    Variables:

    _GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
    _GITLAB_TEMPLATES_REF: 3539542-test-only-patterns
    _TEST_ONLY_FILE_PATTERN: stage_file_proxy\.services\.yml
    CI_DEBUG_TRACE: true
    

    Result - creates the "Test-only changes" job since there are actual test file changes before the current branch and the default one.

    On a sidenote, even though the 3521812-test-web-pipeline branch has the _GITLAB_TEMPLATES_REPO and _GITLAB_TEMPLATES_REF variables overridden and hardcoded in the .gitlab-ci.yml, new manual pipelines which don't explicitly specify the override end up using:

    _GITLAB_TEMPLATES_REPO: project/gitlab_templates
    _GITLAB_TEMPLATES_REF: default-ref
    

    for some reason as can be seen here and here.

  • Pipeline finished with Failed
    2 months ago
    Total: 80s
    #567290
  • 🇪🇸Spain fjgarlin

    Yeah, sometimes variables inheritance / precedence gets in the way with those global variables.
    https://project.pages.drupalcode.org/gitlab_templates/info/testing-mrs/

    Thanks for the test links and the different cases. I'm having a look at the code.

  • 🇪🇸Spain fjgarlin

    The code looks great and the new additions are really useful in the context of the test-only job.
    I've raised one minor thing about the new documentation as the image will not render as it is right now.

  • Pipeline finished with Success
    2 months ago
    Total: 48s
    #567917
  • 🇪🇸Spain fjgarlin

    I think this looks good. There was additional testing on #8.

    It keeps what we have and only adds things on top (like triggering via web UI and adding the extra pattern), so I think it's also low risk
    RTBC.

  • Pipeline finished with Skipped
    2 months ago
    #568173
  • 🇪🇸Spain fjgarlin

    Merged. Great addition!
    Thanks so much.

  • 🇪🇸Spain fjgarlin

    Included in the latest tag and made available for all contrib too as part of the default ref.

  • 🇬🇧United Kingdom jonathan1055

    This is an excellent additional feature, thanks codebymikey. I was wondering if we might be able to make it even better, as I don't see any reason to restrict the new "compare to another branch, when not in a MR" feature to only work if the pipeline was submitted from the web UI $CI_PIPELINE_SOURCE == "web". If other pipeline_source values were allowed, or maybe we just detect if $_TEST_ONLY_TARGET_BRANCH has a different value from $CI_DEFAULT_BRANCH' to be the criteria for the job rule. This would allow the 'test only' job to be run in pipeines on non-default branches, when triggered by other means, which would allow us to work on the Gitlab Templates DownStream issue Add coverage for test-only changes job Active

  • 🇬🇧United Kingdom jonathan1055

    From #8 @codebymikey

    On a sidenote, even though the 3521812-test-web-pipeline branch has the _GITLAB_TEMPLATES_REPO and _GITLAB_TEMPLATES_REF variables overridden and hardcoded in the .gitlab-ci.yml, new manual pipelines on that branch which don't explicitly specify the override ....

    If the gitlab MR changes any scripts or other files that are retrieved via curl, then when running via a modified .gitlab-ci.yml you also need to set the two variables _CURL_TEMPLATES_REPO and _CURL_TEMPLATES_REF - See the last two lines on gitlab_templates/info/testing-mrs. It is a bit of a fuss, and we have tried various way to get round it, but it is not possible.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 93s
    #574323
  • This would allow the 'test only' job to be run in pipeines on non-default branches, when triggered by other means

    Yup! That seems like a great idea actually.

    It is a bit of a fuss, and we have tried various way to get round it, but it is not possible.

    Yeah, it does seem like it. I think from memory/personal experience with gitlab, and based on how the current variable precedence work, the best way to work around this is to lazily load the environment variables (similar to the .calculate-gitlab-ref) such that:
    1. The variable isn't set on the Project/Group variable since that takes precedence over whatever the project has in its .gitlab-ci.yml file.
    2. The _GITLAB_TEMPLATES_REPO and _CURL_TEMPLATES_REF are left empty in the main template, but the documentation references what the default value actually is.
    3. The _GITLAB_TEMPLATES_REPO and _CURL_TEMPLATES_REF variables are lazily exported in the gitlab-ci.yml script and set to the default value when empty (that way, the custom overrides should take precedence over the "default" empty value).

    Switching this to Needs Work so that it's not automatically closed as Fixed.

  • 🇬🇧United Kingdom jonathan1055

    Thanks for the details in #21. So that we don't get more sidetracked here, would you like to open a new issue, if you think there is a better way to do it and avoid the _CURL_TEMPLATES_REPO and _CURL_TEMPLATES_REF.

  • Pipeline finished with Success
    about 2 months ago
    Total: 150s
    #575669
  • 🇬🇧United Kingdom jonathan1055

    @codebymikey I am testing MR396 on #3523139-8: Add coverage for test-only changes job and would appreciate your help over there, when you have time to take a quik look. Thanks,

  • Pipeline finished with Success
    about 2 months ago
    Total: 50s
    #577768
  • 🇪🇸Spain fjgarlin

    Thanks both for the back and forth and improvements here. I will review it when ready. The changes suggested so far look good.

  • Pipeline finished with Success
    about 2 months ago
    Total: 51s
    #578271
  • 🇬🇧United Kingdom jonathan1055

    📌 Pass a "d7" parameter into scripts/test-only.sh and discard scripts/test-only-d7.sh Active is ready for review, and when that is committed I will merge it into this MR396 and continue here. Then we get D7 and D10 available for testing.

  • Pipeline finished with Success
    about 1 month ago
    Total: 81s
    #588250
  • Pipeline finished with Failed
    about 1 month ago
    #588932
  • Pipeline finished with Failed
    about 1 month ago
    #589077
  • 🇬🇧United Kingdom jonathan1055

    Now that 📌 Pass a "d7" parameter into scripts/test-only.sh and discard scripts/test-only-d7.sh Active is done and we no longer have to make near-duplicate changes in a d7 script, I have merged that in.

    Testing the script locally was not working for me (different flavor of bash / zsh) - in particular the array expansion from a string of changed files created by CHANGES_TO_REVERT=$(git diff ${BASELINE} --diff-filter=DM --name-only | grep -Ev $PATTERN). The for loop was only ever getting the first item, even if I referred to it using the longhand "${CHANGES_TO_REVERT[@]}". A solution that worked was to add a second set of ( ) around the definition.

    This also works in the pipeline job and the correct files are reverted.

    The problem is this now fails ShellCheck

    In scripts/test-only.sh line 71:
    CHANGES_TO_REVERT=($(git diff ${BASELINE} --diff-filter=DM --name-only | grep -Ev $PATTERN)) || true
                       ^-- SC2207 (warning): Prefer mapfile or read -a to split command output (or quote to avoid splitting).
    For more information:
      https://www.shellcheck.net/wiki/SC2207 -- Prefer mapfile or read -a to split command output
    

    See the Check Code job log and and https://www.shellcheck.net/wiki/SC2207
    None of the suggested resolutions work for me locally, so I don't think I'm on the right track. Maybe the original default array expansion was OK left as-is, as it works in the Gitlab job, just not in my local terminal session where I am doing the development. There should be a way round this, or maybe I just change the code to test, then revert before pushing. But that has many dangers.

  • 🇪🇸Spain fjgarlin

    So, what do you get in CHANGES_TO_REVERT without the wrapping parentheses? Did you output that?

    Also, https://www.shellcheck.net/wiki/SC2207 suggests that maybe adding some wrapping double quotes to the part inside the parentheses might fix the issue.

  • 🇬🇧United Kingdom jonathan1055

    The existing (as in main, before any changes in this MR) without the extra wrapping parenthesis, locally gives all the changed files in one "string" but includes line breaks. The printed output is:

    CHANGES_TO_REVERT=.prettierignore
    scheduler.module
    scheduler.services.yml
    

    Then the for loop acts on the entire value in one, so it tries

    executing: git checkout 89456f774b3d226c0234d055c6e281fbd1fd1ff4 -- .prettierignore
    scheduler.module
    scheduler.services.yml
    

    which gives the error

    error: pathspec '.prettierignore
    scheduler.module
    scheduler.services.yml' did not match any file(s) known to git
    adding some wrapping double quotes to the part inside the parentheses might fix

    I saw that and tried, but no better.

    I did try to alter the for file in $CHANGES_TO_REVERT; do but without success. Maybe I should try that angle again, and not alter the way the string in generated.

  • The output from a subshell is just a single string, so isn't available as an array to loop over.

    A couple approaches to parse the output properly is available here: https://onlinegdb.com/6IsOZCVHQ9

  • 🇬🇧United Kingdom jonathan1055

    Ah, thank you @codebymikey, those ideas are great. Thanks for the tip, I will use that site too.

    I have just discovered, though, that locally I get different behavior if run the script by calling it directly (all works OK, newlines in the list give separate loop values)

    CHANGES_TO_REVERT=.prettierignore
    scheduler.module
    scheduler.services.yml
    ==start of loop
    File -  .prettierignore
    File -  scheduler.module
    File -  scheduler.services.yml
    ==end of loop
    

    Whereas if I exuecute using source /the/script.sh it fails as above, with

    CHANGES_TO_REVERT=.prettierignore
    scheduler.module
    scheduler.services.yml
    ==start of loop
    File -  .prettierignore
    scheduler.module
    scheduler.services.yml
    ==end of loop
    
  • Pipeline finished with Success
    about 1 month ago
    #589237
  • 🇬🇧United Kingdom jonathan1055

    I used the loop with read and <<< "$CHANGES_TO_REVERT" which worked locally in testing and passes shellcheck. Thanks to @codebymikey again.

    In theory I should really fix the other array to loop bits, as they also do not work properly locally. They do work OK in the gitlab pipeline job, so I guess that is why we've not seen a problem so far.

    Testing via UI still works as expected, in that the correct tests are run. But even though Exiting with EXIT_CODE= when all tests passed, we still get ERROR: Job failed: command terminated with exit code 1 and the job ends amber. Not sure why that is, as it used to end green if no tests failed. More investigation needed.

    Now also I can start work on the downstream projects, to see if we can test there too (ie not MR and not UI)

  • Pipeline finished with Success
    about 1 month ago
    Total: 75s
    #589263
  • Pipeline finished with Success
    about 1 month ago
    Total: 110s
    #589282
  • Pipeline finished with Failed
    about 1 month ago
    Total: 49s
    #589913
  • 🇬🇧United Kingdom jonathan1055

    jonathan1055 changed the visibility of the branch 3539542-test-only-patterns to hidden.

  • Pipeline finished with Success
    about 1 month ago
    Total: 355s
    #589920
  • Pipeline finished with Failed
    about 1 month ago
    Total: 116s
    #590168
  • Pipeline finished with Success
    about 1 month ago
    Total: 50s
    #590174
  • Pipeline finished with Success
    about 1 month ago
    Total: 422s
    #590191
  • Pipeline finished with Success
    28 days ago
    Total: 124s
    #595298
  • Pipeline finished with Success
    28 days ago
    Total: 920s
    #595327
  • Pipeline finished with Success
    1 day ago
    Total: 268s
    #620538
Production build 0.71.5 2024