Add selector concurrency property tests (#57) - #62
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:
WalkthroughSplits ChangesTest suite refactoring and reorganisation
Sequence Diagram(s) sequenceDiagram
participant T1 as Thread 1
participant Checkpoint as _CheckpointBackendSelector
participant Lock as _BACKEND_LOCK/_SignallingRLock
participant T2 as Thread 2
participant Env as backend_env
T1->>Checkpoint: enter_context(backend A)
Checkpoint->>Lock: acquire
T1->>Env: mutate backend env
T2->>Checkpoint: enter_context(backend B)
Checkpoint->>Lock: wait_for_release
T1->>Lock: release on context exit
T2->>Lock: acquire
T2->>Env: observe serialised env state
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (16 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds property-based and checkpointed concurrency tests around File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@coderabbitai please create new GitHub issues for the following: High-Value Candidates CrossHair is only a good fit where the target can be made pure and bounded. For async subprocess, file descriptor, or OS-environment code, Hypothesis with fakes is the better immediate tool; CrossHair becomes useful after extracting pure state-transition helpers.
Recommended Order
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create all 14 GitHub issues now. 🏁 Script executed: Length of output: 622
|
|
@coderabbitai please create new GitHub issues for the following: Ranked Candidates Baseline: I found no Rust-level
Tool Fit
Highest-ROI First Step Add Rust unit/property test support and start with |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/developers-guide.md`:
- Line 307: The doc uses British "-ise" forms; update all occurrences of the
tokens "serialisation", "serialises", and "serialised" in
docs/developers-guide.md to the en-GB-oxendict "-ize" forms "serialization",
"serializes", and "serialized" respectively (including the other occurrences
noted in the review), by performing a targeted find-and-replace so the
documentation conforms to repository spelling standards.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7d3229e5-de2e-452e-8341-660e3035f3d2
📒 Files selected for processing (2)
docs/developers-guide.mdrust/cuprum-rust/tests/ui/fail/invalid_pymodule_return.stderr
💤 Files with no reviewable changes (1)
- rust/cuprum-rust/tests/ui/fail/invalid_pymodule_return.stderr
Add Hypothesis coverage for generated backend selector reentrancy and concurrent worker completion inputs. Document the selector state-machine invariants and add checkpointed interleaving coverage for lock-serialised environment mutation.
Separate core worker execution, selector concurrency, and CLI validation coverage into focused test modules. Move the Syrupy snapshots to the matching module snapshot files so the existing snapshot assertions keep using the same keys after collection.
Use the availability-aware backend strategy for generated reentrancy cases and centralize concurrency wait/join timeouts so slow CI tuning only changes one place. Leave the remaining two-thread orchestration cases separate because their event sequencing differs and the shared multi-worker path already uses `_assert_backend_pair_completes`.
Refresh the maturin wheel contents snapshot so it expects the split tee profile worker test modules and their renamed snapshot files.
Instrument the test-only backend lock wrapper so the interleaving test sets `second_waiting_for_lock` only after a blocking acquire observes the lock is already held. Keep production selector code unchanged and preserve the existing serialised environment observation assertion.
Move backend availability decisions out of module-level constants and into small factory helpers so the concurrency tests avoid import-time conditional work. Use a deferred Hypothesis strategy for generated backend names while preserving the existing test settings and assertions.
Share the timeout join and alive-thread assertion used by the selector interleaving tests so both scenarios report unfinished worker threads in one consistent form.
- Add `timeout`, `capture_output`, and `text` options to the worker CLI subprocess test so hanging or non-zero exits surface immediate diagnostics. - Centralise worker-thread completion checks in `_assert_backend_pair_completes` using `_join_and_assert_finished`. - Assert that the interleaving selector test waits for the release signal before the second thread observes environment state.
Address the Sourcery "Developer Documentation" warning for #57. Add a developers' guide section describing the three-way split of the tee_profile_worker test suite (core, CLI, concurrency) and the Hypothesis-based and checkpointed concurrency approach used to verify the `_EnvBackendSelector` invariants: lock hold duration, environment restoration, cache clearing, and same-thread reentrancy rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The repository follows en-GB Oxford spelling (-ize), as in the existing "serialized"/"serializes" usage in docs/cuprum-design.md and the prevalent "normalized", "synchronized", and "optimization" forms across the docs. Align the newly added selector concurrency section by replacing "serialisation", "serialises", and "serialised" with their -ize forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebasing the worker test-suite documentation onto main's new "Rust property testing and verification" section left two double blank lines around the inserted headings, tripping markdownlint MD012. Collapse them to single blank lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
af8468c to
ef488d2
Compare
Rebase #58 (EnvBackendSelector lock-contention and reentrant-rejection metrics) onto origin/main. Main's #57/#62 reorganised the tee-profile worker tests into core/concurrency/cli modules, while this branch had split the same original file into result/selector_guard/selector_metrics. The two refactors were structurally incompatible, so this re-integrates the branch's net value onto main's now-canonical layout rather than replaying the old structural commits: - Keep the observability-metrics implementation in ``benchmarks/tee_profile_worker.py`` (``_MetricsState``, the selector ``metrics_state`` property, lock-wait/rejection accounting) plus the two new ``TeeProfileWorkerResult`` fields. - Expose ``metrics_state`` on main's ``_CoordinatedBackendSelector`` and ``_CheckpointBackendSelector`` so they satisfy the ``BackendSelector`` protocol that ``run_tee_profile_worker`` now relies on. - Add ``test_tee_profile_worker_selector_metrics.py`` with the metrics tests, including the two reentrant-rejection cases that previously lived in the branch's selector_guard module; drop the branch's result/selector_guard/ helpers modules as main's core/concurrency modules supersede them. - Regenerate the worker-core and maturin-build snapshots for the new metrics fields and the updated test-file inventory. - Re-apply the benchmark validator split, docstrings, docs links, selector metrics doc section, CrossHair confirmation-budget fix, and other #58 work that main did not touch. Cargo and uv lockfiles are taken from main unchanged; no dependency changes were introduced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The rebase onto main brings the interrogate 100% docstring gate (#131) together with the selector concurrency test helpers (#57/#62), whose `_RLockLike` protocol methods and `_SignallingRLock` dunder/`release` methods lacked docstrings. Add minimal NumPy-style docstrings so `make lint` passes after the rebase. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebase #58 (EnvBackendSelector lock-contention and reentrant-rejection metrics) onto origin/main. Main's #57/#62 reorganised the tee-profile worker tests into core/concurrency/cli modules, while this branch had split the same original file into result/selector_guard/selector_metrics. The two refactors were structurally incompatible, so this re-integrates the branch's net value onto main's now-canonical layout rather than replaying the old structural commits: - Keep the observability-metrics implementation in ``benchmarks/tee_profile_worker.py`` (``_MetricsState``, the selector ``metrics_state`` property, lock-wait/rejection accounting) plus the two new ``TeeProfileWorkerResult`` fields. - Expose ``metrics_state`` on main's ``_CoordinatedBackendSelector`` and ``_CheckpointBackendSelector`` so they satisfy the ``BackendSelector`` protocol that ``run_tee_profile_worker`` now relies on. - Add ``test_tee_profile_worker_selector_metrics.py`` with the metrics tests, including the two reentrant-rejection cases that previously lived in the branch's selector_guard module; drop the branch's result/selector_guard/ helpers modules as main's core/concurrency modules supersede them. - Regenerate the worker-core and maturin-build snapshots for the new metrics fields and the updated test-file inventory. - Re-apply the benchmark validator split, docstrings, docs links, selector metrics doc section, CrossHair confirmation-budget fix, and other #58 work that main did not touch. Cargo and uv lockfiles are taken from main unchanged; no dependency changes were introduced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebase #58 (EnvBackendSelector lock-contention and reentrant-rejection metrics) onto origin/main. Main's #57/#62 reorganised the tee-profile worker tests into core/concurrency/cli modules, while this branch had split the same original file into result/selector_guard/selector_metrics. The two refactors were structurally incompatible, so this re-integrates the branch's net value onto main's now-canonical layout rather than replaying the old structural commits: - Keep the observability-metrics implementation in ``benchmarks/tee_profile_worker.py`` (``_MetricsState``, the selector ``metrics_state`` property, lock-wait/rejection accounting) plus the two new ``TeeProfileWorkerResult`` fields. - Expose ``metrics_state`` on main's ``_CoordinatedBackendSelector`` and ``_CheckpointBackendSelector`` so they satisfy the ``BackendSelector`` protocol that ``run_tee_profile_worker`` now relies on. - Add ``test_tee_profile_worker_selector_metrics.py`` with the metrics tests, including the two reentrant-rejection cases that previously lived in the branch's selector_guard module; drop the branch's result/selector_guard/ helpers modules as main's core/concurrency modules supersede them. - Regenerate the worker-core and maturin-build snapshots for the new metrics fields and the updated test-file inventory. - Re-apply the benchmark validator split, docstrings, docs links, selector metrics doc section, CrossHair confirmation-budget fix, and other #58 work that main did not touch. Cargo and uv lockfiles are taken from main unchanged; no dependency changes were introduced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Integrate selector observability metrics onto restructured worker tests Rebase #58 (EnvBackendSelector lock-contention and reentrant-rejection metrics) onto origin/main. Main's #57/#62 reorganised the tee-profile worker tests into core/concurrency/cli modules, while this branch had split the same original file into result/selector_guard/selector_metrics. The two refactors were structurally incompatible, so this re-integrates the branch's net value onto main's now-canonical layout rather than replaying the old structural commits: - Keep the observability-metrics implementation in ``benchmarks/tee_profile_worker.py`` (``_MetricsState``, the selector ``metrics_state`` property, lock-wait/rejection accounting) plus the two new ``TeeProfileWorkerResult`` fields. - Expose ``metrics_state`` on main's ``_CoordinatedBackendSelector`` and ``_CheckpointBackendSelector`` so they satisfy the ``BackendSelector`` protocol that ``run_tee_profile_worker`` now relies on. - Add ``test_tee_profile_worker_selector_metrics.py`` with the metrics tests, including the two reentrant-rejection cases that previously lived in the branch's selector_guard module; drop the branch's result/selector_guard/ helpers modules as main's core/concurrency modules supersede them. - Regenerate the worker-core and maturin-build snapshots for the new metrics fields and the updated test-file inventory. - Re-apply the benchmark validator split, docstrings, docs links, selector metrics doc section, CrossHair confirmation-budget fix, and other #58 work that main did not touch. Cargo and uv lockfiles are taken from main unchanged; no dependency changes were introduced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix runtime Path annotations and per-selector metrics isolation Two review fixes in the benchmark worker surface: - benchmarks/_benchmark_types.py: import ``pathlib as pth`` at module runtime rather than only under ``TYPE_CHECKING``. ``PipelineBenchmarkConfig`` and ``PipelineBenchmarkRunResult`` annotate fields as ``pth.Path``, so under ``from __future__ import annotations`` a ``typing.get_type_hints(...)`` call raised ``NameError: name 'pth' is not defined``. The import carries a ``# noqa: TC003`` with a comment because the alias is needed at runtime; the repo-wide ``runtime-evaluated-decorators`` alternative would have forced 25 unrelated TC004 import moves across the codebase. - benchmarks/tee_profile_worker.py: ``_EnvBackendSelector`` now creates its own ``_MetricsState(threading.local())`` when no ``metrics_state`` is injected, instead of falling back to a module-global singleton. Two selectors on the same thread no longer share or reset each other's counters. The module-global ``_metrics_state`` is removed; ``run_tee_profile_worker`` already reads ``selector.metrics_state``, so worker and selector still agree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review: timing, docstring, int validation, metrics test - tee_profile_worker._build_worker_result: capture wall_time_seconds before _manifest_hash and _scenario_label run, so manifest I/O no longer inflates the measured worker-run duration. - _benchmark_type_validators: correct the module docstring's exception contract. It claimed all wrong-type inputs raise TypeError, but several helpers (_validate_backend, _validate_non_empty_string, _validate_payload_ bytes) raise ValueError; state that each helper raises TypeError or ValueError per its own contract. - tee_profile_scenarios._check_int_bound: validate the int type up front via the shared _validate_int (rejecting non-int and bool deterministically with TypeError, consistent with the other benchmark validators) before the range comparisons. The local helper is retained rather than replaced wholesale because perf-frequency needs an unbounded maximum that the centralised _validate_iteration_count (fixed 1000 cap) cannot express. - test_tee_profile_worker_cli: assert the exported metrics fields (reentrant_rejection_count, lock_wait_seconds) in the subprocess JSON output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix rebased selector test helpers Forward metrics_state from the coordinated selector test helper so it satisfies the BackendSelector protocol after rebasing onto main's split worker-test layout. Add the missing test-helper docstrings and adjust a comment that Ruff parsed as an invalid noqa directive, keeping the lint gate clean. * Address selector observability review (#58) Signal test lock contention only immediately before the blocking acquire, so contention tests observe the real blocking path rather than the probe. Route tee-profile driver bounds through the shared benchmark validators and make alternate-backend test setup fail explicitly when Rust is unavailable instead of falling back to `auto`. Clarify that selector observability metrics are per worker invocation and update the documented Windows wheel platforms. * Sort concurrent worker type imports (#58) Order the type-only imports in the concurrent worker tests so Ruff's import sorting gate passes after the `ParameterSet` annotation change. * Collapse duplicate blank lines in the developers' guide Rebasing onto the merged toolchain-pin docs left double blank lines at the seams between the existing tail sections and the sections this branch appends, failing markdownlint MD012. * Test worker metrics reset on selector reuse (#58) Add an integration test that pre-populates an injected selector's metrics state before running `run_tee_profile_worker`. This proves the worker resets selector metrics at the run boundary, rather than relying only on isolated `_MetricsState.reset()` coverage. * Document selector metrics architecture (#58) Add the design-level description for tee profile selector metrics, including per-run reset scope, the `BackendSelector.metrics_state` protocol property, and deterministic clock injection for lock-wait timing. * Cover worker repeat upper bound in CLI tests (#58) Add the missing invalid `repeat_count=1001` parametrized case so the CLI configuration tests cover both lower and upper repeat-count validation. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
This branch adds property-based and checkpointed concurrency coverage for issue #57. It generates backend selector reentrancy cases, exercises concurrent worker completion across generated backend sequences and thread counts, and documents the
_EnvBackendSelectorstate-machine invariants that the tests exercise.Closes #57.
Review walkthrough
Validation
uv run pytest cuprum/unittests/test_tee_profile_worker.py: 20 passed.make check-fmt: passed.make lint: passed.make typecheck: passed.make test: 474 passed, 43 skipped.Notes
coderabbit review --agentwas attempted twice after the implementation milestone. The CLI returned a recoverable service-side rate limit both times, so there were no CodeRabbit concerns to clear locally.Summary by Sourcery
Add property-based and checkpointed concurrency coverage for the _EnvBackendSelector used by tee_profile_worker, and reorganize its test suite into focused core, CLI, and concurrency modules.
Enhancements:
Tests: