Skip to content

test(purchase): stub RecoverStrandedApprovals late-completion mock to fix panic - #663

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/657-stranded-approvals-mock-panic
May 22, 2026
Merged

test(purchase): stub RecoverStrandedApprovals late-completion mock to fix panic#663
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/657-stranded-approvals-mock-panic

Conversation

@cristim

@cristim cristim commented May 22, 2026

Copy link
Copy Markdown
Member

Closes #657. Stubs the GetExecutionByID call in the RecoverStrandedApprovals race-disambiguation path so the late-completion-not-clobbered test no longer panics on an unstubbed mock. Full internal/purchase suite green with -race (144 pass).

Summary by CodeRabbit

  • Tests
    • Enhanced test coverage for purchase execution recovery scenarios.

Review Change Stack

… test does not panic

When TransitionExecutionStatus returns an error, RecoverStrandedApprovals
calls GetExecutionByID to distinguish a race (the row already left
"approved") from a real store failure. The test
TestManager_RecoverStrandedApprovals_LateCompletionNotClobbered had no
expectation for GetExecutionByID, so testify panicked on the unstubbed
call (mocks_test.go:277).

Add a stub returning Status="completed" so the manager correctly skips
the execution and the assertion (recovered==0) still genuinely proves
that a row which completed between the stale SELECT and the recovery
UPDATE is not clobbered by the sweep.

Closes #657
@cristim cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/internal Team-internal only effort/s Hours type/bug Defect labels May 22, 2026
@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 971a08ee-9747-4816-bbee-9f466bdc9db4

📥 Commits

Reviewing files that changed from the base of the PR and between e09d0a7 and fa76556.

📒 Files selected for processing (1)
  • internal/purchase/manager_test.go

📝 Walkthrough

Walkthrough

This PR fixes a test panic in TestManager_RecoverStrandedApprovals_LateCompletionNotClobbered by adding a missing mock stub. The test now stubs GetExecutionByID to return a completed execution, allowing the recovery path to verify it correctly skips already-completed executions when a status transition fails.

Changes

Recovery Race Detection Test

Layer / File(s) Summary
Mock completion check in late-completion recovery test
internal/purchase/manager_test.go
Mock expectation for GetExecutionByID(ctx, "exec-raced") returns an execution with Status: "completed", verifying the recovery sweep correctly interprets a failed status transition as a race condition by fetching the row state and skipping the execution.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related issues

  • LeanerCloud/CUDly#657: Directly fixes the panic caused by the unstubbed GetExecutionByID call in the late-completion recovery path.
  • LeanerCloud/CUDly#651: Addresses the same failing test and missing mock for GetExecutionByID in RecoverStrandedApprovals race detection.

Possibly related PRs

  • LeanerCloud/CUDly#635: The main PR's GetExecutionByID stub addition directly refines the stranded-approval recovery race-handling behavior introduced by that PR's RecoverStrandedApprovals implementation.

Suggested labels

severity/low, bug

Poem

🐰 A test that panicked without a trace,
Now mocks the fetch that handles the race,
The sweep sees completion and skips with grace,
No clobbering here—just the right embrace! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the fix: stubbing a mock call (GetExecutionByID) in the RecoverStrandedApprovals test to resolve a panic.
Linked Issues check ✅ Passed The PR directly addresses issue #657 by adding the missing GetExecutionByID mock stub, allowing the test to exercise the late-completion-not-clobbered path without panicking.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to fixing the mock panic in the single test file without introducing unrelated modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/657-stranded-approvals-mock-panic

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit f1e3251 into feat/multicloud-web-frontend May 22, 2026
4 checks passed
@cristim
cristim deleted the fix/657-stranded-approvals-mock-panic branch June 3, 2026 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant