test: Remove timing dependencies from worker tests - #1674
Merged
Conversation
Four tests in JobWorkerBaseTest raced the worker instead of synchronising with
it, which made them fail on a loaded CI runner.
AddFailedProgressAsync_ShouldAddFailedProgres let the process function sleep
100ms and called AddFailedProgressAsync afterwards. If the test thread was not
scheduled within that window, HandleJobRunAsync had already completed and set
_jobRepository to null, so the call threw:
System.InvalidOperationException : Unable to set job items because no job
repository is set.
Under 2x CPU load this test failed 15 out of 15 runs. Its two siblings
(SetTotalItemsAsync/AddProgressAsync) were already converted to a
TaskCompletionSource handshake; this applies the same pattern to the one that
was missed.
The three AddInitialJob tests waited a fixed 10ms ("Give some time to add job")
before asserting. AddInitialJob_ShouldAddAnInitialJob_IfConfigured failed 4 out
of 6 runs under load. They now wait on TestWorker.JobRunCompleted, which
completes when the worker's first iteration is actually finished. That also
removes a false-negative risk in the ShouldNotAddInitialJob case, which could
previously pass simply because the worker had not got there yet.
Un-skips SetTotalItemsAsync_ShouldThrowInvalidOperationException_WhenNoCurrent
JobIsSet, which was marked "Find a way to setup state for this without repo
being nulled after a run". Deleting old jobs runs while the repository is still
set but the current job has been reset, so blocking inside DeleteJobsAsync holds
the worker in exactly that state. Verified by mutation: without the wait the
test fails on the "no job repository is set" message instead.
After the change the whole test project passed 20 out of 20 runs under 2x load.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015BX2NY9ozmWKHsifii1KYP
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
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.
Follow-up to #1673. Fixes the flaky test that failed on #1667, plus three more with the same defect found while verifying.
Note
The
coveragejob on this PR will stay red until #1673 is merged — that's the unrelated coveralls timezone crash, not this change.The flake from CI
AddFailedProgressAsync_ShouldAddFailedProgreslet the process function sleep 100 ms, then calledAddFailedProgressAsyncafterStartAsyncreturned:StartAsyncreturns as soon asExecuteAsyncsuspends — which is inside thatTask.Delay(100). If the test thread isn't scheduled again within 100 ms,HandleJobRunAsyncfinishes and sets_jobRepository = null(JobWorkerBase.cs:360), so the call throws:Reproduced deterministically by inserting a 300 ms delay after
StartAsync— byte-identical to the CI failure.The two sibling tests (
SetTotalItemsAsync_ShouldSetTotalItems,AddProgressAsync_ShouldAddProgress) had already been converted to aTaskCompletionSourcehandshake. This one was missed; it now uses the same pattern.Three more with the same defect
The
AddInitialJobtests waited a fixedTask.Delay(10)— "Give some time to add job" — before asserting. They now wait on a newTestWorker.JobRunCompleted, which completes when the worker's first iteration has actually finished.This also removes a false negative:
AddInitialJob_ShouldNotAddInitialJob_IfThereIsAnyInDueRangeassertsAddJobAsyncwas never called, which previously could pass simply because the worker hadn't reached that code yet. Waiting for the iteration to complete makes the assertion meaningful.I used the
PostJobRunHookAsyncseam rather than a callback on theAddJobAsyncmock: a mock callback can't work for the negative test (you can't wait for a call that must never happen), and one uniform mechanism beats two.Un-skipped test
SetTotalItemsAsync_ShouldThrowInvalidOperationException_WhenNoCurrentJobIsSetwas marked:It needs
_jobRepository != null && _currentJob == null. That window exists:DeleteOldJobsruns after_currentJobis reset but before_jobRepositoryis nulled. Blocking inside theDeleteJobsAsyncmock holds the worker there deterministically.Verified it isn't passing vacuously — with the wait removed it fails on the other branch:
Evidence
All soaks on 16 cores with 2× busy-loop load:
AddFailedProgressAsync_ShouldAddFailedProgresAddInitialJob_ShouldAddAnInitialJob_IfConfiguredJobService.TestprojectFull solution green 3× (
17 + 17 + 19passed, 0 skipped — was 1 skipped).No production code changed;
TestWorkergains one test-only seam.🤖 Generated with Claude Code
https://claude.ai/code/session_015BX2NY9ozmWKHsifii1KYP