Fix unit stress flakes in JumpList and QueryCache threading tests#394
Merged
Conversation
Two flakes hit in the 1000x unit stress run:
* JumpListUpdateValidationTests.UpdateAsync_Separator_With_Empty_Title_Is_Allowed
raced on static JumpList.AppUserModelId with JumpListStateTests, which
also mutates the same statics and calls ResetForTests(). Without a
shared collection, xUnit runs the two classes in parallel; a concurrent
ResetForTests() clears AppUserModelId between the test's setter and its
UpdateAsync call, tripping the unpackaged-AppUserModelId guard.
Putting both classes in a [Collection("JumpListGlobals")] serializes
them, matching the pattern this repo already uses for PersistedStateCache,
UnobservedTaskException, and DockingGlobals.
* QueryCacheThreadingTests.Concurrent_Subscribe_Unsubscribe_Converges_To_Zero
hit the 60s DrainTimeout once. The workload is pure lock-bound
bookkeeping; eviction cannot fire in 60s given the 5-minute default
CacheTime, so this is scheduler-induced rather than a deadlock. Bump
the drain ceiling to 120s without weakening the deadlock guarantee.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR targets rare CI stress flakes in the Reactor test suite by improving test isolation for JumpList global state and by loosening an overly tight timeout in a threading stress test.
Changes:
- Serialize JumpList tests that mutate
JumpListstatic state by placing them in the same xUnit collection (JumpListGlobals). - Increase
QueryCacheThreadingTests’ global drain timeout from 60s to 120s to reduce scheduler-noise flakes without changing the test’s deadlock-detection intent.
Show a summary per file
| File | Description |
|---|---|
| tests/Reactor.Tests/JumpListUpdateValidationTests.cs | Adds JumpListGlobals collection usage and documents why JumpList tests must be serialized. |
| tests/Reactor.Tests/JumpListItemTests.cs | Places JumpListStateTests into the same JumpListGlobals collection to prevent concurrent static resets. |
| tests/Reactor.Tests/Core/QueryCacheThreadingTests.cs | Bumps DrainTimeout from 60s to 120s and expands rationale in comments. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
Comment on lines
+18
to
22
| /// <c>ResetForTests()</c> in another class. | ||
| /// </summary> | ||
| [Collection("JumpListGlobals")] | ||
| public class JumpListUpdateValidationTests : IDisposable | ||
| { |
Comment on lines
182
to
186
| /// Spec 036 §11.3 — JumpList static state. We don't exercise the live shell | ||
| /// here (that's a selftest fixture); only the public state surface. | ||
| /// </summary> | ||
| [Collection("JumpListGlobals")] | ||
| public class JumpListStateTests |
Addresses CR feedback: the other shared-state collections in this repo (ConsoleTests, UnobservedTaskException, PersistedStateCache, DockingGlobals) all have explicit [CollectionDefinition(..., DisableParallelization = true)] markers. Match the convention with a JumpListGlobalsCollection marker so the serialization intent is grep-able and the collection is also serialized against unrelated collections that may transitively touch the same statics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Two 1-in-1000 unit-stress flakes surfaced in the latest CI Stress run (#26345012652):
JumpListUpdateValidationTests.UpdateAsync_Separator_With_Empty_Title_Is_Allowed— real isolation bug. StaticJumpList.AppUserModelIdis mutated by both this class andJumpListStateTests, and both callJumpList.ResetForTests()(which clears the static). xUnit's default parallelizes test classes, so a concurrentResetForTests()fromJumpListStateTestscould wipe the AumiD between this test's setter (= "Test.App") and itsUpdateAsynccall, tripping the unpackaged-AumiD guard and throwingInvalidOperationException. Fix: put both classes into[Collection(\"JumpListGlobals\")], matching the pattern already used forPersistedStateCache,UnobservedTaskException, andDockingGlobals.QueryCacheThreadingTests.Concurrent_Subscribe_Unsubscribe_Converges_To_Zero— environmental. Hit the 60sDrainTimeout. Static analysis: the workload is 8 threads × 1000 lock-bound bookkeeping ops on one slot; eviction physically cannot fire within 60s given the 5-minute defaultCacheTime, so this is scheduler-induced (ThreadPool ramp-up / hosted-VM noise), not a deadlock. Bump the drain ceiling to 120s without weakening the deadlock guarantee.Test plan
dotnet buildcleandotnet test --filter ~JumpList— 32/32 pass🤖 Generated with Claude Code