-
Notifications
You must be signed in to change notification settings - Fork 5
Switch from TestItemRunner --> ParallelTestRunner #88
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
==========================================
+ Coverage 94.43% 94.70% +0.26%
==========================================
Files 11 11
Lines 935 736 -199
==========================================
- Hits 883 697 -186
+ Misses 52 39 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is fun |
|
Side note, looks like the docs workflow would benefit a lot from caching |
|
Oh no, good fun or bad fun? 😂 |
|
Good fun, I'm surprised by people discovering this package before any public announcement 😅 |
|
haha, for sure. I like it a lot! I stumbled on it after following the breadcrumbs from Thomas' activity here julia-vscode/TestItemRunner.jl#113 |
|
I think I've had a change of heart with using TestItemRunner since discovering ParallelTestRunner. It combines the nice filtering capabilities of TestItems with the drop-in integration with Tests, and it is compatible with Julia v1.13. I added some brief usage examples to the contributor docs for your review. Are there any reservations about switching over to this newer test framework? |
README.md
Outdated
| const DATA_DIR = joinpath(@__DIR__, "data") | ||
| end | ||
| julia> runtests(Photometry, ["--verbose"]; init_code, test_filter = test -> occursin("aperture", test)) |
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.
Why not using Pkg.test? You don't need to do all the above.
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.
Right, that's shown in the Pkg.test() example right above that one, no?
Is there a way to pass filters to Pkg.test directly?
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.
Yes, if you don't override the test arguments
diff --git a/test/runtests.jl b/test/runtests.jl
index 6858225..fce2a4e 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -10,4 +10,4 @@ const init_code = quote
end
-runtests(Photometry, ["--verbose"]; init_code)
+runtests(Photometry, Base.ARGS; init_code)Then you can do things like
julia> using Pkg
julia> Pkg.test("Photometry"; test_args=`--list`);
# ...
Testing Running tests...
Available tests:
- aperture/circular
- aperture/elliptical
- aperture/overlap
- aperture/photometry
- aperture/plotting
- aperture/rectangle
- backgroud/background
- backgroud/estimators
- backgroud/interpolators
- detection/detection
Testing Photometry tests passed
julia> Pkg.test("Photometry"; test_args=`aperture`);
# ...
Testing Running tests...
Running 3 tests in parallel. If this is too many, specify the `--jobs=N` argument to the tests, or set the `JULIA_CPU_THREADS` environment variable.
│ │ ──────────────── CPU ──────────────── │
Test (Worker) │ Time (s) │ GC (s) │ GC % │ Alloc (MB) │ RSS (MB) │
aperture/plotting (3) │ 4.96 │ 0.40 │ 8.0 │ 710.77 │ 719.78 │
aperture/overlap (2) │ 6.76 │ 0.40 │ 6.0 │ 853.93 │ 729.97 │
aperture/circular (3) │ 1.19 │ 0.00 │ 0.0 │ 135.66 │ 738.62 │
aperture/elliptical (2) │ 0.47 │ 0.00 │ 0.0 │ 68.41 │ 733.27 │
aperture/rectangle (3) │ 0.97 │ 0.00 │ 0.0 │ 112.31 │ 744.61 │
aperture/photometry (1) │ 13.30 │ 0.64 │ 4.8 │ 2595.35 │ 847.80 │
Test Summary: | Pass Total Time
Overall | 9478 9478 17.5s
SUCCESS
Testing Photometry tests passed
julia> Pkg.test("Photometry"; test_args=`--verbose aperture`);
# ...
Testing Running tests...
Running 3 tests in parallel. If this is too many, specify the `--jobs=N` argument to the tests, or set the `JULIA_CPU_THREADS` environment variable.
│ │ ──────────────── CPU ──────────────── │
Test (Worker) │ Time (s) │ GC (s) │ GC % │ Alloc (MB) │ RSS (MB) │
aperture/photometry (1) │ started at 2025-10-21T23:46:23.085
aperture/overlap (2) │ started at 2025-10-21T23:46:23.094
aperture/plotting (3) │ started at 2025-10-21T23:46:23.094
aperture/plotting (3) │ 5.02 │ 0.39 │ 7.8 │ 710.82 │ 699.16 │
aperture/circular (3) │ started at 2025-10-21T23:46:30.214
aperture/overlap (2) │ 6.85 │ 0.40 │ 5.9 │ 853.93 │ 693.41 │
aperture/elliptical (2) │ started at 2025-10-21T23:46:31.916
aperture/circular (3) │ 1.18 │ 0.00 │ 0.0 │ 135.66 │ 699.16 │
aperture/rectangle (3) │ started at 2025-10-21T23:46:32.068
aperture/elliptical (2) │ 0.47 │ 0.00 │ 0.0 │ 68.40 │ 693.41 │
aperture/rectangle (3) │ 0.97 │ 0.00 │ 0.0 │ 112.31 │ 699.16 │
aperture/photometry (1) │ 13.39 │ 0.63 │ 4.7 │ 2595.44 │ 774.45 │
Test Summary: | Pass Total Time
Overall | 9478 9478 17.4s
aperture/plotting | 27 27 5.0s
aperture/overlap | 9226 9226 6.8s
aperture/circular | 21 21 1.2s
aperture/elliptical | 20 20 0.5s
aperture/rectangle | 12 12 1.0s
aperture/photometry | 172 172 13.4s
SUCCESS
Testing Photometry tests passedThere 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.
If you really wanted to force verbose tests you could use push!(copy(Base.ARGS), "--verbose"), but ParallelTestRunner.runtests is supposed to take command line arguments via Base.ARGS, if you override that you miss a good chunk of the interface
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.
Oh, and you can also do things like
Pkg.test("Photometry"; test_args=`aperture`, julia_args=`--threads=auto`)to pass Julia arguments to the tests, possibly different from the arguments of the current session, so you don't need to restart julia with --threads=auto just to change that for the tests.
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.
Oh, sick! Thanks for giving me a walk-through, adding this now
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.
If you think ParallelTestRunner documentation could be improved to make that clearer, please do submit a PR! And then you could also add this package to https://github.com/JuliaTesting/ParallelTestRunner.jl/blob/main/README.md#packages-using-paralleltestrunnerjl 😁
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.
If you think ParallelTestRunner documentation could be improved to make that clearer
Maybe it needs a section for those using the tests, in addition to the one about setting them up?
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! Just opened JuliaTesting/ParallelTestRunner.jl#60 to discuss
Co-authored-by: Mosè Giordano <765740+giordano@users.noreply.github.com>
giordano
left a comment
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.
For what is worth, this looks good to me. Tests on CI are ~20 seconds (out of ~60-70) faster than on main, not a massive improvement (could be better with more even load of the tests across the files, one is sensibly larger than all the others), but I think it's good nevertheless
|
Interesting, thanks for taking a look at the timings! |
Other than the single comment about test variable |
|
Great, thanks for the review! @cgarling does this look good to merge to you? |
|
Sorry for the late additions. I thought it would be good to tighten up the imports a little. Would probably be good to do a pass with ExplicitImports.jl after this PR is merged |
|
I don't have time to do a full review right now (sorry) but I did a quick skim and it looks good. Also ParallelTestRunner seems cool |
|
Thanks! No worries, totally understand. Fortune to have Abhro and Mosè's eyes on this too. Will go ahead and merge |
Following the lead here: tkemmer/ImplicitArrays.jl#11
Can maybe revisit after julia-vscode/TestItemRunner.jl#113 is resolved