Skip to content

test(purchase): regression guard for parallel multi-account fan-out (#210) - #245

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
test/multi-account-isolation
May 3, 2026
Merged

test(purchase): regression guard for parallel multi-account fan-out (#210)#245
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
test/multi-account-isolation

Conversation

@cristim

@cristim cristim commented May 3, 2026

Copy link
Copy Markdown
Member

Summary

Closes #210 — adds the parallelism regression guard for executeMultiAccount that the existing TestExecuteMultiAccount_PartialFailure_IsolatesAccounts doesn't catch.

The existing isolation test would still pass if the fan-out were refactored into a serial for loop. Spec acceptance criterion E-2 explicitly calls out parallelism as a separate promise — this test pins it.

Mechanics

  • Two valid AWS accounts (A + B), both with valid mock credentials.
  • Each PurchaseCommitment mock blocks for 300 ms.
  • Wall-clock the whole executePurchase call.
    • Parallel fan-out: ~310 ms (one delay + scheduling overhead).
    • Serial loop: ~600 ms (sum of two delays).
    • Threshold: 500 ms — comfortably between the two.

Verified locally via mutation testing: rewrote executeMultiAccount to a serial for loop and ran the test:

"603.577875ms" is not less than "500ms"
--- FAIL: TestExecuteMultiAccount_RunsAccountsInParallel (0.60s)

After restoring the errgroup-style fan-out the test passes at ~310 ms.

What this does NOT cover (per #210 out-of-scope)

  • Load testing of CUDLY_MAX_ACCOUNT_PARALLELISM (separate ticket if needed).
  • Credential refresh failures mid-execution.
  • Mocking the actual AWS purchase APIs end-to-end.

Test plan

  • go test ./internal/purchase/ -run TestExecuteMultiAccount_RunsAccountsInParallel -v -count=1 — passes at ~310 ms.
  • go test ./internal/purchase/ -count=1 — full package green.
  • go build ./... clean.
  • go vet ./... clean.
  • Mutation test: serial-loop refactor causes the test to fail with the expected error message (verified locally; mutation reverted before commit).
  • All pre-commit hooks pass (gofmt, govet, gocyclo, gosec, trivy, AWS secret scan, full Go tests).

Summary by CodeRabbit

  • Tests
    • Added concurrency regression test for multi-account purchase execution to verify parallel processing and performance behavior.

…210)

The companion TestExecuteMultiAccount_PartialFailure_IsolatesAccounts
already pins per-account error isolation but would still pass if
executeMultiAccount were refactored to a serial for-loop. Spec
acceptance criterion E-2 calls out parallelism as a separate promise
("pick a sentinel that fails on a serial loop but passes on errgroup")
— this test pins it.

Two valid AWS accounts each block PurchaseCommitment for 300ms.
Parallel fan-out completes in ~310ms; a serial loop takes ~600ms. The
500ms threshold sits comfortably between the two so CI scheduler jitter
does not flake it but a serial-loop regression is caught (verified via
local mutation: serial fails at ~603ms, parallel passes at ~310ms).

Closes #210.
@coderabbitai

coderabbitai Bot commented May 3, 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: a4eff94e-3900-4571-af3c-bacbfd8fbb8b

📥 Commits

Reviewing files that changed from the base of the PR and between c6588bb and 4833083.

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

📝 Walkthrough

Walkthrough

A concurrency regression test is added to verify that multi-account purchase execution runs in parallel across AWS accounts rather than serially. The test mocks two accounts with timed calls, asserts completion within a parallel upper bound, and verifies correct execution record counts and mock interactions.

Changes

Multi-Account Execution Parallelism Test

Layer / File(s) Summary
Test Setup & Mock Configuration
internal/purchase/execution_test.go
TestExecuteMultiAccount_RunsAccountsInParallel creates two mock AWS accounts and configures mocks for SavePurchaseExecution to record per-account executions, with provider/client mocks injecting delays via time.Sleep(perCallDelay) to simulate realistic call latencies.
Parallelism Assertion & Verification
internal/purchase/execution_test.go
manager.executePurchase is invoked and completion time is measured against parallelMaxBound to catch serial fan-out regressions; test asserts no error, exactly two execution records saved, and all mock expectations met.

Sequence Diagram(s)

(Skipped: test-only change with no new feature or control flow modifications)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 Hop, hop—two accounts now race!
No serial slowdown to embrace,
Our test keeps time with mighty care,
Ensures the parallelism's there!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR adds a test with two accounts and parallel execution timing bounds, but does not fully implement the linked issue criteria: it lacks verification of independent result_data, proper error sanitization checks, and uses only valid accounts instead of one valid and one invalid. Add assertions for independent result_data between accounts, verify error sanitization (no credential material), and include both valid and invalid mock credentials as specified in #210 acceptance criteria.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adding a regression test for parallel multi-account fan-out execution, which directly matches the test addition in the changeset.
Out of Scope Changes check ✅ Passed The PR correctly adds only a regression test for parallel multi-account execution without introducing load testing, credential refresh logic, or end-to-end AWS mocking, staying within defined scope.
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 test/multi-account-isolation

Review rate limit: 4/5 reviews remaining, refill in 12 minutes.

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

@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/all-users Affects every user effort/m Days type/chore Maintenance / non-user-visible labels May 3, 2026
@cristim

cristim commented May 3, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 3, 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 bc25a4d into feat/multicloud-web-frontend May 3, 2026
4 checks passed
@cristim
cristim deleted the test/multi-account-isolation branch June 3, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/all-users Affects every user priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant