refactor(test-cases): consolidate diagnostic execution and drift-check integration tests#789
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR removes ChangesRegression validation replacement
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Factory as create_no_drift_test
participant Assert as assert_test_case_no_drift
participant Diagnostic
participant Bundle as Committed Bundle
Factory-->>Test: test_run_test_cases (parameterised)
Test->>Assert: assert_test_case_no_drift(config, diagnostic, test_case_name, paths, tmp_path)
Assert->>Diagnostic: run(definition, capture_regression=True)
Diagnostic-->>Assert: ExecutionResult
Assert->>Bundle: rebuild committed bundle from outputs
Assert->>Bundle: compare against regression baseline
Assert-->>Test: assertion pass/fail on drift
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 23 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/step_test_case.py (1)
100-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated catalog-existence check.
The catalog-existence guard is duplicated verbatim (aside from the function name in the error message) between
_run_executeand_run_replay. Could be extracted into a small helper, though the benefit is limited given this is a debugging script.Also applies to: 125-129
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 724a730e-132e-4a26-b3a6-62713e604f76
📒 Files selected for processing (13)
changelog/789.trivial.mddocs/api-surface.mdpackages/climate-ref-core/src/climate_ref_core/diagnostics.pypackages/climate-ref-core/src/climate_ref_core/testing.pypackages/climate-ref-core/tests/unit/test_testing.pypackages/climate-ref-esmvaltool/tests/integration/test_diagnostics.pypackages/climate-ref-example/tests/integration/test_diagnostics.pypackages/climate-ref-ilamb/tests/integration/test_diagnostics.pypackages/climate-ref-pmp/tests/integration/test_diagnostics.pypackages/climate-ref/src/climate_ref/conftest_plugin.pypackages/climate-ref/src/climate_ref/testing.pypackages/climate-ref/tests/unit/test_testing.pyscripts/step_test_case.py
💤 Files with no reviewable changes (2)
- docs/api-surface.md
- packages/climate-ref-core/tests/unit/test_testing.py
…k integration tests Collapse the diagnostic execution paths down to a single execute+build sequence and make the provider integration tests assert the committed regression bundle has not drifted. - Add a `capture_regression` hook to `Diagnostic.run` so the regression runner reuses the production execute+build sequence instead of mirroring it; `TestCaseRunner.run` now delegates to it. - Remove the parked offline `test_validate_test_case_regression` tests and the now-unused `RegressionValidator` (superseded by the store-backed `ref test-cases replay` baselines). - Replace `validate_result` with a shared `assert_test_case_no_drift` helper: `test_run_test_cases` re-executes each case and compares the rebuilt committed bundle to the tracked baseline within tolerance. Bundle ingestion is covered by the executor result-handling tests and is documented as out of scope here.
lewisjared
force-pushed
the
refactor/test-case-execution-consolidation
branch
from
990318f to
b3ad98d
Compare
…oss providers The four provider integration test modules were byte-identical apart from the provider import. Each now wires its suite in one line via climate_ref.testing.create_no_drift_test, which builds the parameterized drift test and resolves test-case paths with TestCasePaths.from_diagnostic, the same resolution the CLI uses. Also remove the unused run_test_case pytest fixture; custom tests should use TestCaseRunner directly.
Replace the removed run_test_case fixture section with the current setup: the generated per-provider drift test, custom tests via TestCaseRunner, and a new troubleshooting section on debugging a failing test case (output slots, baseline diffing, replay vs run, pytest --pdb).
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: 1844c8f6-db39-41a2-ae93-a85fd5761e29
📒 Files selected for processing (13)
changelog/789.trivial.mddocs/api-surface.mddocs/how-to-guides/testing-diagnostics.mdpackages/climate-ref-core/src/climate_ref_core/diagnostics.pypackages/climate-ref-core/src/climate_ref_core/testing.pypackages/climate-ref-core/tests/unit/test_testing.pypackages/climate-ref-esmvaltool/tests/integration/test_diagnostics.pypackages/climate-ref-example/tests/integration/test_diagnostics.pypackages/climate-ref-ilamb/tests/integration/test_diagnostics.pypackages/climate-ref-pmp/tests/integration/test_diagnostics.pypackages/climate-ref/src/climate_ref/conftest_plugin.pypackages/climate-ref/src/climate_ref/testing.pypackages/climate-ref/tests/unit/test_testing.py
💤 Files with no reviewable changes (2)
- docs/api-surface.md
- packages/climate-ref-core/tests/unit/test_testing.py
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/climate-ref-core/src/climate_ref_core/diagnostics.py
- packages/climate-ref-core/src/climate_ref_core/testing.py
assert_test_case_no_drift called get_case directly after checking only for a missing test_data_spec, so an unknown test case name surfaced a bare StopIteration instead of the friendlier TestCaseNotFoundError that TestCaseRunner.run already raises for the same situation. Add the matching has_case guard and document the raised exceptions.
1 participant
Description
Slims the diagnostic test-case machinery down to a single execution path and turns the provider integration tests into genuine drift checks.
Previously the same execute+build sequence was implemented three ways (
Diagnostic.run, theTestCaseRunnermirror, and a bespokevalidate_result), andtest_run_test_casesonly asserted a run succeeded — it never checked the committed bundle for drift. There was also a parked, always-skipped offline validator (RegressionValidator/test_validate_test_case_regression) superseded by the store-backedref test-cases replaybaselines.Changes:
capture_regressionhook toDiagnostic.run(keyword-only, defaults toFalse, so production execution is unchanged).TestCaseRunner.runnow delegates to it instead of hand-mirroring execute+build, leaving one execute+build sequence in the codebase.test_validate_test_case_regressiontests across all four providers and delete the now-unusedRegressionValidator(and its unit tests).validate_resultwith a sharedassert_test_case_no_drifthelper.test_run_test_casesnow re-executes each test case using the samestage_execute/stage_build/stage_comparestages asref test-cases run, rebuilds the committed bundle, and compares it to the trackedregression/baseline within tolerance. This asserts both that the diagnostic still executes and that its committed bundle has not drifted — via re-execution, not replay.scripts/step_test_case.py, a debugging aid to step through the execute or replay of a single test case using the same test-cases CLI stages.Net: +153 / −552 across the test/execution layers.
Checklist
Please confirm that this pull request has done the following:
changelog/Summary by CodeRabbit
New Features
Bug Fixes
Documentation