Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize tests to always use runtests.jl files #2813

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

fingolfin
Copy link
Member

... to group things together. Also merge test/Experimental content into the experimental directory.

This should be mostly orthogonal to PR #2810 by @benlorenz

@fingolfin fingolfin requested a review from benlorenz September 18, 2023 10:08
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.

thanks, looks good!

"StraightLinePrograms/runtests.jl"
# Automatically include tests of all experimental packages following our
# guidelines.
"../experimental/runtests.jl",
Copy link
Member

Choose a reason for hiding this comment

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

For the parallelization, it would make sense to not include experimental/runtests.jl as the last item, as that file contains a lot of tests that in total need a long time. And parallelization with pmap gets more efficient if the larger tasks are at the front and the smaller ones in the end of the list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can move it further up again.

That said, perhaps we should revise how we do this parallelization anyway: it would make more sense to parallelize all those include statements (under the assumption that they are all mutually independent, which they really should be).

For this, we could just take all the *.jl files under test which are not named runtests.jl, and then pass all of them to pmap. Then the runtests.jl files would just be for convenience (and they could all also become identical, with the same code that includes all "*.jl" files in "subdirectories of the current directory except for those named runtests.jl).

That would then also render the part of PR #2810 moot which checks for files that we accidentally did not include...

Copy link
Member

Choose a reason for hiding this comment

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

When I prepared #2810 I was considering including everything automatically as well. But I thought it would be easier / safer to just check the includes first.
Luckily there weren't that many missing includes. Automatically including all jl files in the test folders would be fine with me as well.

@@ -1,3 +1,6 @@
using Oscar
using Test
Copy link
Member

Choose a reason for hiding this comment

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

If we are using Test in basically every testfile, we do not check that it is already loaded in

@req isdefined(Base.Main, :Test) "You need to do \"using Test\""

Copy link
Member Author

Choose a reason for hiding this comment

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

But we currently don't do it in ever testfile, just in the runtests.jl files...

... to group things together. Also merge `test/Experimental` content
into the `experimental` directory.
@fingolfin fingolfin merged commit e38f9a0 into oscar-system:master Sep 18, 2023
@fingolfin fingolfin deleted the mh/runtests branch September 18, 2023 19:55
antonydellavecchia pushed a commit that referenced this pull request Sep 19, 2023
... to group things together. Also merge `test/Experimental` content
into the `experimental` directory.
fieker pushed a commit that referenced this pull request Sep 29, 2023
... to group things together. Also merge `test/Experimental` content
into the `experimental` directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants