test: increased test coverage for failover#450
Conversation
📝 WalkthroughWalkthroughThis PR adds test coverage without modifying production code. New unit tests exercise SQLite failover store CRUD operations, timestamps, and list ordering, plus fallback resolver behavior with dynamic rule providers. The E2E scenario document is expanded from 132 to 141 scenarios, adding a manual failover management section. ChangesTest Coverage Additions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SantiagoDePolonia
changed the title
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/failover/store_sqlite_test.go`:
- Around line 65-89: The update-path test in store_sqlite_test.go documents that
updated_at advances, but it never verifies that behavior in the Rule upsert
flow. In the test around store.Upsert and store.Get, capture the original
UpdatedAt after the initial insert, then assert the post-update UpdatedAt is not
earlier than the original value since timestamps are stored as Unix seconds and
can tie within the same second. Use the existing Rule fields and the Upsert/Get
assertions to locate the check, and keep the verification focused on the
updated_at field alongside the existing CreatedAt preservation assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e0c08b2-27e8-45e0-988d-5b4b7e667edd
📒 Files selected for processing (3)
internal/failover/store_sqlite_test.gointernal/fallback/resolver_test.gotests/e2e/release-e2e-scenarios.md
| // Update via ON CONFLICT: targets/enabled change, created_at is preserved even | ||
| // though the caller passes a different value, while updated_at advances. | ||
| update := Rule{ | ||
| Source: "openai/gpt-5", | ||
| Targets: []string{"groq/llama-3.3-70b-versatile"}, | ||
| Enabled: false, | ||
| ManagedSource: ManagedSourceDashboard, | ||
| CreatedAt: time.Unix(5000, 0).UTC(), | ||
| } | ||
| if err := store.Upsert(ctx, update); err != nil { | ||
| t.Fatalf("Upsert(update) error = %v", err) | ||
| } | ||
| got, err = store.Get(ctx, "openai/gpt-5") | ||
| if err != nil || got == nil { | ||
| t.Fatalf("Get() after update = %+v, %v", got, err) | ||
| } | ||
| if got.Enabled { | ||
| t.Fatalf("Get().Enabled = true after disabling; enabled=false must round-trip") | ||
| } | ||
| if !reflect.DeepEqual(got.Targets, []string{"groq/llama-3.3-70b-versatile"}) { | ||
| t.Fatalf("Get().Targets = %v, want updated single target", got.Targets) | ||
| } | ||
| if got.CreatedAt.Unix() != 1000 { | ||
| t.Fatalf("Get().CreatedAt = %d after update, want 1000 (ON CONFLICT must not touch created_at)", got.CreatedAt.Unix()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Test doesn't actually verify the "updated_at advances" claim it documents.
The comment at Lines 65-66 states the update path should show updated_at advancing, but only Enabled, Targets, and CreatedAt are asserted after the update (Lines 81-89). Capture UpdatedAt after the insert and assert against it post-update. Since timestamps are persisted as Unix seconds (per sqliteUpsertArgs/stampUpsert), a strict After check could be flaky if both calls land in the same second — use a non-decreasing check instead.
🔧 Proposed fix
if got.UpdatedAt.IsZero() {
t.Fatalf("Get().UpdatedAt is zero; stampUpsert should set it")
}
+ firstUpdatedAt := got.UpdatedAt
// Update via ON CONFLICT: targets/enabled change, created_at is preserved even
// though the caller passes a different value, while updated_at advances.
@@
if got.CreatedAt.Unix() != 1000 {
t.Fatalf("Get().CreatedAt = %d after update, want 1000 (ON CONFLICT must not touch created_at)", got.CreatedAt.Unix())
}
+ if got.UpdatedAt.Before(firstUpdatedAt) {
+ t.Fatalf("Get().UpdatedAt = %v after update, want >= %v (ON CONFLICT must advance updated_at)", got.UpdatedAt, firstUpdatedAt)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Update via ON CONFLICT: targets/enabled change, created_at is preserved even | |
| // though the caller passes a different value, while updated_at advances. | |
| update := Rule{ | |
| Source: "openai/gpt-5", | |
| Targets: []string{"groq/llama-3.3-70b-versatile"}, | |
| Enabled: false, | |
| ManagedSource: ManagedSourceDashboard, | |
| CreatedAt: time.Unix(5000, 0).UTC(), | |
| } | |
| if err := store.Upsert(ctx, update); err != nil { | |
| t.Fatalf("Upsert(update) error = %v", err) | |
| } | |
| got, err = store.Get(ctx, "openai/gpt-5") | |
| if err != nil || got == nil { | |
| t.Fatalf("Get() after update = %+v, %v", got, err) | |
| } | |
| if got.Enabled { | |
| t.Fatalf("Get().Enabled = true after disabling; enabled=false must round-trip") | |
| } | |
| if !reflect.DeepEqual(got.Targets, []string{"groq/llama-3.3-70b-versatile"}) { | |
| t.Fatalf("Get().Targets = %v, want updated single target", got.Targets) | |
| } | |
| if got.CreatedAt.Unix() != 1000 { | |
| t.Fatalf("Get().CreatedAt = %d after update, want 1000 (ON CONFLICT must not touch created_at)", got.CreatedAt.Unix()) | |
| } | |
| // Update via ON CONFLICT: targets/enabled change, created_at is preserved even | |
| // though the caller passes a different value, while updated_at advances. | |
| update := Rule{ | |
| Source: "openai/gpt-5", | |
| Targets: []string{"groq/llama-3.3-70b-versatile"}, | |
| Enabled: false, | |
| ManagedSource: ManagedSourceDashboard, | |
| CreatedAt: time.Unix(5000, 0).UTC(), | |
| } | |
| if err := store.Upsert(ctx, update); err != nil { | |
| t.Fatalf("Upsert(update) error = %v", err) | |
| } | |
| got, err = store.Get(ctx, "openai/gpt-5") | |
| if err != nil || got == nil { | |
| t.Fatalf("Get() after update = %+v, %v", got, err) | |
| } | |
| if got.Enabled { | |
| t.Fatalf("Get().Enabled = true after disabling; enabled=false must round-trip") | |
| } | |
| if !reflect.DeepEqual(got.Targets, []string{"groq/llama-3.3-70b-versatile"}) { | |
| t.Fatalf("Get().Targets = %v, want updated single target", got.Targets) | |
| } | |
| if got.CreatedAt.Unix() != 1000 { | |
| t.Fatalf("Get().CreatedAt = %d after update, want 1000 (ON CONFLICT must not touch created_at)", got.CreatedAt.Unix()) | |
| } | |
| if got.UpdatedAt.Before(firstUpdatedAt) { | |
| t.Fatalf("Get().UpdatedAt = %v after update, want >= %v (ON CONFLICT must advance updated_at)", got.UpdatedAt, firstUpdatedAt) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/failover/store_sqlite_test.go` around lines 65 - 89, The update-path
test in store_sqlite_test.go documents that updated_at advances, but it never
verifies that behavior in the Rule upsert flow. In the test around store.Upsert
and store.Get, capture the original UpdatedAt after the initial insert, then
assert the post-update UpdatedAt is not earlier than the original value since
timestamps are stored as Unix seconds and can tie within the same second. Use
the existing Rule fields and the Upsert/Get assertions to locate the check, and
keep the verification focused on the updated_at field alongside the existing
CreatedAt preservation assertions.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Update via ON CONFLICT: targets/enabled change, created_at is preserved even | ||
| // though the caller passes a different value, while updated_at advances. | ||
| update := Rule{ | ||
| Source: "openai/gpt-5", | ||
| Targets: []string{"groq/llama-3.3-70b-versatile"}, | ||
| Enabled: false, | ||
| ManagedSource: ManagedSourceDashboard, | ||
| CreatedAt: time.Unix(5000, 0).UTC(), | ||
| } | ||
| if err := store.Upsert(ctx, update); err != nil { | ||
| t.Fatalf("Upsert(update) error = %v", err) | ||
| } | ||
| got, err = store.Get(ctx, "openai/gpt-5") | ||
| if err != nil || got == nil { | ||
| t.Fatalf("Get() after update = %+v, %v", got, err) | ||
| } | ||
| if got.Enabled { | ||
| t.Fatalf("Get().Enabled = true after disabling; enabled=false must round-trip") | ||
| } | ||
| if !reflect.DeepEqual(got.Targets, []string{"groq/llama-3.3-70b-versatile"}) { | ||
| t.Fatalf("Get().Targets = %v, want updated single target", got.Targets) | ||
| } | ||
| if got.CreatedAt.Unix() != 1000 { | ||
| t.Fatalf("Get().CreatedAt = %d after update, want 1000 (ON CONFLICT must not touch created_at)", got.CreatedAt.Unix()) | ||
| } |
There was a problem hiding this comment.
UpdatedAt advancement assertion after upsert
The comment on line 65 says "while updated_at advances", but no assertion after the second Upsert verifies that UpdatedAt actually changed. The post-update block only checks Enabled, Targets, and CreatedAt. If stampUpsert stopped writing UpdatedAt on a conflict update, this test would still pass, defeating its stated purpose of catching that regression.
| .primary_model == $s | ||
| and (.fallback_models | length) == 2 | ||
| and .fallback_models[0] == "openai/gpt-4.1-mini" | ||
| and .enabled == false | ||
| ' >/dev/null | ||
| curl -fsS "$BASE_URL/admin/failover" \ | ||
| | jq -e --arg s "$SRC" 'any(.[]; .primary_model == $s and .enabled == false)' >/dev/null | ||
| HEADERS_FILE=$(mktemp "$QA_RUN_DIR/s134.headers.XXXXXX") | ||
| curl -sS -D "$HEADERS_FILE" -o /dev/null -X DELETE "$BASE_URL/admin/failover" \ | ||
| -H 'Content-Type: application/json' -d "{\"primary_model\":\"$SRC\"}" | ||
| grep -Eiq '^HTTP/.* 204 ' "$HEADERS_FILE" | ||
| ``` | ||
|
|
||
| ### S135 Disable a primary with an empty target list | ||
|
|
||
| A disabled mapping is allowed to omit targets, which records the primary as a | ||
| failover-disabled source rather than rejecting the request. | ||
|
|
||
| ```bash | ||
| SRC="qa-fo-dis-$QA_SUFFIX" | ||
| curl -fsS -X PUT "$BASE_URL/admin/failover" -H 'Content-Type: application/json' \ |
There was a problem hiding this comment.
The scenario explicitly notes "the current status code is 502 provider_error" for what is a client-supplied validation error (an enabled mapping with no targets). This makes grep -Eiq '^HTTP/.* 502 ' the pass condition. When this bug is fixed and the endpoint returns the semantically correct 400 invalid_request_error, S139 will start failing and could block a release, forcing the fixer to remember to update this scenario. Consider using a looser assertion — e.g., grep -Eiq '^HTTP/.* [45]' — plus the message match as the durable invariant, so that fixing the status code does not require a simultaneous doc update.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Summary by CodeRabbit
Documentation
Tests