Introducing regression tests#1345
Open
kholoimov wants to merge 2 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a Python-driven FairShip regression framework integrated with CTest and Pixi, defines simulation and reconstruction cases with captured references, documents test workflows, adds deterministic generator seeding, and clarifies existing C++ test terminology. ChangesFairShip regression testing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CTest
participant harness.py
participant FairShipScript
participant ReferenceOutput
CTest->>harness.py: run configured regression test
harness.py->>FairShipScript: execute with FAIRSHIP_ROOT
FairShipScript-->>harness.py: capture combined output
harness.py->>ReferenceOutput: compare filtered output
ReferenceOutput-->>CTest: report pass or failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/fairship_tests/harness.py (1)
35-112: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
test_cases()to avoid repeated re-parsing.
test_cases()re-reads and fully re-validates (including cycle detection)test_cases.yamlon every call. It's invoked repeatedly acrosstest_names(),_test_case(),_patterns_for(),dependency_groups(), and once per test insideregenerate_test()'s recursive loop. This is fine at 8 tests but scales poorly and does redundant work.♻️ Suggested fix
+from functools import lru_cache + +@lru_cache(maxsize=1) def test_cases() -> list[TestCase]: """Return and validate the tests declared by the configuration file."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fairship_tests/harness.py` around lines 35 - 112, Cache the validated result of test_cases() so the YAML is parsed and dependency validation, including validate_acyclic(), occur only once per process. Preserve the existing returned list and all validation behavior, and ensure callers such as test_names(), _test_case(), _patterns_for(), dependency_groups(), and regenerate_test() reuse the cached result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/fairship_tests/harness.py`:
- Around line 180-196: Update the subprocess.run invocation in run to enforce a
finite timeout for configured test scripts, including muonback-io.sh and
muonback-summary.sh. Handle subprocess.TimeoutExpired by returning an
appropriate failing status and captured diagnostic output, while preserving
normal return-code and output normalization behavior for completed commands.
- Around line 81-91: Update test_cases() validation to require every declared
dependency to appear earlier in the cases list than its dependent. Track
previously seen test names while iterating and reject any dependency not yet
seen, while preserving the existing duplicate, unknown-dependency, and
self-dependency checks.
In `@tests/fairship_tests/references/pythia8-summary.txt`:
- Line 1: Update skip_patterns.conf to mask all environment-specific
absolute-path output, including the listed Media file used, ---User path,
G4ParticleHPInelasticData, NeutronHP, field-creation, and Target configuration
patterns. Then rerun regenerate_references.sh so
tests/fairship_tests/references/pythia8-summary.txt (1-1) and
tests/fairship_tests/references/tracking-benchmark.txt (1-1) no longer contain
unfiltered local paths.
In `@tests/fairship_tests/scripts/reconstruction.sh`:
- Around line 5-7: Set the QT_QPA_PLATFORM environment variable to offscreen in
reconstruction.sh before invoking ShipReco.py, matching the setup used by the
other FairShip test scripts and preserving the existing command flow.
In `@tests/fairship_tests/skip_patterns.conf`:
- Around line 32-51: Update tests/fairship_tests/skip_patterns.conf lines 32-51
by consolidating the muonback-io and reconstruction path filters into the global
test * block and adding filters for DecayConfig, EvtGen, and geometry-file
paths. Then run pixi run regenerate-fairship-references to refresh
tests/fairship_tests/references/ci-sim.txt lines 4-5, evtgen-summary.txt line 1,
muonback-io.txt line 180, muonback-summary.txt line 94, and particle-gun.txt
line 1 so leaked absolute local paths are removed.
In `@tests/README.md`:
- Around line 81-87: Correct both YAML examples in tests/README.md (lines 81-87)
and tests/fairship_tests/README.md (lines 24-30) by defining the dependency-free
simulation test as a mapping with a name field instead of a plain string; leave
harness.py unchanged.
---
Nitpick comments:
In `@tests/fairship_tests/harness.py`:
- Around line 35-112: Cache the validated result of test_cases() so the YAML is
parsed and dependency validation, including validate_acyclic(), occur only once
per process. Preserve the existing returned list and all validation behavior,
and ensure callers such as test_names(), _test_case(), _patterns_for(),
dependency_groups(), and regenerate_test() reuse the cached result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 821676a9-a5e5-4962-a889-8d3206d19131
📒 Files selected for processing (28)
README.mdpixi.tomltests/CMakeLists.txttests/README.mdtests/fairship_tests/README.mdtests/fairship_tests/__init__.pytests/fairship_tests/harness.pytests/fairship_tests/references/ci-sim.txttests/fairship_tests/references/evtgen-summary.txttests/fairship_tests/references/muonback-io.txttests/fairship_tests/references/muonback-summary.txttests/fairship_tests/references/particle-gun.txttests/fairship_tests/references/pythia8-summary.txttests/fairship_tests/references/reconstruction.txttests/fairship_tests/references/tracking-benchmark.txttests/fairship_tests/regenerate_references.shtests/fairship_tests/scripts/ci-sim.shtests/fairship_tests/scripts/evtgen-summary.shtests/fairship_tests/scripts/muonback-io.shtests/fairship_tests/scripts/muonback-summary.shtests/fairship_tests/scripts/particle-gun.shtests/fairship_tests/scripts/pythia8-summary.shtests/fairship_tests/scripts/reconstruction.shtests/fairship_tests/scripts/tracking-benchmark.shtests/fairship_tests/skip_patterns.conftests/fairship_tests/test_cases.yamltests/test_data_class_io.cxxtests/test_rntuple_io.cxx
Contributor
Author
|
Going to apply coderabbit comments |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
|
@kholoimov Could you please get the CI to pass? I think most jobs fail because the lockfile needs updating due to the added env. |
Contributor
|
No merge commit please. |
Contributor
Author
|
It's empty. Have no idea why github decided that I need to update my branch |
kholoimov
force-pushed
the
dev_vkholoim_ctest
branch
from
July 21, 2026 08:13
16c0e3c to
b7c258a
Compare
Contributor
Author
|
@olantwin I believe now it's failing not because of my changes. I can fix it in the PR, but probably will be better to have it separately: |
Contributor
This is indeed my fault due to geant4-vmc moving to conda-forge. |
Contributor
|
#1348 should fix the geant4-vmc issue. |
Contributor
|
Rebasing now should work. |
kholoimov
force-pushed
the
dev_vkholoim_ctest
branch
from
July 21, 2026 09:29
97dc9ad to
c9ca589
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/fairship_tests/harness.py (1)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse iterable unpacking for list concatenation.
You can use iterable unpacking instead of list concatenation for slightly better performance and more idiomatic Python, as recommended by static analysis tools (e.g., Ruff's
RUF005).♻️ Proposed refactor
if name in visiting: - cycle = visiting[visiting.index(name) :] + [name] + cycle = [*visiting[visiting.index(name) :], name] raise RuntimeError(f"Dependency cycle in {TEST_CASES}: {' -> '.join(cycle)}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fairship_tests/harness.py` around lines 108 - 110, Update the cycle construction in the dependency traversal around the visiting list to use iterable unpacking instead of list concatenation, while preserving the same ordering and repeated cycle-name behavior in the RuntimeError raised by the cycle detection logic.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/fairship_tests/harness.py`:
- Around line 108-110: Update the cycle construction in the dependency traversal
around the visiting list to use iterable unpacking instead of list
concatenation, while preserving the same ordering and repeated cycle-name
behavior in the RuntimeError raised by the cycle detection logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24e7c89f-7185-448f-941e-ca2f831917c5
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
README.mdpixi.tomltests/CMakeLists.txttests/README.mdtests/fairship_tests/README.mdtests/fairship_tests/__init__.pytests/fairship_tests/harness.pytests/fairship_tests/references/ci-sim.txttests/fairship_tests/references/evtgen-summary.txttests/fairship_tests/references/muonback-io.txttests/fairship_tests/references/muonback-summary.txttests/fairship_tests/references/particle-gun.txttests/fairship_tests/references/pythia8-summary.txttests/fairship_tests/references/reconstruction.txttests/fairship_tests/references/tracking-benchmark.txttests/fairship_tests/regenerate_references.shtests/fairship_tests/scripts/ci-sim.shtests/fairship_tests/scripts/evtgen-summary.shtests/fairship_tests/scripts/muonback-io.shtests/fairship_tests/scripts/muonback-summary.shtests/fairship_tests/scripts/particle-gun.shtests/fairship_tests/scripts/pythia8-summary.shtests/fairship_tests/scripts/reconstruction.shtests/fairship_tests/scripts/tracking-benchmark.shtests/fairship_tests/skip_patterns.conftests/fairship_tests/test_cases.yamltests/test_data_class_io.cxxtests/test_rntuple_io.cxx
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/fairship_tests/regenerate_references.sh
- tests/fairship_tests/skip_patterns.conf
- tests/fairship_tests/test_cases.yaml
- tests/fairship_tests/scripts/ci-sim.sh
- tests/fairship_tests/scripts/evtgen-summary.sh
- tests/fairship_tests/scripts/muonback-io.sh
- tests/fairship_tests/init.py
- tests/fairship_tests/scripts/muonback-summary.sh
- tests/fairship_tests/scripts/reconstruction.sh
- tests/test_rntuple_io.cxx
- tests/fairship_tests/scripts/pythia8-summary.sh
- tests/fairship_tests/references/reconstruction.txt
- tests/fairship_tests/scripts/tracking-benchmark.sh
- README.md
- tests/fairship_tests/scripts/particle-gun.sh
- tests/CMakeLists.txt
- tests/test_data_class_io.cxx
- tests/README.md
- tests/fairship_tests/references/ci-sim.txt
- tests/fairship_tests/references/evtgen-summary.txt
kholoimov
force-pushed
the
dev_vkholoim_ctest
branch
3 times, most recently
from
July 21, 2026 11:31
cc607b2 to
836071a
Compare
Add reusable regression-test harnesses, reference outputs, CTest integration, and documentation. Isolate lint tooling and preserve generated reference files during formatting checks.
kholoimov
force-pushed
the
dev_vkholoim_ctest
branch
from
July 21, 2026 11:37
836071a to
ac4afce
Compare
Contributor
Author
|
I faced the problem that even with Seed propagation inside Pythia8 and EvtGen generators, I see different output on different machines, while the output on the same machine stays the same. |
Contributor
Author
Looks like the issue is that my second machine utilises an AMD processor. Going to validate tests on lxplus, and if everything goes well, this is ready to be merged |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/fairship_tests/harness.py (2)
59-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the validation error message.
The current validation logic strictly requires the test entry to be a dictionary (mapping), so a bare string "test name" will trigger this error. Updating the message to reflect that only a mapping is allowed will prevent configuration confusion.
♻️ Proposed refactor
else: raise RuntimeError( - f"{location} must be a test name or a mapping containing 'name' and optional 'depends_on'" + f"{location} must be a mapping containing 'name' and optional 'depends_on'" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fairship_tests/harness.py` around lines 59 - 61, Update the validation error raised in the harness entry-validation logic to state that location must be a mapping containing “name” and optional “depends_on”; remove the misleading allowance for a bare test-name string. Preserve the existing validation behavior and RuntimeError handling.
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider unpacking instead of list concatenation.
As indicated by static analysis, using list unpacking is slightly more idiomatic and performant for constructing the cycle path than list concatenation.
♻️ Proposed refactor
if name in visiting: - cycle = visiting[visiting.index(name) :] + [name] + cycle = [*visiting[visiting.index(name) :], name] raise RuntimeError(f"Dependency cycle in {TEST_CASES}: {' -> '.join(cycle)}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fairship_tests/harness.py` around lines 108 - 110, Update the cycle construction in the dependency traversal around the visiting list to use list unpacking when appending name, while preserving the existing cycle path and RuntimeError message behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/fairship_tests/harness.py`:
- Around line 59-61: Update the validation error raised in the harness
entry-validation logic to state that location must be a mapping containing
“name” and optional “depends_on”; remove the misleading allowance for a bare
test-name string. Preserve the existing validation behavior and RuntimeError
handling.
- Around line 108-110: Update the cycle construction in the dependency traversal
around the visiting list to use list unpacking when appending name, while
preserving the existing cycle path and RuntimeError message behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccdef4f1-86db-4c6e-a7e0-8016a306d292
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.pre-commit-config.yamlREADME.mdgconfig/DecayConfigTEvtGen.Cmacro/run_simScript.pypixi.tomlshipgen/DPPythia8Generator.cxxshipgen/DPPythia8Generator.hshipgen/FixedTargetGenerator.cxxshipgen/HNLPythia8Generator.cxxshipgen/HNLPythia8Generator.hshipgen/Pythia8Generator.cxxshipgen/Pythia8Generator.hshipgen/TEvtGenDecayer.cxxshipgen/TEvtGenDecayer.hstubs/ROOT/__init__.pyitests/CMakeLists.txttests/README.mdtests/fairship_tests/README.mdtests/fairship_tests/__init__.pytests/fairship_tests/harness.pytests/fairship_tests/references/ci-sim.txttests/fairship_tests/references/evtgen-summary.txttests/fairship_tests/references/muonback-io.txttests/fairship_tests/references/muonback-summary.txttests/fairship_tests/references/particle-gun.txttests/fairship_tests/references/pythia8-summary.txttests/fairship_tests/references/reconstruction.txttests/fairship_tests/references/tracking-benchmark.txttests/fairship_tests/regenerate_references.shtests/fairship_tests/scripts/ci-sim.shtests/fairship_tests/scripts/evtgen-summary.shtests/fairship_tests/scripts/muonback-io.shtests/fairship_tests/scripts/muonback-summary.shtests/fairship_tests/scripts/particle-gun.shtests/fairship_tests/scripts/pythia8-summary.shtests/fairship_tests/scripts/reconstruction.shtests/fairship_tests/scripts/tracking-benchmark.shtests/fairship_tests/skip_patterns.conftests/fairship_tests/test_cases.yamltests/test_data_class_io.cxxtests/test_rntuple_io.cxx
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/fairship_tests/scripts/pythia8-summary.sh
- tests/fairship_tests/scripts/reconstruction.sh
- tests/fairship_tests/regenerate_references.sh
- tests/fairship_tests/test_cases.yaml
- tests/fairship_tests/scripts/muonback-summary.sh
- tests/fairship_tests/scripts/muonback-io.sh
- tests/fairship_tests/scripts/particle-gun.sh
- README.md
- tests/README.md
- tests/fairship_tests/scripts/ci-sim.sh
- tests/fairship_tests/init.py
- tests/fairship_tests/scripts/evtgen-summary.sh
- tests/fairship_tests/scripts/tracking-benchmark.sh
- tests/fairship_tests/skip_patterns.conf
- tests/fairship_tests/references/reconstruction.txt
- tests/CMakeLists.txt
- tests/test_rntuple_io.cxx
- tests/test_data_class_io.cxx
- pixi.toml
- tests/fairship_tests/references/evtgen-summary.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a CTest-based regression testing framework for FairShip.
Main features:
Run the tests with:
pixi run test-fairshipRegenerate references with:
pixi run regenerate-fairship-referencesSummary by CodeRabbit