test(metadata): harden Retry-After clamp boundary + log tests (bookshelf-8oj1) #1026

Merged

zombor merged 1 commit from bd-bookshelf-8oj1 into main 2026-07-08 05:32:53 +00:00

Conversation 1 Commits 1 Files changed 2 +78

zombor commented 2026-07-08 05:05:46 +00:00

Owner

Copy link

Summary

• Adds a test for the exact-60s boundary in ParseRetryAfter (confirming clamped=false when value equals cap exactly — the > vs >= correctness verified by 0sha refactor)
• Adds three log-output tests in googlebooks/ratelimit_test.go pinning the retry_after_clamped field: set for values > cap (9999), set for overflow input (1e300), absent for value == cap (60)
• No production code changed; the underlying logic was already correct after the 0sha refactor

Test plan

• go test ./internal/metadata/... passes (all new tests green)
• New tests exercise the boundary/overflow inputs identified in PR #569 review
• Log-capture tests use a 100ms context timeout to trigger sleep cancellation without hanging

Closes bead bookshelf-8oj1 on merge.

## Summary - Adds a test for the exact-60s boundary in `ParseRetryAfter` (confirming `clamped=false` when value equals cap exactly — the `>` vs `>=` correctness verified by 0sha refactor) - Adds three log-output tests in `googlebooks/ratelimit_test.go` pinning the `retry_after_clamped` field: set for values > cap (9999), set for overflow input (1e300), absent for value == cap (60) - No production code changed; the underlying logic was already correct after the 0sha refactor ## Test plan - `go test ./internal/metadata/...` passes (all new tests green) - New tests exercise the boundary/overflow inputs identified in PR #569 review - Log-capture tests use a 100ms context timeout to trigger sleep cancellation without hanging Closes bead bookshelf-8oj1 on merge.

zombor added 1 commit 2026-07-08 05:05:46 +00:00

test(metadata): harden Retry-After clamp boundary + log tests (bookshelf-8oj1) ... e4ff89a622

Add explicit tests for the ParseRetryAfter overflow/boundary edge cases raised in
the PR #569/0sha review:

1. provider_helpers_test.go: add test asserting that Retry-After = "60" (exactly
   MaxRetryAfterWait) returns clamped=false — confirming the boundary is strict-
   greater-than, not greater-than-or-equal.

2. googlebooks/ratelimit_test.go: add three tests that capture log output to verify
   the retry_after_clamped log field is:
   - present when Retry-After > cap (9999 → clamped to 60s → field set)
   - present for an astronomically large value (1e300 → float-overflow guard → field set)
   - absent when Retry-After equals the cap exactly (60 → not clamped → field absent)

No production code changed; the underlying logic was already correct after the
0sha refactor (strict-greater-than comparison in float space before int64
conversion). These tests document and pin that behavior.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-08 05:24:11 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED — 0 blocker, 0 major, 1 minor

Phase 0: DEMO Verification

This PR is test-only (no production code); the "DEMO" is CI green. Verified via diff inspection per the review-standard instructions (no test re-run).

Phase 1: Spec Compliance

Truly test-only confirmed. Diff touches exactly two files, both *_test.go:

• internal/metadata/provider_helpers_test.go
• internal/metadata/googlebooks/ratelimit_test.go

No production file changed. Scope is correct.

All four cases from the bead description are covered:

1. ParseRetryAfter("60", MaxRetryAfterWait) → (MaxRetryAfterWait, false) — provider_helpers_test.go new It
2. Retry-After: 9999 → retry_after_clamped present in log — ratelimit_test.go new Describe #1
3. Retry-After: 1e300 → retry_after_clamped present in log — ratelimit_test.go new Describe #2
4. Retry-After: 60 → retry_after_clamped absent from log — ratelimit_test.go new Describe #3

Phase 2: Code Quality

Boundary correctness — verified against production code

provider_helpers.go:57: if secs > maxSecs { return capDur, true } — strictly greater-than.

• "60" → secs=60.0, maxSecs=60.0 → 60 > 60 = false → returns (60s, false) ✓ test asserts clamped=BeFalse() ✓
• "9999" → 9999 > 60 = true → returns (60s, true) ✓ test asserts field present ✓
• "1e300" → 1e300 > 60 = true (no overflow, comparison is in float64 space) → (60s, true) ✓ test asserts field present ✓

All three boundary cases are semantically correct.

Log field assertions — meaningful, not vacuous

ratelimit_test.go (new Describe #3, Retry-After=60): NotTo(ContainSubstring("retry_after_clamped")) could pass vacuously if no Warn was ever logged. Verified it cannot: handle429 always emits logger.Warn("google books: retryable status", ...) before calling SleepWithContext — the log line is written unconditionally before the sleep that times out. The buffer will contain the Warn entry (just without the clamped field). Assertion is meaningful. ✓

ContainSubstring("retry_after_clamped") on slog JSON output: the key name retry_after_clamped is unique — "retry_after" does not contain "retry_after_clamped" as a substring. No false-positive risk from the shorter sibling field. ✓

Black-box / package declarations

• provider_helpers_test.go: package metadata_test ✓
• ratelimit_test.go: package googlebooks_test ✓

Both black-box. No unexported symbols referenced. ✓

No coverage exclusions or golangci changes

No exclusion list edits anywhere in the diff. ✓

---

[MINOR] internal/metadata/provider_helpers_test.go new boundary It — two separate Expect calls in one It block (Expect(d).To(...) then Expect(clamped).To(BeFalse())). Per project conventions, each It should assert exactly one behavior; the boundary test should be split: one It for d == MaxRetryAfterWait and one It for clamped == false. Note: this pattern already exists in the same file at the "caps at capDur" and "does not clamp when value is within cap" Its — the new test follows established (but non-compliant) file convention. No correctness impact; does not block.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

CODE REVIEW: APPROVED — 0 blocker, 0 major, 1 minor ## Phase 0: DEMO Verification This PR is test-only (no production code); the "DEMO" is CI green. Verified via diff inspection per the review-standard instructions (no test re-run). ## Phase 1: Spec Compliance **Truly test-only confirmed.** Diff touches exactly two files, both `*_test.go`: - `internal/metadata/provider_helpers_test.go` - `internal/metadata/googlebooks/ratelimit_test.go` No production file changed. Scope is correct. **All four cases from the bead description are covered:** 1. `ParseRetryAfter("60", MaxRetryAfterWait)` → `(MaxRetryAfterWait, false)` — `provider_helpers_test.go` new It 2. `Retry-After: 9999` → `retry_after_clamped` present in log — `ratelimit_test.go` new Describe #1 3. `Retry-After: 1e300` → `retry_after_clamped` present in log — `ratelimit_test.go` new Describe #2 4. `Retry-After: 60` → `retry_after_clamped` absent from log — `ratelimit_test.go` new Describe #3 ## Phase 2: Code Quality ### Boundary correctness — verified against production code `provider_helpers.go:57`: `if secs > maxSecs { return capDur, true }` — strictly greater-than. - `"60"` → `secs=60.0`, `maxSecs=60.0` → `60 > 60` = false → returns `(60s, false)` ✓ test asserts `clamped=BeFalse()` ✓ - `"9999"` → `9999 > 60` = true → returns `(60s, true)` ✓ test asserts field present ✓ - `"1e300"` → `1e300 > 60` = true (no overflow, comparison is in float64 space) → `(60s, true)` ✓ test asserts field present ✓ All three boundary cases are semantically correct. ### Log field assertions — meaningful, not vacuous `ratelimit_test.go` (new Describe #3, Retry-After=60): `NotTo(ContainSubstring("retry_after_clamped"))` could pass vacuously if no Warn was ever logged. Verified it cannot: `handle429` always emits `logger.Warn("google books: retryable status", ...)` before calling `SleepWithContext` — the log line is written unconditionally before the sleep that times out. The buffer will contain the Warn entry (just without the clamped field). Assertion is meaningful. ✓ `ContainSubstring("retry_after_clamped")` on slog JSON output: the key name `retry_after_clamped` is unique — `"retry_after"` does not contain `"retry_after_clamped"` as a substring. No false-positive risk from the shorter sibling field. ✓ ### Black-box / package declarations - `provider_helpers_test.go`: `package metadata_test` ✓ - `ratelimit_test.go`: `package googlebooks_test` ✓ Both black-box. No unexported symbols referenced. ✓ ### No coverage exclusions or golangci changes No exclusion list edits anywhere in the diff. ✓ --- [MINOR] `internal/metadata/provider_helpers_test.go` new boundary It — two separate `Expect` calls in one `It` block (`Expect(d).To(...)` then `Expect(clamped).To(BeFalse())`). Per project conventions, each `It` should assert exactly one behavior; the boundary test should be split: one `It` for `d == MaxRetryAfterWait` and one `It` for `clamped == false`. Note: this pattern already exists in the same file at the "caps at capDur" and "does not clamp when value is within cap" Its — the new test follows established (but non-compliant) file convention. No correctness impact; does not block. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-8oj1 from e4ff89a622 to 4a500635e5 2026-07-08 05:27:53 +00:00 Compare

zombor merged commit 72644f7894 into main 2026-07-08 05:32:53 +00:00

zombor referenced this pull request from a commit 2026-07-08 05:32:54 +00:00

Merge pull request 'test(metadata): harden Retry-After clamp boundary + log tests (bookshelf-8oj1)' (#1026) from bd-bookshelf-8oj1 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1026

No description provided.