feat(test-cases): output slots, stage decomposition, and CLI package split#748
Merged
Conversation
Materialise every run/replay/mint/build into a gitignored output slot (`<case>/output/<label>/`) holding the curated native set, the rebuilt committed bundle, and a `.source.json` stamp, so a developer can see and diff what a run produced. Refactor run/replay/mint onto a shared stage toolkit (execute / materialise / rebuild / build / upload / compare) and add: - a `build` verb that rebuilds the committed bundle from a slot without re-executing - `mint --from-replay` to re-author the manifest from stored native instead of re-running the diagnostic - `--label` on run/replay/mint/build to keep named slots side by side - changed-digest upload skip and a native-staleness warning run/build write the slot on every invocation but only promote the rebuilt bundle to the tracked `regression/` baseline on `--force-regen` or when seeding; the native block stays mint-owned, and mint uploads before it promotes.
test_cases.py had grown past 1700 lines. Split it into a climate_ref.cli.test_cases package grouped by concern: discovery (fetch/list), run, baselines (replay/mint/build), store (sync/check-store) and ci_gate, over _catalog/_common/_stages helpers and an _app module holding the shared Typer app. Command implementations are carried over unchanged. The package re-exports the helpers the unit tests import directly and registers the command modules in a fixed order so `ref test-cases --help` keeps its verb ordering. _test_case_stages.py moves in as _stages.py; the test mock targets are updated to the new module paths.
|
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 monolithic ChangesCLI test-cases package refactor with output-slot pipeline
Regression manifest and float handling validation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Record the output-slots feature and the test_cases CLI package split.
![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 (3)
packages/climate-ref/src/climate_ref/cli/test_cases/baselines.py (1)
454-477: 💤 Low valueConsider extracting shared promotion logic.
This block is nearly identical to
run.pylines 212–235: both gate onforce_regen or not paths.regression.exists(), preserveprevious.native, and warn on staleness. Extracting a shared helper (e.g.,_maybe_promote_baseline(slot, paths, committed, previous, *, print_summary=False)) would reduce duplication, though the current repetition is manageable.packages/climate-ref/src/climate_ref/cli/test_cases/store.py (1)
88-94: ⚖️ Poor tradeoffConsider if store API offers direct cache-warming method.
The pattern of fetching to a temporary file that's immediately discarded is used to trigger cache population as a side effect. Whilst this works, it's not immediately obvious to future maintainers.
If the underlying store API (Pooch) exposes a direct
.ensure_cached(sha256)or similar method, prefer that for clarity. Otherwise, add a brief inline comment explaining that fetch populates the cache as a side effect.packages/climate-ref/tests/unit/cli/test_test_cases.py (1)
1764-1778: 💤 Low valueConsider documenting the path assumption for the gitignore test.
The
parents[5]assumes a specific directory depth from the test file to the repository root. If this test file is ever relocated, the path resolution will break silently (or fail to find the correct.gitignore).Consider adding a defensive assertion or comment:
def test_output_slot_pattern_is_gitignored(tmp_path): """The repo .gitignore shadows materialised output slots under any test-data tree.""" from git import Repo repo_gitignore = Path(__file__).resolve().parents[5] / ".gitignore" + # Sanity check: ensure we found the repo root (not some other directory) + assert repo_gitignore.exists(), f"Expected repo .gitignore at {repo_gitignore}" repo = Repo.init(tmp_path)This would provide a clearer failure message if the path assumption becomes invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 048a493f-e2f4-4d9e-a6fd-4390ab359071
📒 Files selected for processing (17)
.gitignorechangelog/748.feature.mdchangelog/748.trivial.mddocs/background/regression-baselines.mdpackages/climate-ref-core/src/climate_ref_core/testing.pypackages/climate-ref/src/climate_ref/cli/test_cases.pypackages/climate-ref/src/climate_ref/cli/test_cases/__init__.pypackages/climate-ref/src/climate_ref/cli/test_cases/_app.pypackages/climate-ref/src/climate_ref/cli/test_cases/_catalog.pypackages/climate-ref/src/climate_ref/cli/test_cases/_common.pypackages/climate-ref/src/climate_ref/cli/test_cases/_stages.pypackages/climate-ref/src/climate_ref/cli/test_cases/baselines.pypackages/climate-ref/src/climate_ref/cli/test_cases/ci_gate.pypackages/climate-ref/src/climate_ref/cli/test_cases/discovery.pypackages/climate-ref/src/climate_ref/cli/test_cases/run.pypackages/climate-ref/src/climate_ref/cli/test_cases/store.pypackages/climate-ref/tests/unit/cli/test_test_cases.py
💤 Files with no reviewable changes (1)
- packages/climate-ref/src/climate_ref/cli/test_cases.py
…oring Two correctness fixes in the regression replay/mint machinery surfaced by an adversarial review: - stage_compare now drives the committed-bundle comparison from the manifest's committed set and hard-fails when an expected file is missing from the tracked baseline or from the regenerated bundle (or when there is nothing to compare). It previously derived the set from whatever existed under regression/, so a missing baseline let a replay report success without verifying anything. - mint --from-replay now preserves the previously minted native block verbatim instead of snapshotting the slot. stage_materialise hydrates the slot's native in place while rebuilding, so a fresh snapshot would re-author the manifest with non-portable, slot-specific blobs even though the canonical native baseline is unchanged. Adds stage_compare unit tests and a from-replay native-preservation test.
- docs: clarify run seeds manifest.native={} only on first seed
- catalog: sort parent dirs for deterministic baseline selection
- catalog: fail fast on adapter parse errors instead of a partial catalog
- stages: collect per-blob upload errors instead of aborting the mint loop
- discovery: extract duplicated provider validation to a shared helper
stage_rebuild_from_slot imported load_datasets_from_yaml at module level, while the run/mint verbs import it locally inside the function. A single climate_ref_core.testing.load_datasets_from_yaml mock therefore did not reach the replay rebuild path, which needed a second use-site patch. Import it locally in _stages too (TestCasePaths, annotation-only, moves under TYPE_CHECKING, so the module no longer pulls climate_ref_core.testing into CLI startup), so one mock target now covers every call site. Also tidies the round-trip integration test docstrings/section comments and corrects the corrupted-bundle test docstring: the advisory integrity check does not gate replay; the tolerant comparison does.
replay/mint/build/sync/ci-gate accepted --provider without checking it is a configured provider, so a typo iterated zero cases and exited 0 — for ci-gate that was a silent CI bypass that verified nothing. Route all five verbs through the shared _validate_provider_in_registry helper and replace run's hand-rolled lookup with it, so an unknown provider fails with exit 1 everywhere. A real provider with zero matching cases still exits 0.
Manifest.loads accepted any schema value verbatim, despite the documented contract that the loader rejects manifests whose schema does not match SCHEMA_VERSION — so a manifest from a future/incompatible writer would be silently misread under today's field semantics by the coupling gate. It also took NativeEntry.size straight from JSON, letting a negative, float, or string size load cleanly and break the byte-identical-dump guarantee. Enforce schema == SCHEMA_VERSION and a non-negative int size in loads.
The docstrings claimed output.json's floats are rounded, but it was omitted from the rounding map, and the rounded-file set was a drift-prone hand-maintained subset of COMMITTED_BUNDLE_FILES. output.json carries no float leaves and is Pydantic-serialised (no sort_keys), so rounding it via json.dumps(sort_keys=True) would reorder its keys and churn the committed bytes. Make the rounded/not-rounded classification explicit and exhaustive (killing the drift), correct the docstrings, and warn at runtime if an unrounded committed file ever gains a float. No committed bytes change.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 781f662a-1892-4889-b15c-9f0b2461d9f9
📒 Files selected for processing (10)
packages/climate-ref-core/src/climate_ref_core/regression/_quantise.pypackages/climate-ref-core/src/climate_ref_core/regression/capture.pypackages/climate-ref-core/src/climate_ref_core/regression/manifest.pypackages/climate-ref-core/tests/unit/regression/test_capture.pypackages/climate-ref-core/tests/unit/regression/test_manifest.pypackages/climate-ref/src/climate_ref/cli/test_cases/baselines.pypackages/climate-ref/src/climate_ref/cli/test_cases/ci_gate.pypackages/climate-ref/src/climate_ref/cli/test_cases/run.pypackages/climate-ref/src/climate_ref/cli/test_cases/store.pypackages/climate-ref/tests/unit/cli/test_test_cases.py
✅ Files skipped from review due to trivial changes (1)
- packages/climate-ref-core/src/climate_ref_core/regression/_quantise.py
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/climate-ref/src/climate_ref/cli/test_cases/store.py
- packages/climate-ref/src/climate_ref/cli/test_cases/run.py
- packages/climate-ref/src/climate_ref/cli/test_cases/ci_gate.py
- packages/climate-ref/src/climate_ref/cli/test_cases/baselines.py
- packages/climate-ref/tests/unit/cli/test_test_cases.py
The schema guard used value equality only, so True (a bool subclass of int) and 1.0 (1.0 == 1) were accepted as valid when SCHEMA_VERSION == 1. Require schema to be a non-bool int equal to SCHEMA_VERSION, matching the NativeEntry.size validation in the same loader, and extend the rejection test to cover True and 1.0.
On CI ($GITHUB_ACTIONS) Typer forces its help console into terminal mode and emits ANSI. NO_COLOR strips colour but not the bold its highlighter wraps around <metavars>, which split tokens like `output/<label>` and broke the substring assertion in test_run_help (green locally, red on CI). Set TERM=dumb so Rich emits no escape codes at all.
This was referenced Jun 25, 2026
Closed
3 tasks
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
1 participant
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.
Description
This branch makes two changes to the
ref test-casesdeveloper workflow.Output slots + stage decomposition
Every
run/replay/mint/buildnow materialises into a gitignored output slot (<case>/output/<label>/) holding the curated native set, the rebuilt committed bundle, and a.source.jsonstamp, so a developer can see and diff exactly what a run produced. The verbs are refactored onto a shared stage toolkit (execute / materialise / rebuild / build / upload / compare), adding:buildverb that rebuilds the committed bundle from a slot without re-executingmint --from-replayto re-author the manifest from stored native instead of re-running the diagnostic--labelon run/replay/mint/build to keep named slots side by siderun/buildwrite the slot on every invocation but only promote the rebuilt bundle to the trackedregression/baseline on--force-regenor when seeding; the native block stays mint-owned, andmintuploads before it promotes.CLI subpackage split
test_cases.pyhad grown past 1700 lines, so it is decomposed into aclimate_ref.cli.test_casespackage grouped by concern —discovery(fetch/list),run,baselines(replay/mint/build),store(sync/check-store) andci_gate, over_catalog/_common/_stageshelpers and an_appmodule holding the shared Typer app. Command implementations are carried over unchanged; the package re-exports the helpers the unit tests import directly and registers the command modules in a fixed order soref test-cases --helpkeeps its verb ordering. No behaviour change.Checklist
Please confirm that this pull request has done the following:
changelog/Summary by CodeRabbit
Release Notes
New Features
ref test-casesruns now write results to dedicated, gitignoredoutput/<label>/directories (including curated native outputs, regenerated bundles, and a source stamp) to enable side-by-side inspection.ref test-casesverbs:run,replay,mint,build, plus--labelfor concurrent comparisons.Documentation
Bug Fixes
Chores
.gitignoreto exclude materialised output-slot test data.