Needs work
Project:
Drupal core
Version:
main
Component:
phpunit
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
11 Sep 2023 at 19:56 UTC
Updated:
6 Oct 2023 at 15:02 UTC
Jump to comment: Most recent, Most recent file

Comments
Comment #3
larowlanAdded screenshot to issue summary
Comment #4
catchWould 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.
Comment #5
larowlanYeah 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.
Comment #6
larowlanSample pipeline showing this works - https://git.drupalcode.org/project/drupal/-/pipelines/19253
MR currently has patch from #3180588: "Enforced" Dependencies of Optional Configs Overwrite Other Dependencies as a proof of concept
Comment #7
larowlanI guess we postpone this on the parent?
Comment #8
catchLet's postpone, but also try to get it in asap once the main issue lands. The output looks really good!
Comment #9
catchComment #10
smustgrave commentedWonder if this could include a CR or note how to do a test-only in gitlab? is it automatic?
Comment #11
bbralaSince it was merged, we need a rebase since this includes some changes that now live in core.
Comment #12
fjgarlin commentedAnswering #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).
Comment #13
catchYeah 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.
Comment #14
smustgrave commentedThat. Is. AMAZING!!
So it just automatically picks up test files?
Comment #15
catchYes 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.
Comment #16
larowlanComment #17
larowlanRebased on top of 11.x and removed the code from #3180588: "Enforced" Dependencies of Optional Configs Overwrite Other Dependencies
Comment #18
bbralaI 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.
Comment #19
andypostComment #20
larowlanAddressed reviews and fixed the out of date partial names
Comment #21
smustgrave commentedDon't want to interrupt the work here but may need a rebase? But there are some open threads from what I can tell.
Comment #22
larowlanComment #23
smustgrave commentedPicked #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.
Comment #24
larowlanThink I've fixed that, didn't revert #3356684: Generic Revision UI's Revision overview page generates wrong operations/view links for a translation yet
Comment #25
larowlanStill 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
Comment #26
longwaveDo 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.
Comment #27
larowlanDo 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?
Comment #28
catchYeah 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.
Comment #29
fjgarlin commentedI think I fixed the test-only script part.
https://git.drupalcode.org/issue/drupal-3386566/-/jobs/105712
Comment #30
catchLooks like this might need review again?
Comment #31
smustgrave commentedI 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?
Comment #32
fjgarlin commentedThe 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).
Comment #33
catchThis also seems like a feature for making sure that changes won't cause test failures against HEAD :)
Comment #34
smustgrave commentedSo 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.
Comment #35
larowlanI 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
Comment #36
catchThis 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.
Comment #39
longwaveFantastic 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!
Comment #40
xjmThis is awesome!
However, it needs:
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.)
To be added to the core gate item about test-only patches, linking the above docs.
Comment #41
catchWe 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.
Comment #42
bhanu951 commentedIn reference to #26 we need to fix this issue #3268489: [PP-upstream] Rename test classes not ending with Test to end with Test
Comment #43
bhanu951 commentedcatch 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
Comment #44
bhanu951 commentedComment #45
xjmAgreed with #41; despite all the fast simplicity of GitLab CI, I find it really difficult to actually see the relevant results.
Comment #46
catchI opened #3390878: [GitLab CI] Improve failure reporting summary.
Comment #47
poker10 commentedI have created #3391114: Remove variables export from test-only job, as it seems like that we have reintroduced variables export in this commit.
Comment #48
penyaskitoWe 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?
Comment #49
berdirCame 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?
Comment #50
catchThat seems like a good idea to me too, will catch more changes that way.