fix(test): stop wall-clock and mock races failing CI on macOS and Windows - #1382
Merged
Conversation
TestStreamingSource_PrefetchFillsRingWithoutTickerPacing proved prefetch burst-fills its ring by giving it 2 s to do so: burst-filling lands well inside that, one chunk per 100 ms tick would need 4 s. But the same 2 s was also the budget for decoding and resampling 6 s of 44.1 kHz audio, in a parallel test on a shared runner, so a loaded macOS job failed it with prefetch behaving correctly. That is how 4023de5 went red on main. streamingSource now carries the prefetch wake-up interval as a field and the test stretches it to an hour. Reaching a full ring then requires a goroutine that keeps decoding until the ring is full, which is the property under test, and no machine is slow enough to fail it. Confirmed by reinstating per-tick pacing: the test still catches it. The deadline survives only as a hang guard. A zero interval falls back to the 100 ms default, since newTestSource builds a bare streamingSource and time.NewTicker(0) panics. ConcurrentPrefetchAndMix waits for the same fill on the same 2 s deadline. There it is a precondition rather than the subject, so it just gets a deadline it cannot lose.
TestChunkedStepTiming_PacingIsNotBilledAsSQL asserted that timing.bounds was positive, which asks whether a small SQLite query took a measurable amount of time. Windows' timer ticks at ~15.6 ms and the query finishes well inside one tick, so it read back as exactly 0s and failed there while passing on Linux and macOS, whose clocks are finer. The test now wraps the DB so the bounds lookup, the only QueryRowContext sqlUpsertStagedMedia issues, sleeps 40 ms. Two claims replace positivity: the lookup runs once per chunk, counted rather than timed, and the injected delay lands inside timing.bounds, so the timer really does wrap the bounds statement. The requirement stays at one delay rather than one per call, because a coarse clock can read each measured interval up to a tick short and scaling by the call count would put the granularity back in. Confirmed by dropping the accumulation in sql_scan_reconcile.go: the test fails. The removed sqlTime assertion was tautological once bounds is non-negative. The attribution it was reaching for, recordChunkedStep subtracting bounds and pacing from the step, is covered by TestReconcileStepTimingsLine_AccountsForWallTime.
TestHandleGenerateMedia_SystemFiltering runs real indexing against a mocked MediaDB and then cancels it. Cancellation normally wins the race, but when the indexing goroutine gets further it calls ListMediaUserData on a MockUserDBI carrying no expectations at all, and testify's "I don't know what to return" panic takes down the whole pkg/api/methods binary rather than failing this test. That is what happened on macOS in 06521be. ListMediaUserData is the only UserDB method the indexing path calls, but letting the goroutine run to completion showed that the next call it reaches, RunBackgroundOptimizationWithLease, is unmocked too, so covering one alone would have moved the panic rather than removed it. Both are expectations now, as they already are in mediascanner's own tests, and the test passes with the goroutine running to the end.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesReliability fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change makes flaky audio, database-timing, and mock-based tests deterministic while preserving production behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR satisfies the coding objectives in [ Full details: Out of Scope Changes checkExplanation All changes support the linked issue [
✨ Finishing Touches 💡 1📝 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1379.
Three tests fail on CI for reasons unrelated to the product, and between them they make main's status close to meaningless: of the last 25
Lint and Testruns on main, one succeeded. Because the macOS and Windows jobs only run on pushes to main, none of this is visible on a pull request.macOS,
TestStreamingSource_PrefetchFillsRingWithoutTickerPacing. The test proved prefetch burst-fills its ring by giving it 2 s, since per-tick pacing would need 4 s. That 2 s was also the budget for decoding and resampling 6 s of 44.1 kHz audio in a parallel test on a shared runner, so a loaded macOS job failed it with prefetch behaving correctly — that is how4023de5bwent red.streamingSourcenow carries the prefetch wake-up interval as a field and the test stretches it to an hour, so a full ring can only be the result of a burst fill and runner speed cannot decide the outcome.ConcurrentPrefetchAndMixwaits for the same fill on the same 2 s deadline as a precondition; it gets a deadline it cannot lose.Windows,
TestChunkedStepTiming_PacingIsNotBilledAsSQL.assert.Positive(t, timing.bounds)asks whether a small SQLite query took a measurable amount of time. Windows' timer ticks at ~15.6 ms and the query finishes inside one tick, so it read back as exactly0s. The test now wraps the DB so the bounds lookup sleeps 40 ms, and asserts the two things actually claimed: the lookup runs once per chunk (counted, not timed) and the injected delay lands intiming.bounds.macOS,
TestHandleGenerateMedia_SystemFiltering. Separate flake found while confirming the other two. The test runs real indexing against a mocked MediaDB and cancels it; when the goroutine outruns cancellation it callsListMediaUserDataon a mock with no expectations, and testify's panic takes down the wholepkg/api/methodsbinary. Letting the goroutine run to completion showedRunBackgroundOptimizationWithLeaseis unmocked too, so covering only the first call would have moved the panic rather than removed it.Both wall-clock fixes were checked against injected regressions rather than trusted: reinstating per-tick pacing fails the audio test, and dropping
timing.bounds += time.Since(...)fails the bounds test. The mock panic reproduces exactly as it did on CI when the expectation is removed and cancellation delayed.macOS and Windows are not reachable locally and a pull request will not run them either, so the injected-regression checks stand in for that; the real verdict lands on the post-merge run on main.
The
concurrencyblock noted at the bottom of #1379 is deliberately not touched here — cancelling superseded runs is intended behaviour.Summary by CodeRabbit