Skip to content

Conversation

@lgoettgens
Copy link
Member

While working on #5449 and #5451, I need to repeatedly run the whole of the testsuite over and over again. To not spend too much of my lifetime, I am doing so in parallel on >40 cores.
When doing this, there are a few minutes where most cores are busy, and then one has to wait for several more minutes for the last one or two cores to finish up their task. The three test files that are touched in this PR are responsible for this, as running a single one of these test files alone needs more time than the other cores need for the rest of the testsuite.
Therefore, I propose to split these test files into smaller chunks that are easier to parallelize by our runtest.jl script.

I would just consider this a technical maintenance change, and would thus ask @benlorenz and @fingolfin for a review.

Nonetheless, let me ping the original authors of these tests (@tom111 @annahofer00, @StevellM @simonbrandhorst, @simonbrandhorst @HechtiDerLachs) to make them aware of the changes. I did my best to somehow do a split into sensible parts (in some cases oriented by how src/ is structured, in other cases just one file per testset). If any of you are unhappy about the concrete way the split is performed, but don't object to a split in general, I would suggest that you either provide a follow-up PR to this here or an alternative one.

Once this is merged, I'll experiment with putting the merge commit hash into .git-blame-ignore-refs to keep the original history in a git blame, but I am not sure if git does understand what's going on here.

@lgoettgens lgoettgens added testsuite Everything concerning testsuite release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Oct 14, 2025
@tom111
Copy link
Contributor

tom111 commented Oct 14, 2025

For the local cohomlogy / injective resolutions tests this is completely fine. You just split three entirely independent computations.

@StevellM
Copy link
Member

For QuadFormAndIsom it looks OK, though two files, printings.jl and setup_tests.jl, are not included.

@lgoettgens
Copy link
Member Author

For QuadFormAndIsom it looks OK, though two files, printings.jl and setup_tests.jl, are not included.

They are not included in the long testset. printings.jl only needs a few seconds and thus I didn't consider it to be a "long" test. setup_tests.jl is a magic file name in our setup, resulting in the contained code running directly before each of the other test files in the same directory runs. This way, the setting is set for each of the (now multiple) test files without copying the few lines over into each of them.

Copy link
Member

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, no idea how the ignore revs would work for the moved code blocks.

Copy link
Collaborator

@simonbrandhorst simonbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my side (EllipticSurface.jl)

@lgoettgens lgoettgens merged commit 664d7f4 into oscar-system:master Oct 14, 2025
32 of 37 checks passed
@lgoettgens lgoettgens deleted the lg/split-long-test-files branch October 14, 2025 16:36
@lgoettgens lgoettgens mentioned this pull request Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes testsuite Everything concerning testsuite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants