-
Notifications
You must be signed in to change notification settings - Fork 65
Improve testing on ParallelTestRunner #878
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
Conversation
test/runtests.jl
Outdated
| return true, val | ||
| # Force 4 workers if running on buildkite | ||
| if parse(Bool, get(ENV, "BUILDKITE", "false")) | ||
| jobs_pos = findfirst(arg -> startswith(arg, "--jobs"), ARGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor change:
| jobs_pos = findfirst(arg -> startswith(arg, "--jobs"), ARGS) | |
| jobs_pos = findfirst(startswith("--jobs"), ARGS) |
Also, you aren't really interested in the position, so you can also do something like
if !any(startswith("--jobs"), ARGS)
push!(ARGS, "--jobs=4")
end(similarly below)
test/runtests.jl
Outdated
| # Hostcall tests must run on main thread (not in parallel workers) | ||
| delete!(testsuite, "device/hostcall") | ||
| delete!(testsuite, "device/output") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for reference, there's interest in having a way to specify serial tests: JuliaTesting/ParallelTestRunner.jl#77. I discussed with Valentin a way to implement it, but haven't found the time to do it (my need for this feature shifted, at least in the short term, but I think it'd still be nice to have, and I may still need it later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads-up. I will leave things for now, and happy to move to the new implementation when available for use. I'll leave a comment in the file with link to the issue.
|
Thanks for the improvements. LGTM |
|
Thanks. I will wait until CI runs and then merge. |
Following up on switching to ParallelTestRunner.jl as testing infra in #876 , there are several points that were not fully resolved such as:
We now only miss the actual way of selecting tests based on some pre-defined categories as it "used to be":
For this reason, I partly renamed the tests as one could now pass either or combination of those terms in order to trigger similar behaviour as it used to be prior to #876 , e.g.,
without clashing with ParallelTestRunner setup. If there is a better way to address this last bit without too much of a machinery, then we could give it a shot.
In any case, feedback welcome!