Problem/Motivation

With patches you can upload a test-only patch to demonstrate we have new test coverage
We need a way to replicate that with Gitlab CI

Steps to reproduce

Proposed resolution

Add a manual job to run 'test only changes'

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 Screenshot from 2023-09-12 09-26-09.png240.61 KBlarowlan

Issue fork drupal-3386566

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Issue summary: View changes
StatusFileSize
new240.61 KB

Added screenshot to issue summary

catch’s picture

Status: Active » Needs review

Would never have believed this would be easier than patches. There will be cases that this doesn't cover, but it should cover 90% of issues where we'd ask for a 'test only' patch or more than that.

larowlan’s picture

Yeah this is also better on the $$ than patches because we just run the changed tests and not the whole suite.

Pushed some changes to split out how I revert deletions and modifications from additions using diff-filter.

The cases it won't catch are nightwatch tests. Still thinking about how to handle that, but they're nowhere near as common.

larowlan’s picture

larowlan’s picture

I guess we postpone this on the parent?

catch’s picture

Title: Add support for 'test only' changes to gitlab CI » [PP-1] Add support for 'test only' changes to gitlab CI
Priority: Normal » Major

Let's postpone, but also try to get it in asap once the main issue lands. The output looks really good!

catch’s picture

Title: [PP-1] Add support for 'test only' changes to gitlab CI » Add support for 'test only' changes to gitlab CI
smustgrave’s picture

Wonder if this could include a CR or note how to do a test-only in gitlab? is it automatic?

bbrala’s picture

Status: Needs review » Needs work

Since it was merged, we need a rebase since this includes some changes that now live in core.

fjgarlin’s picture

Answering #10. Yes, the idea is that it is automatic. No need to upload test-only branches or patches, the Gitlab CI job will do it for you (and the reviewers).

catch’s picture

Yeah there'll be a button to press on the pipeline UI to get the test only run, but no extra MR to create, no patch to upload etc.

smustgrave’s picture

That. Is. AMAZING!!

So it just automatically picks up test files?

catch’s picture

Yes so what the new pipeline actually does is:

1. For every change that's not to an test file, checks it out from origin.
2. Lists the remaining files - which is just the tests now.
5. Runs phpunit against those files - so if there is one method addition to a test class, it'll run that test class, if a patch updates 15 tests, it runs those 15.

It's an individual pipeline job which is manually triggered alongside the other pipeline jobs, and it is set not to cause the entire pipeline to fail. So you can get a passing pipeline, + a test only job which you manually start, and when it fails, that means the tests failed.

There are occasionally issues where this approach won't work - say you need half a change to make the tests run at all but omit something else to show the test failure, but those are pretty rare and we could handle it ad-hoc issue by issue - possibly with extra 'draft' MRs, possibly something else - like reverting a commit, running a pipeline, taking a screenshot, reinstating the change. But they're extremely rare so we can worry about that much later if at all.

larowlan’s picture

larowlan’s picture

Status: Needs work » Needs review
bbrala’s picture

I think this is a great start. We might need to consider that there might be changes in something like composer.json. What is a change is also adding/updating a package since that broke. Then we need to composer install. I don't think we'd want to do this manually, but perhaps some sort of check for that is a good idea.

This could also be a follow up, to not run tests if [insert list of files] is changed.

andypost’s picture

larowlan’s picture

Addressed reviews and fixed the out of date partial names

smustgrave’s picture

Status: Needs review » Needs work

Don't want to interrupt the work here but may need a rebase? But there are some open threads from what I can tell.

larowlan’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Picked #3356684: Generic Revision UI's Revision overview page generates wrong operations/view links for a translation for testing

Got a failure when I triggered the test-only build

fatal: ambiguous argument 'origin/11.x': unknown revision or path not in the working tree.

larowlan’s picture

Status: Needs work » Needs review
larowlan’s picture

Status: Needs review » Needs work

Still not working, something wrong with the bash commands, pausing for today

https://git.drupalcode.org/project/drupal/-/jobs/102286 didn't checkout the non test changes so the tests passed

longwave’s picture

Do we have a better way of identifying tests? Not all of them end in Test.php, although maybe we should have a policy and test coverage to ensure this is consistent.

larowlan’s picture

Do we have many tests in core that use the @test annotation? Short of that I thought the file name had to end in Test.php to be found by phpunit?

catch’s picture

Yeah discovery looks for Test.php so I think everything actually does end with that, or at least the class name has to finish with Test which is the same thing.

fjgarlin’s picture

I think I fixed the test-only script part.

https://git.drupalcode.org/issue/drupal-3386566/-/jobs/105712

catch’s picture

Status: Needs work » Needs review

Looks like this might need review again?

smustgrave’s picture

Status: Needs review » Needs work

I see that the pipeline failed in a few spots. Kernel, FunctionalJavascript.

Also looking at the test-only spot the failures it's reporting I don't believe are from this MR. HttpExceptionNormalizerTest was not something that was changed?

fjgarlin’s picture

Status: Needs work » Needs review

The last pipeline fails were widespread across all MRs, with GitLab not being able to spin up runners, those should be fixed already.

I rebased the branch. The pipelines are running again.

Regarding the files, if the branch is not rebased it can show different files which are not in this branch but made it to the HEAD of 11.x.
I added an informative message after listing the "changed" files prompting users to rebase the branch if this happens.

For example:
* https://git.drupalcode.org/issue/drupal-3386566/-/jobs/124327 reported some files and could not find those
* Those files were added after the last rebase here https://git.drupalcode.org/project/drupal/-/commit/5a6c7983ac3c6f4e46f96...

Back to Needs Review (will keep an eye on the pipeline).

catch’s picture

Regarding the files, if the branch is not rebased it can show different files which are not in this branch but made it to the HEAD of 11.x.

This also seems like a feature for making sure that changes won't cause test failures against HEAD :)

smustgrave’s picture

Status: Needs review » Needs work

So the pipeline is showing all green, tests are showing all green.

When I run the test-only though I get errors https://git.drupalcode.org/issue/drupal-3386566/-/jobs/124477 from the last ticket to be merged.

larowlan’s picture

Status: Needs work » Needs review

I think it just needed a rebase, MRs aren't like patches in so far as they don't apply to HEAD, they apply to the point in time

catch’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me, there's no test changes in the branch so we can't check the latest MR's output (and I realised this after pushing the button, not before), but there are successful examples from earlier in the issue. Once it's merged we can try it out on core MRs and will find any issues that way, it's only running on-demand so shouldn't introduce instability.

  • longwave committed d60aab72 on 10.1.x
    Issue #3386566 by larowlan, fjgarlin, smustgrave, catch: Add support for...

  • longwave committed 3f49e8cd on 11.x
    Issue #3386566 by larowlan, fjgarlin, smustgrave, catch: Add support for...
longwave’s picture

Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Fantastic to see this working, we might need some tweaks in followups but let's see how we get on with this - and hopefully it should make it much easier than uploading separate test-only patches.

Committed and pushed 3f49e8cdca to 11.x and d60aab72b3 to 10.1.x. Thanks!

xjm’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation updates

This is awesome!

However, it needs:

  1. To be documented in the hanbdook somewhere, with the screenshot from the IS (probably along with everything else about GitLab CI for issue users, like the little green check/red x, how to find the specific results of failed jobs because half the time a text search gets "stuck" in the results output and there's no clear build status with a fail message that I can find, etc. etc.)
     

  2. To be added to the core gate item about test-only patches, linking the above docs.

catch’s picture

We need an issue, not sure if one is open or not, for better failed result summaries. i.e. at least at the job level but ideally at the pipeline level we should be able to show which tests failed and with what message etc. Right now I am relying on ctrl-f. Keep forgetting to search/open issues though.

bhanu951’s picture

catch in#28:

> so I think everything actually does end with that, or at least the class name has to finish with Test

That is still not the case there are still test classes without ending with Test.php

bhanu951’s picture

Issue tags: +Needs followup
xjm’s picture

Agreed with #41; despite all the fast simplicity of GitLab CI, I find it really difficult to actually see the relevant results.

catch’s picture

poker10’s picture

I have created #3391114: Remove variables export from test-only job, as it seems like that we have reintroduced variables export in this commit.

penyaskito’s picture

      if [[ $(git diff refs/heads/${TARGET_BRANCH} --diff-filter=DM --name-only|grep -Ev "Test.php$"|grep -v .gitlab-ci|grep -v scripts/run-tests.sh) ]]; then

We are filtering *Test.php.
IMHO we should use */tests/* instead. When we move this to the gitlab contrib templates, I've used to have mock data too in the tests folder.
This might actually be relevant for core too if there are fixes to e.g. Upgrade tests with db dumps.

Didn't create a new issue, but we might if you agree?

berdir’s picture

Came here to comment on that as well. test-only tests often also require changes to/new test modules and other information that is most of the time found in tests folders. I guess there are exceptions to that, but they are probably pretty rare?

catch’s picture

That seems like a good idea to me too, will catch more changes that way.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.