test: fail fast under --no-build when a needed binary is missing; add --no-chemistry - #1934
Conversation
… --no-chemistry Under --no-build, build() is a no-op, so a case whose build variant was never compiled failed only when it ran. Chemistry is the usual one: ./mfc.sh build does not produce the chemistry variant, so a full suite ran for its whole length before the chemistry cases failed at the end. Before running any case, check that every (target, slug) the selected cases need has an installed binary. If any are missing, list each with an example case and the expected path, and exit non-zero. When every affected case uses chemistry, suggest --no-chemistry. --no-chemistry skips cases with chemistry = 'T'. It keys on the parameter, not the trace, because Example cases built on reacting examples need a chemistry build but carry no "Chemistry" trace element.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Adds a --no-build preflight check to fail fast when required binaries are missing, and introduces --no-chemistry to skip chemistry-enabled test cases (useful when chemistry build variants aren’t installed).
Changes:
- Add
--no-buildpreflight to detect missing(target, slug)binaries before running any tests and emit a consolidated error message. - Add
--no-chemistryCLI flag and filtering logic to skip cases withchemistry = T. - Add docs and unit tests covering preflight grouping/counting and chemistry filtering behavior.
| File | Description |
|---|---|
| toolchain/mfc/test/test_nobuild_preflight.py | New unit tests for missing-binary preflight and chemistry skipping. |
| toolchain/mfc/test/test.py | Implements chemistry detection/filtering, missing-binary discovery, and the --no-build preflight in test(). |
| toolchain/mfc/cli/commands.py | Adds the new --no-chemistry flag. |
| docs/documentation/testing.md | Documents --no-chemistry and the new --no-build preflight behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| continue | ||
| checked.add(key) | ||
| binpath = code.get_install_binpath(input_file) | ||
| if not os.path.isfile(binpath): |
There was a problem hiding this comment.
Fixed in c0de563: the preflight now requires os.path.isfile(binpath) and os.access(binpath, os.X_OK), with a test that a non-executable file is reported as missing.
| @@ -368,6 +416,8 @@ def test(): | |||
|
|
|||
| cases, skipped_cases = __filter(cases) | |||
| cases = [_.to_case() for _ in cases] | |||
There was a problem hiding this comment.
Fixed in c0de563. The filter can't move before to_case(): Example cases only know their chemistry value after their case.py is loaded. So _drop_chemistry_cases now takes the builders alongside the converted cases and appends the skipped builders. skipped_cases stays a list of builders, as __filter returns it.
| lines = [f"--no-build was given, but {len(unbuilt)} build(s) needed by {n_cases} test case(s) are missing:"] | ||
| for entry in unbuilt: | ||
| cases = entry["cases"] | ||
| more = f" (+{len(cases) - 1} more)" if len(cases) > 1 else "" | ||
| lines.append(f" {entry['target']} [{entry['slug']}]: {len(cases)} case(s), e.g. {cases[0].trace} ({cases[0].get_uuid()}){more}") | ||
| lines.append(f" expected {entry['binpath']}") |
There was a problem hiding this comment.
Leaving this as is: the output is already deterministic, because it follows list_cases() order, which is fixed. Listing missing builds in suite order also matches the order the cases would have run.
Address review: the --no-build preflight now also requires the binary to be executable, and --no-chemistry appends the skipped cases' builders rather than their converted TestCases, so skipped_cases holds one type as __filter leaves it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1934 +/- ##
=======================================
Coverage 62.82% 62.82%
=======================================
Files 86 86
Lines 22394 22394
Branches 3305 3305
=======================================
Hits 14070 14070
Misses 6071 6071
Partials 2253 2253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
./mfc.sh buildfollowed by./mfc.sh test --no-buildruns the full suite and fails every chemistry case at the end. Under--no-build,build()does nothing, and./mfc.sh buildnever produces the chemistry build variant (one*-chem-<hash>slug per mechanism). The same applies to any case whose slug differs from the plain build (e.g. analytic initial conditions inpre_process), or when build and test flags disagree.Change
--no-buildpreflight: before running anything, check that every(target, slug)the selected cases need has an installed binary. If not, list each missing build with an example case and the expected path, then exit non-zero. Behavior without--no-buildis unchanged.--no-chemistry: skip cases withchemistry = 'T'. Filters on the parameter rather than the trace, so reactingExamplecases are covered too. Suggested by the preflight error when every missing build is a chemistry one.testing.md; unit tests intoolchain/mfc/test/test_nobuild_preflight.py.Testing
./mfc.sh build, thentest --no-build --only 1D: fails immediately with the error above.test --no-build --only 1D --no-chemistry: 175 passed, 0 failed../mfc.sh precheckpasses.CI already builds every slug with
test --dry-runbeforetest --no-build, so the preflight passes there.Acknowledgement