iframe-proxy

cristim · 2026-06-11T05:55:39Z

Problem

Closes #1154 (review finding TEST-02, P2).

The CLI purchase-path tests in cmd/ were exercise-only or tautological:

TestRunToolFromCSV drove the CSV purchase entry point and asserted nothing (assert.NotPanics). Worse, its fixture used CSV headers the parser does not recognize (Instance Type, Instance Count), so every row loaded with Count=0, the entire run processed zero recommendations, and the test stayed green, exactly the silent-regression mode the finding warned about.
TestRunToolFromCSV_NonExistentFile and _EmptyFile were t.Skip-ped, citing integration coverage that does not exist; runToolFromCSV called log.Fatalf, making the branches structurally untestable.
TestQueryMajorEngineVersions_Success and _ProfileHandling were tautological and non-hermetic: they loaded live AWS config and asserted on whichever branch the environment produced, issuing real AWS API calls on credentialed machines with no testing.Short() guard, so they ran under CI's go test -short ./....

Fix

runToolFromCSV now returns an error instead of calling log.Fatalf; runToolMultiService keeps the fail-loud CLI behavior by turning the returned error fatal. No behavior change for users.
TestRunToolFromCSV asserts observable output: the written purchase report's resource types, post-sizing counts, and dry-run success flags, for both the 100% and 50% coverage paths, using the real round-trip column names (ResourceType, Count, ...).
The two skipped error-path tests are un-skipped with real assertions on the wrapped error chain.
_WithMaxInstances / _WithOverrideCount assert the instance cap and count override against the report rows instead of NotPanics.
The live-AWS engine-version tests are replaced by hermetic stubbed-client tests of queryMajorEngineVersionsWithClient (full engine fan-out, result merging, warn-and-continue on per-engine failure) plus a deterministic profile-precedence test that fails at config load time against a redirected empty shared config, so no network or credentials are involved.
New isolateAWSEnv helper pins invalid static credentials so the CSV-path orchestration tests behave identically on credentialed developer machines and bare CI runners, and never touch a real account.

Test evidence

go build ./... clean; go vet ./cmd/ clean; gofmt -l cmd/ clean.
go test ./cmd/ -run 'TestRunToolFromCSV|TestQueryMajorEngineVersions': 16 passed.
go test -short ./cmd/...: 752 passed in 7 packages.
Regression-test validity: the error-path tests could not exist pre-fix (log.Fatalf exits the process), and the happy-path assertions were observed failing against the old broken fixture shape (report never written because the run processed zero recommendations) before the fixture was corrected, demonstrating they catch the silent-orchestration-regression mode described in the finding.

Out of scope (sibling occurrences, same defect shape)

TestQueryMajorEngineVersions_ErrorHandling and TestQueryRunningInstanceEngineVersions_ErrorHandling in cmd/multi_service_engine_versions_test.go have the same live-config tautological pattern but were not cited by TEST-02; left untouched to keep the diff focused.

coderabbitai · 2026-06-11T05:55:45Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7135b828-f94c-48e3-b3b2-8d2e542f1554

📥 Commits

Reviewing files that changed from the base of the PR and between 81cd1f4 and 12e80c5.

📒 Files selected for processing (3)

cmd/multi_service.go
cmd/multi_service_coverage_test.go
cmd/multi_service_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch test/test-02-cli-purchase-path-tests

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

cristim · 2026-06-11T05:56:11Z

@coderabbitai review

coderabbitai · 2026-06-11T05:56:17Z

✅ Action performed

Review finished.

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 · 2026-06-19T15:35:47Z

@coderabbitai review

coderabbitai · 2026-06-19T15:35:53Z

✅ Action performed

Review finished.

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 · 2026-06-26T17:28:25Z

Adversarial review

What this PR does well:

Correctly diagnoses the silent-fixture bug. The old fixture used CSV headers (Instance Type, Instance Count) that buildColumnIndexMap does not recognize, so getCSVField returned "" and parseCSVRecord parsed Count=0; the whole run processed zero recommendations while assert.NotPanics stayed green. Replacing the headers with the round-trip column names (ResourceType, Count, ...) is the right call.
Expected values are derived from the algorithm, not "whatever the current code outputs": with 50% coverage int(2 * 0.5) = 1 (kept), int(1 * 0.5) = 0 (dropped by newCount > 0). That matches ApplyCoverage in cmd/helpers.go:165-167 and would catch a regression that broke the discrete-count scaling.
isolateAWSEnv is a clean pattern: pinning invalid static creds + AWS_CONFIG_FILE=os.DevNull makes the orchestration tests deterministic on credentialed laptops and bare CI runners alike.
Un-skipping _NonExistentFile and _EmptyFile against the wrapped error chain is exactly what TEST-02 asked for. The error matching (failed to read CSV file + failed to open/read CSV header) anchors on the wrapping runToolFromCSV adds plus the underlying loadRecommendationsFromCSV error, so a future refactor that drops the outer wrap or stops wrapping fails the test.

Findings:

1. TestQueryMajorEngineVersionsWithClient_EngineErrorContinues locks in a memory-violating contract.
The test comment says "per-engine API failures are warn-and-continue" and asserts require.NoError(t, err) for ANY error returned by the per-engine call. But fetchMajorEngineVersionsForEngine returns ctx.Err() (line 236-237 of cmd/multi_service_engine_versions.go) on context cancellation, and queryMajorEngineVersionsWithClient (line 220-224) treats that the same as a transient throttling error: log a warning and move to the next engine. This is the exact pattern flagged by feedback_ctx_cancel_terminal.md ("Context cancellation is terminal in loops -- treat context.Canceled/DeadlineExceeded as hard stops in API fan-out loops; never accumulate as lastErr and continue").

The test as written makes this bug harder to fix because the comment legitimizes the behavior as a contract. The underlying fix is small (~5 LOC: if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return nil, err } in queryMajorEngineVersionsWithClient) plus a paired test asserting ctx.Canceled propagates. Filed as #1325.

2. "Real behavior" only covers the dry-run path.
All four runToolFromCSV tests pin ActualPurchase=false. The PR title is "make CSV purchase-path tests assert real behavior" but the actual-purchase orchestration (real processPurchaseLoop, adjustRecsForDuplicates hitting DescribeReservedDBInstances / DescribeReservedCacheNodes / DescribeSavingsPlans, real CommitmentID flow) is unverified. A regression that quietly bypassed the purchase-loop on the real path while still writing a dry-run-shaped report would not be caught. Filed as #1326.

3. CSV parser still silently tolerates unknown / missing required columns -- the root cause TEST-02 worked around.
With wrong column names, getCSVField returns "" for the missing columns and parseCSVRecord happily builds a Recommendation{Count: 0, ResourceType: "", ...}. loadRecommendationsFromCSV prints "Loaded N recommendations" and returns success. The fix in this PR corrects the FIXTURE, not the PARSER. Per feedback_no_silent_fallbacks.md (money path), the parser should fail loudly when required columns (Service, ResourceType, Count, Term, PaymentOption) are missing, instead of producing Count=0 rows that silently no-op the entire run. The PR's test is now a witness for the parser regressing back into silence -- but only because the test ASSERTS report rows; nothing in the parser itself errors on the wrong-header case. Filed as #1327.

4. CSV edge-case coverage is thin.
_EmptyFile only covers the 0-byte case. Headers-only-no-rows would surface loadRecommendationsFromCSV returning an empty slice and runToolFromCSV printing "No recommendations to process" with no error -- not currently tested. Also untested: BOM-prefixed file, CRLF vs LF line endings, UTF-16/Latin-1, quoted commas, quoted newlines, mismatched field counts (encoding/csv would error with csv.ErrFieldCount), numeric overflow on Count (strconv.Atoi -> overflow path). Folded into #1327.

5. Idempotency not tested.
Feeding the same CSV twice into a real-purchase run isn't covered. Memory feedback_upsert_key_matches_hash.md is adjacent: the report's CommitmentID should change OR a duplicate-suppression path should fire on the second run, depending on how adjustRecsForDuplicates interacts with newly-purchased RIs. Folded into #1326.

6. PR-acknowledged sibling occurrences.
TestQueryMajorEngineVersions_ErrorHandling and TestQueryRunningInstanceEngineVersions_ErrorHandling in cmd/multi_service_engine_versions_test.go have the same live-AWS + tautological pattern (test passes whether AWS creds are present or not, comment "important thing is that the function doesn't panic"). PR body explicitly leaves them for a follow-up. Filed as #1328.

Notes on the CI failures (UNSTABLE state):

Lint Code, Run pre-commit hooks, Security Scanning, Integration Tests, CI Success are all pre-existing failures on main (errcheck across cmd/configure_azure.go, cmd/configure_gcp.go, cmd/cleanup-lambda/main.go, cmd/rekey/main.go, internal/api/handler.go; TestPostgresStore_UpsertRecommendations_* integration failures; govulncheck HIGH/CRITICAL). None are caused by this PR's three-file diff (cmd/multi_service.go, cmd/multi_service_coverage_test.go, cmd/multi_service_test.go) -- the Unit Tests job that exercises the actual changes is GREEN. Will not block merge on these.

Verification done locally:

go build ./... clean against this branch.
go vet ./cmd/ clean.
gofmt -l cmd/ clean.
go test -short ./cmd/ -run 'TestRunToolFromCSV|TestQueryMajorEngineVersions' -- 16/16 pass.

cristim · 2026-06-26T17:28:32Z

@coderabbitai review

coderabbitai · 2026-06-26T17:28:37Z

✅ Action performed

Review finished.

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 · 2026-07-10T14:06:48Z

@coderabbitai review

coderabbitai · 2026-07-10T14:06:55Z

✅ Action performed

Review finished.

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 · 2026-07-10T22:07:56Z

@coderabbitai review

coderabbitai · 2026-07-10T22:08:01Z

✅ Action performed

Review finished.

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 · 2026-07-16T20:21:30Z

Rebased onto origin/main (73166c2). Clean rebase (conflict in cmd/multi_service.go resolved by keeping main's helper functions + PR's return nil / error-returning signatures, and PR's test struct fields). Gates: go build, go vet, gocyclo -over 10, golangci-lint (0 new issues). Tests: cmd (739 passed).

cristim · 2026-07-16T20:21:53Z

@coderabbitai review

coderabbitai · 2026-07-16T20:21:59Z

✅ Action performed

Review finished.

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.

runToolFromCSV now returns an error instead of calling log.Fatalf, so its error branches are testable; runToolMultiService keeps the fail-loud CLI behavior by turning the returned error fatal. Test changes: - TestRunToolFromCSV asserts observable output (the written purchase report: resource types, counts, dry-run success flags) instead of assert.NotPanics. The old fixture used CSV headers the parser does not recognize (Instance Type, Instance Count), so every row loaded with Count=0, the whole run processed nothing, and NotPanics stayed green; the fixture now uses the real round-trip column names. - Un-skip TestRunToolFromCSV_NonExistentFile and _EmptyFile with real error assertions (previously skipped citing nonexistent integration coverage). - TestRunToolFromCSV_WithMaxInstances/_WithOverrideCount assert the cap and override against the report rows. - Replace tautological, live-AWS TestQueryMajorEngineVersions_Success and _ProfileHandling (ran real API calls under CI go test -short on credentialed machines) with hermetic stubbed-client tests of queryMajorEngineVersionsWithClient and a deterministic profile-precedence test that fails at config load, no network. - isolateAWSEnv pins invalid static credentials so the CSV-path tests behave identically on credentialed and bare machines. Closes #1154

Making runToolFromCSV return an error (so the CSV purchase path is unit-testable) added an `if err != nil { log.Fatalf }` branch to runToolMultiService, pushing its cyclomatic complexity from 10 to 11 and tripping the gocyclo pre-commit hook (over 10). Extract the error-to-fatal glue into runCSVPathOrFatal so the entrypoint drops back to complexity 10 while runToolFromCSV keeps its testable error return.

cristim · 2026-07-16T21:04:03Z

Rebased onto current origin/main (81cd1f4) to pick up the flaky-test fix from #1399 (TestVerifyRetry_RebuildRateLimited is now deterministic). The two PR-owned commits are the only ones on this branch; validator_test.go now matches main's version. All gates passed: go build, go vet, scheduledauth -count=5 (exit 0), cmd package tests, gocyclo -over 10 (clean), golangci-lint full run (0 issues).

cristim · 2026-07-16T22:42:08Z

…ath-tests test(cmd): make CSV purchase-path tests assert real behavior

cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-quarter Within the quarter impact/internal Team-internal only effort/m Days type/chore Maintenance / non-user-visible labels Jun 11, 2026

cristim force-pushed the test/test-02-cli-purchase-path-tests branch from 6dd5831 to 496a914 Compare June 19, 2026 15:35

cristim force-pushed the test/test-02-cli-purchase-path-tests branch from 496a914 to 2d88fbd Compare July 10, 2026 14:06

cristim force-pushed the test/test-02-cli-purchase-path-tests branch from 2d88fbd to 5b35a70 Compare July 10, 2026 22:07

cristim force-pushed the test/test-02-cli-purchase-path-tests branch from 5b35a70 to b39daab Compare July 16, 2026 20:20

cristim added 2 commits July 17, 2026 00:01

cristim force-pushed the test/test-02-cli-purchase-path-tests branch from b39daab to 12e80c5 Compare July 16, 2026 21:03

cristim merged commit cad9c72 into main Jul 16, 2026
19 checks passed

cristim deleted the test/test-02-cli-purchase-path-tests branch July 16, 2026 22:42

cristim added a commit that referenced this pull request Sep 27, 2026

Merge pull request #1225 from LeanerCloud/test/test-02-cli-purchase-p…

7a7e6f9

…ath-tests test(cmd): make CSV purchase-path tests assert real behavior

cristim mentioned this pull request Oct 7, 2026

fix(cli): treat cancelled context as terminal in engine-version fan-out #2139

Merged

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

cristim commented Jun 11, 2026

Problem

Fix

Test evidence

Out of scope (sibling occurrences, same defect shape)

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

cristim commented Jun 11, 2026

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 19, 2026

Uh oh!

coderabbitai Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 26, 2026

Adversarial review

Uh oh!

cristim commented Jun 26, 2026

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 10, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 10, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 16, 2026

Uh oh!

cristim commented Jul 16, 2026

Uh oh!

coderabbitai Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 16, 2026

Uh oh!

Uh oh!

cristim commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

coderabbitai Bot commented Jun 19, 2026 •

edited

Loading

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading