refactor: fold the test-cases verbs together and collapse the native store - #858
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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)
📝 WalkthroughWalkthroughThe regression workflow removes ChangesRegression storage
CLI lifecycle
Workflow and lifecycle documentation
Sequence Diagram(s)sequenceDiagram
participant RunTestCase
participant RebuildFromSlot
participant OutputSlot
participant BundleBuilder
RunTestCase->>RebuildFromSlot: Request --from-slot
RebuildFromSlot->>OutputSlot: Validate native files and catalog
RebuildFromSlot->>BundleBuilder: Rebuild bundle without diagnostic execution
BundleBuilder-->>RunTestCase: Return rebuilt bundle
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
52e034b to
bef0e99
Compare
Replaces the build verb with a --from-slot option on run, which reuses the native already in output/<label>/ instead of executing the diagnostic. Removes the sync verb. It fetched every manifest blob into a temporary directory purely for the store cache side effect, which replay already triggers when it materialises.
Replaces the NativeStore protocol and its three implementations with a single class. The store URL decides the backend: a file:// URL or bare path reads and writes locally, an http(s) URL reads anonymously through pooch, and the same URL with write credentials adds the S3 upload path used by mint. Public operations, error messages and the build_native_store factory are unchanged.
Aligns the --from-slot missing-catalog message with the one the driver gives, makes the pooch cache directory explicit rather than defaulting to the working directory, and refreshes the docs and tests that still described sync.
…rom-slot The guard arrived on the base branch as a build test. Retargets it at the option that replaced build.
7a524df to
f0a6103
Compare
- Rejects --from-slot alongside the options that only apply to an execution, so a rebuild can no longer be skipped silently by --only-missing. - Moves the S3 endpoint, bucket and credentials into an S3WriteConfig, so a store no longer carries five fields that mean nothing unless it writes. - Validates the digest at the point of use rather than behind a key builder. - Drops the exit_if_empty message argument, which lost its only caller with sync. - Restores build's wording for an empty slot and matches the two catalog errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/climate-ref/tests/integration/test_native_roundtrip.py (1)
773-782: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale assertion message.
The test is renamed to
test_missing_blob_detected_by_has, but the assertion at Line 790 still reads"Sync must detect the missing blob". Thesyncverb no longer exists. Change the message to namestore.has.
🧹 Nitpick comments (5)
packages/climate-ref-core/src/climate_ref_core/regression/store.py (1)
390-417: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
putwrites the blob non-atomically.
shutil.copy2writes directly to the canonical content-addressed path. If the process is interrupted, a truncated file remains at that path, and every laterhascall reports it as present.fetchdetects the corruption through_verify_hash_matches, so the failure is loud rather than silent, but a re-mint cannot repair the store becauseputskips the existing path.Copy to a temporary name in the same directory, then
os.replaceit into place.♻️ Proposed change
blob.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(str(path), str(blob)) + tmp = blob.with_name(f"{blob.name}.{os.getpid()}.tmp") + shutil.copy2(str(path), str(tmp)) + os.replace(str(tmp), str(blob))packages/climate-ref-core/tests/unit/regression/test_store.py (1)
20-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear the
_pooch_managercache between tests.
_pooch_manageris decorated withfunctools.cache, and it is keyed by(base_url, cache_dir)._patch_pooch_createreplacespooch.create, but a cached manager from an earlier test bypasses the patch. The current tests pass only because each one uses a distincttmp_pathcache directory. A future test that reuses a cache directory would silently get the real pooch manager and attempt a network call.Add
_pooch_manager.cache_clear()inside_patch_pooch_createto make the isolation explicit.packages/climate-ref/tests/unit/cli/test_test_cases.py (1)
2307-2369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
--from-slotwith a named--label.The new tests cover the default
latestslot, the option conflicts, the missing slot, the missing catalog, and a failed rebuild. No test proves--from-slotreads the slot named by--label. The label is the main input of this verb, and a wrong-slot regression would still pass the current suite.Seed a slot with
run --label before, then assertrun --from-slot --label beforerebuildsoutput/before/regression/.docs/background/regression-baselines.md (1)
322-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the drift wording with the table.
Line 261 states the job "
replays every baseline". Line 322 states it replays "every referenced native blob".replayoperates on a test case baseline; the native blobs are the inputs it materialises. Use the same term in both places.✏️ Proposed fix
-A scheduled (and manually dispatchable) job `replay`s every referenced native blob against the committed bundle. +A scheduled (and manually dispatchable) job `replay`s every baseline, materialising its referenced native blobs and comparing against the committed bundle.Line 323 also reads "a baseline that no longer reproduce"; correct it to "reproduces".
packages/climate-ref/tests/integration/test_native_roundtrip.py (1)
834-838: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe helper duplicates the store's blob layout.
_blob_pathreimplementsroot / digest[:2] / digest, whichNativeStore._blob_pathalready defines. If the layout changes, this helper diverges from the implementation it is meant to probe. Delegate to the store instead.♻️ Proposed change
def _blob_path(store: NativeStore, digest: str) -> Path: """Return the on-disk path of a blob in a local store, for tests that corrupt or delete one.""" root = store.root assert root is not None - return root / digest[:2] / digest + return store._blob_path(digest, root)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 67c27709-dbad-4d19-990d-844de48ddb1a
📒 Files selected for processing (17)
.github/workflows/regression-drift.yamlchangelog/858.breaking.mddocs/background/regression-baselines.mdpackages/climate-ref-core/src/climate_ref_core/regression/__init__.pypackages/climate-ref-core/src/climate_ref_core/regression/store.pypackages/climate-ref-core/tests/unit/regression/test_capture.pypackages/climate-ref-core/tests/unit/regression/test_store.pypackages/climate-ref/src/climate_ref/cli/__init__.pypackages/climate-ref/src/climate_ref/cli/test_cases/__init__.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/run.pypackages/climate-ref/src/climate_ref/cli/test_cases/store.pypackages/climate-ref/tests/integration/test_native_roundtrip.pypackages/climate-ref/tests/unit/cli/test_root.pypackages/climate-ref/tests/unit/cli/test_test_cases.py
💤 Files with no reviewable changes (4)
- packages/climate-ref/src/climate_ref/cli/init.py
- packages/climate-ref/src/climate_ref/cli/test_cases/store.py
- .github/workflows/regression-drift.yaml
- packages/climate-ref/tests/unit/cli/test_root.py
…uched An unvalidated digest was joined onto the pooch cache directory and used as the fetch URL suffix, so a path such as ../../etc/passwd escaped the cache. Validates once at the top of has and fetch instead of per branch. Also keeps the R2 credentials out of the generated repr, reads a single-character URL scheme as a Windows drive letter, and fixes the verb agreement in the docs.
1 participant
Continues the test-cases simplification on top of #856. Two independent changes, one per commit.
Folds
buildandsyncintorunandreplaybuildwasrunwithout the execution, so it becomesrun --from-slot. That reuses the native already inoutput/<label>/and regenerates only the committed bundle, under the same promotion gate as a normal run.syncfetched every manifest blob into a temporary directory purely for the store cache side effect.replayalready does that when it materialises, so the verb and the drift workflow's sync step are both gone.Collapses the native store into one class
NativeStorewas a runtime-checkable Protocol with three implementations and a factory that picked between them. It is now a single class whose URL decides the backend: afile://URL or bare path reads and writes locally in the two-level layout, anhttp(s)URL reads anonymously through pooch, and the same URL with write credentials adds the S3 upload path used bymint. Public operations, error messages andbuild_native_storeare unchanged, boto3 stays lazy, and the digest validation that keeps a hostile digest inside the store root stays where it was.Net effect is about 900 fewer lines across source and tests.
Summary by CodeRabbit
Breaking Changes
test-cases buildandtest-cases synccommands.test-cases run --from-slot.Improvements