test: add shared off-test-goroutine assertion helper and convert the concurrency-sensitive suites

[rudrankriyam]

Why

Three consecutive review rounds flagged the same defect class: t.Fatal / t.Fatalf called from code that does not run on the goroutine running the test.

• Fix availability bulk edit resilience #2053 and Add verified remove-from-sale command #2055 (pricing availability edit, remove-from-sale) — RoundTrippers driven by the territory worker pool.
• feat: add official search optimization plan #2057 (ads search optimization) — httptest handlers; that thread asked for a shared repo-wide helper instead of another isolated fix.

The agreed fix each time was t.Errorf plus an explicit return. The helper was never built, so the idiom has been re-derived by hand, inconsistently, in every round.

t.Fatal runs runtime.Goexit on whichever goroutine calls it. Inside an httptest handler that skips the response write and leaves the client under test waiting for a reply that never arrives. Inside a worker goroutine it kills the worker before it can send its result, so the test blocks on a channel or wait group until the package timeout panics, with the real assertion buried in the goroutine dump.

Measured on TestConcurrentApplyInitializesOnlyAfterReadingStateUnderRunLock, forcing its writeRun guard to fire:

stub reports with	result
t.Fatal (before)	panic: test timed out after 20s, no assertion message
handlertest (after)	--- FAIL (0.00s) naming both the violated guard and the run error it produced

What this adds

internal/handlertest: one Asserter, bound with handlertest.New(t), covering the three shapes the hazard takes.

method	for	returns
Errorf(format, args...)	RoundTrippers, dependency stubs, worker goroutines	error carrying the same message
Respond(w, format, args...)	httptest handlers, which cannot return	writes HTTP 500 + JSON error envelope
Response(format, args...)	fixture builders that must hand back a response	synthetic HTTP 500 *http.Response

Each records once with Errorf, which is safe from any goroutine, and hands the caller a value so the code under test observes a deterministic failure and unwinds normally. Messages render through fmt.Errorf, so %w keeps wrapping the operand and still prints readably in the test log — that collapses the t.Errorf(...) + duplicated fmt.Errorf(...) pairs the previous PRs had to write by hand into a single call with one message.

internal/handlertest/handlertest_test.go proves the semantics: a deliberately failing handler answers a real client with a 500 inside a bounded deadline (a hang fails the test rather than running to the package timeout), the envelope carries the assertion detail, a failing stub lets its worker goroutine return through its channel, and every path reports exactly once. testing.TB cannot be implemented outside testing, so Asserter holds a narrow reporter interface and the tests inject a recorder that captures reports without failing the run.

docs/TESTING.md documents the convention so the next reviewer can point at it.

Blast radius, measured

AST pass over the whole tree (go/parser), counting t.Fatal, t.Fatalf, and t.FailNow lexically inside a func literal whose signature is func(http.ResponseWriter, *http.Request) or func(*http.Request) (*http.Response, error):

• 4,290 call sites across 344 files at this branch's HEAD (4,299 across 345 before this PR).
• 485 in handler literals, 3,805 in RoundTripper literals, 0 directly inside go func() {} bodies.
• Concentration: internal/cli/cmdtest 3,472 sites / 254 files, internal/web 273 / 19, internal/cli/web 74 / 10, internal/cli/apps 68 / 2, internal/cli/publish 66 / 5, internal/cli/assets 59 / 6, internal/cli/shared 50 / 5.
• An earlier coarse regex estimated ~169 files; the real file count is about double.

That number is a ceiling on the pattern, not on the hazard. A Fatal in a RoundTripper only deadlocks when the code under test issues that request off the test goroutine; most of the 3,805 are sequential fixtures where Fatal is merely ugly. It also misses two indirect classes the lexical scan cannot see, both present in this batch: fixture helpers called from inside a RoundTripper, and injected dependency stubs invoked on a worker goroutine.

Deliberately not a mass rewrite. This converts the sharp subset — suites whose production code fans out — and leaves the rest for follow-ups now that the helper and the documented convention exist.

Converted in this batch

file	why it is sharp	change
internal/cli/cmdtest/pricing_availability_edit_territory_test.go	updateTerritoryAvailabilityTargets patches from a worker pool	7 t.Errorf + duplicated fmt.Errorf pairs collapsed; 1 t.Fatalf in the response builder called from the transport
internal/cli/cmdtest/pricing_availability_remove_from_sale_test.go	same worker pool	3 pairs collapsed; 1 t.Fatalf in territoryAvailabilityResponse; 4 silent unexpected-request guards now report
internal/cli/validate/subscription_fetch_test.go	readiness drives these fetches from bounded workers	9 RoundTripper t.Fatalf converted, two of which returned (nil, nil) after the Fatal — a nil response handed to the client if it ever ran on
internal/cli/validate/concurrency_test.go	httptest handler serving a six-way concurrent read fan-out	unexpected-request default answered 500 without failing the test; now reports through Respond
internal/cli/distribute/orchestration_test.go	executeDistributionApply runs on a worker goroutine	2 stub t.Fatal calls — the hang measured above

13 t.Fatal/t.Fatalf removed from off-test-goroutine code, 10 report/return pairs collapsed, 7 previously silent guards now name their cause.

Not converted

• internal/cli/ads/search_optimization_test.go — open PR fix(ads): bound optimization body pagination #2071 owns the file; next batch, after it lands.
• Everything else in the survey. The concentration in internal/cli/cmdtest, internal/web, and internal/cli/web is the natural next sweep, prioritised by whether the command under test fans out.

Behavior

Mechanism only, not assertions. Every converted test still fails on exactly the conditions it failed on before — the fixture now reports and answers instead of terminating its goroutine. Where a Fatal guarded an unrecoverable fixture error (a marshal failure, an unexpected request), the replacement still fails the test and additionally propagates a deterministic error or 500 to the code under test, so the outer assertion fails too rather than hanging.

The seven previously silent guards are the one place strictness increases: they returned an error or a 500 without recording anything, so an unexpected request could only surface indirectly. Each converted suite was re-run to confirm it stays green, which also confirms none of them currently hits those branches.

Verification

• make build, make format, make check-docs, GOLANGCI_LINT_TIMEOUT="10m --allow-parallel-runners" make lint (0 issues), ASC_BYPASS_KEYCHAIN=1 make test — full suite green.
• go test ./internal/cli/validate ./internal/cli/cmdtest -run TestPricingAvailability -race -count=2 green.
• RED proof per suite: broke a pricing fixture path deliberately — failed in 0.02s at the fixture line with the request that violated it, no hang.
• No live API calls; ASC_BYPASS_KEYCHAIN=1 throughout.

Summary by CodeRabbit

• Documentation

  • Added guidance for safely reporting failures from concurrent test routines.
  • Documented helper usage for transport errors, handlers, HTTP responses, and wrapped errors.

• Tests

  • Improved concurrent test reliability by preventing premature goroutine termination and hangs.
  • Added comprehensive coverage for failure reporting, HTTP 500 responses, formatted errors, and wrapped errors.
  • Updated existing tests to use consistent failure-handling helpers without changing their scenarios or assertions.

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
rudrankriyam-app-store-connect-cli-67	🟡 Building	–	Aug 19, 2026, 2:56 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f6dfa2ad-ae82-46ef-8412-74c213a2242b

📥 Commits

Reviewing files that changed from the base of the PR and between bbfeff8 and 8439188.

📒 Files selected for processing (8)

• docs/TESTING.md
• internal/cli/cmdtest/pricing_availability_edit_territory_test.go
• internal/cli/cmdtest/pricing_availability_remove_from_sale_test.go
• internal/cli/distribute/orchestration_test.go
• internal/cli/validate/concurrency_test.go
• internal/cli/validate/subscription_fetch_test.go
• internal/handlertest/handlertest.go
• internal/handlertest/handlertest_test.go

Limit details: You’ve used the included review currently available. Your 70 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

---

📝 Walkthrough

Walkthrough

The PR adds the internal/handlertest package for goroutine-safe test failure reporting. It migrates affected CLI and validation tests from fatal calls and manual responses. It also documents the new helpers.

Changes

Safe test failure handling

Layer / File(s)	Summary
Add handlertest assertion API internal/handlertest/handlertest.go	Adds Asserter, New, Errorf, Respond, and Response. The helpers report failures and return errors or HTTP 500 JSON responses.
Test assertion and response behavior internal/handlertest/handlertest_test.go	Tests reporter binding, non-blocking worker behavior, error wrapping, HTTP response delivery, and exactly-once reporting.
Migrate test handlers and transports internal/cli/cmdtest/..., internal/cli/distribute/orchestration_test.go, internal/cli/validate/..., docs/TESTING.md	Replaces unsafe fatal calls and manual failure responses with handlertest helpers. Adds usage guidance for handlers, transports, worker goroutines, and dependency stubs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 84391

This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains.

Possibly related PRs

• rorkai/App-Store-Connect-CLI#2053: Both PRs modify the pricing availability territory test, but #2053 adds resilience and concurrency scenarios.

Suggested labels: medium

🚥 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 clearly and concisely describes the new shared helper and the conversion of concurrency-sensitive test suites.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ 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/handler-assert-helper

Usage-based review receipt

• Mode: Continue automatically
• Reviewed files: 8
• Charged: $2.00
• View usage details

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. Track spend and usage in your billing settings.

---

_{Comment @coderabbitai help to get the list of available commands.}