test: stabilize flaky TestCtxUsage - #70214
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthrough
ChangesGCS test token flow
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #70214 +/- ##
================================================
- Coverage 76.3237% 73.7952% -2.5286%
================================================
Files 2041 2060 +19
Lines 559257 580082 +20825
================================================
+ Hits 426846 428073 +1227
- Misses 131511 151610 +20099
+ Partials 900 399 -501
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/objstore/gcs_test.go (1)
638-643: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDeclare the token error response as JSON.
Line 641 writes a JSON body, but the handler leaves the response as the default
text/plain. SetContent-Type: application/jsonbefore Line 640 so the mock accurately exercises the OAuth error contract rather than relying on parser permissiveness.Suggested fix
if r.URL.Path == "/token" { + w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusBadRequest)🤖 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 `@pkg/objstore/gcs_test.go` around lines 638 - 643, Update the HTTP handler in the httptest.NewServer setup to set the response Content-Type header to application/json before writing the token error body in the /token branch. Preserve the existing 400 status and JSON payload.
🤖 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.
Nitpick comments:
In `@pkg/objstore/gcs_test.go`:
- Around line 638-643: Update the HTTP handler in the httptest.NewServer setup
to set the response Content-Type header to application/json before writing the
token error body in the /token branch. Preserve the existing 400 status and JSON
payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb471b3b-db6c-48b5-8868-d2a101fbac26
📒 Files selected for processing (1)
pkg/objstore/gcs_test.go
What problem does this PR solve?
Issue Number: close #70211
Problem Summary:
Flaky test
TestCtxUsageinpkg/objstoreintermittently fails, so this PR stabilizes that path.What changed and how does it work?
Root Cause
TestCtxUsage accidentally depended on real Google STS timing while asserting an invalid_request credential error.
Fix
Local token_url preserves the same external-account credential exchange but makes the expected invalid_request deterministic.
Verification
Spec:
pkg/objstore :: TestCtxUsagetidb.go_flaky.defaultBASELINE_ONLYGO_TEST_WITH_TAGSintest, deadlockbaseline_onlyObserved result:
Gate checklist:
Commands:
HTTPS_PROXY=<local CONNECT proxy that stalls TLS after CONNECT> NO_PROXY=127.0.0.1,localhost go test -json -tags=intest,deadlock ./pkg/objstore -run '^TestCtxUsage$' -count=1Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Fixes #70211
Summary by CodeRabbit