Replace SafeTestSets with TestItemRunner#57
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the test harness from SafeTestsets to TestItemRunner, reorganizing test suites into @testitem blocks and extracting shared helper code into a new test/utils.jl.
Changes:
- Replaced
SafeTestsets-basedruntests.jlexecution withTestItemRunner.@run_package_testsplus GROUP-based filtering. - Converted test files from
@testset-driven includes to@testitemstructure. - Added
test/utils.jlto share common helper functions used across test items.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/runtests.jl |
Switches test entrypoint to TestItemRunner with GROUP-based filters. |
test/utils.jl |
New shared test helpers for interpolation + Symbolics-related tests. |
test/test_interpolations.jl |
Converted interpolation tests to @testitem and now includes shared helpers. |
test/test_derivatives.jl |
Converted derivative tests to a @testitem suite. |
test/test_datainterpolations_comparison.jl |
Converted comparison tests to @testitem. |
test/test_symbolics_ext.jl |
Converted Symbolics extension tests to @testitem, now using shared helpers. |
test/test_interface.jl |
Wraps interface test groups in @testitems and normalizes keyword formatting. |
Project.toml |
Removes SafeTestsets and adds TestItemRunner to test dependencies/compat. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test_globally_constant( | ||
| BSplineInterpolationDimension; args1 = (3,), args2 = (1,), | ||
| kwargs1 = (:max_derivative_order_eval => 1,), | ||
| kwargs2 = (:max_derivative_order_eval => 1,), | ||
| cache = NURBSWeights(rand(7, 5)), | ||
| test_derivatives = false | ||
| BSplineInterpolationDimension; args1=(3,), args2=(1,), | ||
| kwargs1=(:max_derivative_order_eval => 1,), | ||
| kwargs2=(:max_derivative_order_eval => 1,), | ||
| cache=NURBSWeights(rand(7, 5)), | ||
| test_derivatives=false |
There was a problem hiding this comment.
This test now uses rand(7, 5) to construct NURBSWeights without setting a seed, which makes the test nondeterministic and can complicate reproducing failures. Consider seeding RNG inside this test item (or using fixed weights) so the inputs are stable run-to-run.
| using Random | ||
| using ForwardDiff | ||
| using DataInterpolationsND: AbstractInterpolationDimension, EmptyCache | ||
| using Symbolics | ||
| import SymbolicUtils as SU | ||
| using Symbolics: unwrap |
There was a problem hiding this comment.
utils.jl is included by core interpolation tests, but it unconditionally loads Symbolics/SymbolicUtils and ForwardDiff. This increases test startup/compilation time for core tests and couples unrelated test dependencies. Consider splitting this file (e.g., interpolation utilities vs. Symbolics utilities) or moving the Symbolics-related imports/helpers into a Symbolics-only helper that is only included from test_symbolics_ext.jl.
| @@ -0,0 +1,90 @@ | |||
| using Random | |||
| using ForwardDiff | |||
There was a problem hiding this comment.
ForwardDiff is imported here but not used anywhere in this helper file. Dropping the unused import will reduce unnecessary loading/compilation during tests (especially since utils.jl is included in multiple test items).
| using ForwardDiff |
| elseif GROUP == "QA" | ||
| activate_qa_env() | ||
| include("qa/runtests.jl") | ||
| @run_package_tests filter = ti -> endswith(ti.filename, "qa/runtests.jl") |
There was a problem hiding this comment.
In the GROUP == "QA" branch, @run_package_tests is filtered to items whose filename ends with qa/runtests.jl, but test/qa/runtests.jl is still written using SafeTestsets/@safetestset (i.e., it does not define any @testitems). This will result in the QA group running zero tests. Consider either keeping include("qa/runtests.jl") for this branch (after activating the QA env) or converting the QA suite to @testitems and filtering those instead.
| @run_package_tests filter = ti -> endswith(ti.filename, "qa/runtests.jl") | |
| include("qa/runtests.jl") |
SouthEndMusic
changed the title
SafeTestSets with TestItemRunner
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.