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

Better way to run parallel test processes in different directories #48399

Closed
wants to merge 3 commits into from

Conversation

BrettDong
Copy link
Member

@BrettDong BrettDong commented Apr 7, 2021

Summary

None

Purpose of change

My solution to avoid concurrent filesystem access in #47134 is not ideal. I drilled a hole in the WORLD class to make test processes run in unique worlds named by its PID, but @jbytheway suggested a more elegant solution: pass --user-mod=<dir> to cata_test. I wanted to change it but was distracted by other things.

Describe the solution

Repay the technical debt. Adopt @jbytheway's suggestion.

Testing

Wait for tests to complete on this pull request.

@BrettDong BrettDong added the Code: Tests Measurement, self-control, statistics, balancing. label Apr 7, 2021
@actual-nh
Copy link
Contributor

If one is using the parallel mechanism to run the equivalent of a "matrix" - more than one changing substitution - what's the recommended way to do this with --user-dir? --user-dir=something depending on {1} and {2}, I would guess - which would need to remove any filesystem-sensitive characters from the {1} and {2}? See #47791 for one particular case involved.

@BrettDong
Copy link
Member Author

What comes to my mind is $(echo -n {1} | base64

@jbytheway
Copy link
Contributor

If you wanted to make them look as similar as possible to the input but avoid a ew problem characters you could convert problematic characters with something like sed, but if you don't care to make it human-readable then I think the base64 solution is reasonable.

@@ -165,8 +165,8 @@ then
make -j$num_jobs
cd ..
# Run regular tests
[ -f "${bin_path}cata_test" ] && parallel --verbose --linebuffer "run_test $(printf %q "${bin_path}")'/cata_test' {} '('{}')=> '" ::: "crafting_skill_gain" "[slow] ~crafting_skill_gain" "~[slow] ~[.]"
[ -f "${bin_path}cata_test-tiles" ] && parallel --verbose --linebuffer "run_test $(printf %q "${bin_path}")'/cata_test-tiles' {} '('{}')=> '" ::: "crafting_skill_gain" "[slow] ~crafting_skill_gain" "~[slow] ~[.]"
[ -f "${bin_path}cata_test" ] && parallel --verbose --linebuffer --link "run_test $(printf %q "${bin_path}")'/cata_test' {2} {1} '('{1}')=> '" ::: "crafting_skill_gain" "[slow] ~crafting_skill_gain" "~[slow] ~[.]" ::: "--user-dir=process1" "--user-dir=process2" "--user-dir=process3"
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to change the run_test function to accept the new argument. (Or you can instead append the command line option to the first argument, I suppose.)

@stale
Copy link

stale bot commented Jun 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Jun 3, 2021
@BrettDong BrettDong closed this Jun 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code: Tests Measurement, self-control, statistics, balancing. stale Closed for lack of activity, but still valid.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants