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

Move test_save_load_roundtrip to init file #2801

Merged
merged 3 commits into from Sep 18, 2023

Conversation

lgoettgens
Copy link
Member

When running the tests with multiple workers, the FTheoryTools and QuadFormAndIso tests may fail, when they are run on a different worker than the serialization tests. This is due to the latter adding a test function test_save_load_roundtrip that is used by the former two.
My solution is to move the test_save_load_roundtrip function to a file that gets included @everywhere right before running any tests. When similar things come up in the future, we may put them there as well.
The name of the file is open for discussion.

Copy link
Collaborator

@antonydellavecchia antonydellavecchia 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, I would just like to see if we can put the init file one level deeper and then merge

test/runtests.jl Outdated Show resolved Hide resolved
@benlorenz
Copy link
Member

benlorenz commented Sep 15, 2023

An alternative would be to add a conditional include to the relevant test files:

isdefined(Main, :test_save_load_roundtrip) || include(joinpath(Oscar.oscardir,"test","Serialization","init_tests.jl"))

This would allow running the tests separately from the main runtests without manually including that file.

@lgoettgens
Copy link
Member Author

An alternative would be to add a conditional include to the relevant test files:

isdefined(Main, :test_save_load_roundtrip) || include(joinpath(Oscar.oscardir,"test","Serialization","init_tests.jl"))

This would allow running the tests separately from the main runtests without manually including that file.

That seems like a better idea. I will change this pr accordingly.

@lgoettgens lgoettgens marked this pull request as draft September 15, 2023 08:35
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #2801 (a9775b0) into master (ebee98c) will increase coverage by 0.01%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2801      +/-   ##
==========================================
+ Coverage   73.61%   73.62%   +0.01%     
==========================================
  Files         455      455              
  Lines       64469    64514      +45     
==========================================
+ Hits        47459    47501      +42     
- Misses      17010    17013       +3     
Files Changed Coverage
experimental/QuadFormAndIsom/test/runtests.jl ø
experimental/FTheoryTools/test/tate.jl 100.00%

@lgoettgens lgoettgens marked this pull request as ready for review September 16, 2023 11:05
@benlorenz benlorenz merged commit 81b983d into oscar-system:master Sep 18, 2023
13 of 15 checks passed
@lgoettgens lgoettgens deleted the lg/parallel-tests branch September 18, 2023 12:28
fieker pushed a commit that referenced this pull request Sep 29, 2023
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.

None yet

4 participants