Repository navigation
controlbridge, sqlreplay: fix Go lint and flaky TestS3DirWatcher on rust - #1390
Conversation
- controlbridge: remove the unused recordingHandler test double. Its last callers were deleted with the legacy Go route path, and golangci-lint's `unused` check now fails pull-check on every PR against rust. - sqlreplay/store: TestS3DirWatcher capped the S3 mock at MaxTimes(4) while polling every 10ms, so a slow CI runner exhausted the expectation and the watcher stopped seeing new dirs. Allow any number of calls, wait up to 5s instead of 3 poll intervals, and guard the files map that the watcher goroutine writes while the test reads it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review: 1
Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ti-chi-bot
Bot
added
the
size/L
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## rust #1390 +/- ##
=======================================
Coverage ? 58.58%
=======================================
Files ? 214
Lines ? 29850
Branches ? 0
=======================================
Hits ? 17488
Misses ? 10876
Partials ? 1486
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djshow832 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
3 participants
What problem does this PR solve?
Issue Number: ref #1388
Problem Summary:
Every PR against the
rustbranch currently fails two Go CI jobs for reasons unrelated to its diff (seen on #1388):pull-check: golangci-lintunusedreports 11 errors inpkg/controlbridge/helpers_test.go. TherecordingHandlertest double lost its last callers when the legacy Go route path was deleted, and the fork's CI never ran Go lint, so it went unnoticed.pull-unit-test:TestS3DirWatcheris timing-sensitive. The S3 mock is capped atMaxTimes(4)while the watcher polls every 10ms; on a slow runner the expectation runs out, the watcher stops seeing the new directory, and theEventually(3 poll intervals = 30ms) fails.What is changed and how it works:
recordingHandlerand its now-unused imports.TestS3DirWatcher: allow any number ofListObjectsV2calls, wait up to 5s instead of 30ms (returns as soon as the condition holds), and lock thefilesmap that the watcher goroutine writes while the test reads it.Check List
Tests
Local:
golangci-lintv1.64.8 (make lintconfig) clean on the whole module (baserustreports the 11unusederrors);go test -race -count=20 -run 'TestS3DirWatcher$' ./pkg/sqlreplay/store/passes;go test ./pkg/controlbridge/...passes.Notable changes
Release note
🤖 Generated with Claude Code