Split up update semver tests

Opened an MR.

UpdateSemverTestBase had a method with a data provider as well as multiple one off tests.

The one-off tests move to one trait.

The data provider tests move to another trait.

UpdateSemverContribTest is split to two new tests that each implement one trait each.

UpdateSemverCoreTest also does this, but is additionally gets UpdateSemverCoreSecurityCoverageTest which also uses a data provider, and the original test class is left with its one-off methods.

This means we split two mammoth tests into a total of six. They still each contain multiple test methods and take minutes to run but with 📌 Distribute @group #slow tests between test runners and mark more tests RTBC that might be within threshold.

The threshold is defined as - if all other tests run by a runner take x minutes to run, the slowest test should finish at the same time as, or before, other tests, and not artificially extend the run beyond this - assuming parallel tests and concurrency this is probably going to be between 3-5 minutes and we can tweak parallel tests and concurrency to match what a realistic finishing time looks like.

If you look at this job https://git.drupalcode.org/project/drupal/-/jobs/114753 (from the omnibus 'speed up gitlab ci runs' issue) you can see these tests starting first and finishing about half way through the job, which is ideal.

Those changes look good, can't RTBC here because I started it off.

Clever to split the tests like this to allow parallelization. I think the traits look good and the different tests are now nicely compartmentalized. I noted that UpdateSemverContribTestBase lost its class phpdoc while being renamed. UpdateSemverCoreTestBase class is newly added and did not get phpdoc. Are those not required anymore? :)

That's the only thing I found.

Added those docs.

Looks all good to me. I think the split of traits and base classes makes sense and its good to see how it speeds up tests.

I had some concerns on the comments when I looked last night. I’ll have to check again with the latest commits.

It’s better, but some of the new stuff has vague / duplicate comments, or nothing to help make sense of what’s what. I need to get my laptop out and make some concrete suggestions (or just push a commit). First, lunch. 😅

It was easier to push a commit to cleanup all the file comments. It's not totally perfect, but it's a lot less copy/paste and a lot more accurate. 😉 Also rebased to the very latest 11.x.

I'll note that UpdateSemverCoreTest is no longer really testing anything about semver for core as such. It's all the edge-case functional tests for various aspects of the Update Manager UI that we decided to add coverage for. Core uses semver, so this test uses all the semver* plumbing, but it's not really a test of "core semver" anymore. However, I don't think it's worth renaming it at this point. I left a slightly more verbose file comment at the top. Feedback welcome.

Thanks, y'all!
-Derek

p.s. Normally I wouldn't hold up important test performance issues like this over doc cleanups. But Update Manager tests are notoriously confusing and hard to follow, and I think it's crucial to try to make them as well documented as possible to make it feasible for other people to contribute to them over time...

Thanks/sorry,
-Derek

I reviewed your doc updates at https://git.drupalcode.org/project/drupal/-/merge_requests/4872/diffs?co... and they are good improvements IMHO.

lauriii → closed merge request !4872

Committed ef4cdf0 and pushed to 11.x. Thanks!

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