Hypothesis state-machine tests for pipeline completion (#73) - #243
Open
leynos wants to merge 3 commits into
Open
Hypothesis state-machine tests for pipeline completion (#73)#243leynos wants to merge 3 commits into
leynos wants to merge 3 commits into
Conversation
Pipeline completion ordering lived inline in _process_completed_task, which mutated _PipelineWaitState, read the clock, and terminated stages in one async method — untestable at the state-transition level. Extract the pure decision into _PipelineWaitState.record_completion( completed_idx, exit_code, *, ended_at): it stamps the exit code and end time (clock injected), latches the first non-zero exit in completion order as failure_index, and returns whether the remaining downstream stages must be terminated. _process_completed_task now calls it and keeps the clock read and the async termination side effect. Add cuprum/unittests/test_pipeline_wait.py: a Hypothesis RuleBasedStateMachine drives random completion orders (restarting fresh pipelines when drained) and asserts first-failure semantics, timing-slot population, and the termination decision; example tests pin the boundaries (first-completed failure wins, final-stage failure requests no termination, all-success records no failure, fail-fast fires once). Regenerate the maturin wheel-manifest snapshot for the new test file. Closes #73 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's GuideExtracts pipeline completion decision logic into a pure Sequence diagram for pipeline task completion and fail-fast terminationsequenceDiagram
participant Task
participant _process_completed_task
participant _PipelineWaitState
participant _terminate_pipeline_remaining_stages
Task->>_process_completed_task: result()
_process_completed_task->>_PipelineWaitState: record_completion(idx, exit_code, ended_at)
alt [record_completion returns True]
_process_completed_task->>_terminate_pipeline_remaining_stages: _terminate_pipeline_remaining_stages(processes, wait_tasks, idx, cancel_grace)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Summary
WalkthroughChangesPipeline completion handling
Sequence Diagram(s)sequenceDiagram
participant CompletedTask
participant _process_completed_task
participant _PipelineWaitState
participant _terminate_pipeline_remaining_stages
CompletedTask->>_process_completed_task: stage exit code and completion time
_process_completed_task->>_PipelineWaitState: record_completion(idx, exit_code, ended_at)
_PipelineWaitState-->>_process_completed_task: should_terminate_others(idx)
_process_completed_task->>_terminate_pipeline_remaining_stages: terminate remaining stages
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87e8aae8a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cuprum/_pipeline_wait.py`:
- Around line 73-76: Update the transition documentation around the completion
condition to state that _terminate_pipeline_remaining_stages terminates every
still-running stage except the failed stage, rather than only remaining
downstream stages. Preserve the existing condition and the description that I/O
remains with the caller.
In `@cuprum/unittests/test_pipeline_wait.py`:
- Line 27: Add substantive async tests for _process_completed_task in
test_pipeline_wait.py, covering completed and termination cases, including
termination invocation and correct clock/index argument forwarding. Exercise the
actual async integration boundary rather than only record_completion, and assert
the expected state/results for each path.
- Around line 88-118: Update the assertions in the stateful and example test
sections, including the flow around record_completion and the additional ranges
123-127 and 147-191, to provide descriptive diagnostic messages instead of using
bare asserts. Cover every assertion, including state initialization,
pending-stage updates, timing and exit-code checks, failure_index, and
termination expectations, with messages that identify the relevant state and
expected values.
🪄 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: 36bb32ad-5983-4db4-b3a6-190fc75782ba
📒 Files selected for processing (3)
cuprum/_pipeline_wait.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_pipeline_wait.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Review findings on the completion transition: - record_completion both mutated the wait state and returned the termination decision, so a query was inseparable from a command, against the command/query segregation rule in AGENTS.md. Split it: record_completion is now a command returning None, and the new should_terminate_others(completed_idx) query reports the fail-fast decision from state without changing it, so it is repeatable and order-independent. _process_completed_task calls the command, then the query. - AGENTS.md requires function documentation to demonstrate usage and outcome; both methods gained worked examples. - The docstring described terminating the "remaining downstream stages". _terminate_pipeline_remaining_stages stops every still-running stage except the failed one, upstream included, so the wording now says "every other still-running stage". - The suite only covered the pure transition, so it could not catch inverted or omitted termination, or a mis-forwarded clock or index. Add TestProcessCompletedTask, which drives the real async boundary with a recording termination double and an injected perf_counter. Verified non-vacuous: omitting the termination call and forwarding the wrong clock each fail these tests. - Every bare assertion in the suite gained a diagnostic message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The pre-merge Developer Documentation check flagged that the new record_completion / should_terminate_others seam was described only in docstrings, with the design doc covering fail-fast behaviour generally. Add a "Completion ordering seam" subsection to the fail-fast policy in the design doc: what each half of the command/query split does, that the clock is injected so the transition is deterministic, that completion order rather than stage order decides which failure latches, and that _process_completed_task is the sole caller joining the two. Also correct the adjacent policy wording, which said fail-fast terminates "the remaining stages": it terminates every other still-running stage, upstream included. That is the same inaccuracy already corrected in the docstrings on this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
3 participants
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.
Summary
Pipeline completion ordering — first-failure latching, timing-slot population, and the fail-fast termination decision — lived inline in
_process_completed_task, which mutated_PipelineWaitState, read the clock, and terminated stages in one async method. It was untestable at the state-transition level (#73).Seam
Extract the pure decision into
_PipelineWaitState.record_completion(completed_idx, exit_code, *, ended_at):exit_codes[idx]andended_at[idx](the clock is injected asended_at, so the transition is deterministic),failure_index,Trueiff this completion is that first failure and the stage is not the final one — the sole condition under which the caller terminates the remaining downstream stages._process_completed_tasknow reads the clock and performs the async termination side effect around this pure call, so behaviour is unchanged.Tests
cuprum/unittests/test_pipeline_wait.py:RuleBasedStateMachinedrives randomized completion orders across 1–8 stages, restarting a fresh pipeline once drained, and asserts on every step: the exit code and timestamp land in the right slots,failure_indexequals the first-completed non-zero exit and never changes, and the termination flag matchesfirst_failure and idx != last. An invariant checksfailure_indexalways points at a recorded non-zero exit.The transition needs no event loop or subprocess, so the machine exercises completion ordering directly.
Validation
Full gates green:
make check-fmt,make lint(ruff, interrogate 100%, pylint 10.00/10),make test(826 passed / 58 skipped incl. the new suite; Rust nextest 57/57). The maturin wheel-manifest snapshot is regenerated for the new test file.Closes #73
🤖 Generated with Claude Code
Summary by Sourcery
Extract the pipeline completion decision logic into a pure
_PipelineWaitState.record_completiontransition and adapt the async handler to delegate to it while preserving behavior.Enhancements:
record_completionmethod on_PipelineWaitStateto encapsulate exit-code, timing, and fail-fast bookkeeping separate from async side effects.Tests:
_PipelineWaitState.record_completionto validate completion ordering, first-failure semantics, and termination decisions across varied pipelines.