Repository navigation
refactor(test): stop TestPartition feeding const initialisers (unblocks per-class isolation) - #1042
Merged
Merged
Conversation
Second-heaviest project after Monolith (shard-assign.sh weight 177 s; 1,138 tests,
39 classes carrying a ShareMeshAcrossTests opt-in).
threads=1 1138 tests, 0 failed, 198.2s
threads=4 1138 tests, 0 failed, 93.7s 2.11x
One config file, nothing else. Unlike Hosting.Monolith.Test this needed NO
scoping: the assembly has no GC-reachability assertions (no WeakReference +
forced GC.Collect), so there is nothing here whose precondition a busy heap
invalidates, and no shared Testcontainers instance of the kind that keeps
Hosting.PostgreSql.Test serial. Ran clean at 4 threads first try.
Note on the earlier reading: an AI.Test run this morning showed 1 failure at
threads=1, which I flagged as "no clean local baseline". That was the nuget.org
metadata flake this machine hit repeatedly today, not a defect — the baseline
above is green at threads=1 on the same machine.
The isolation this relies on already exists: MonolithMeshTestBase keys its
IServiceProvider cache by test-class Type, and xUnit's parallelism unit is the
collection (one per class by default), so concurrent classes already get
independent meshes. Opt-in stays per project — parallel safety is a property of
the tests, not the runner.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prerequisite ONLY — no behaviour change, no parallelism enabled, no measurable speedup. It removes a compile-time constraint that currently makes per-class test isolation impossible. `MonolithMeshTestBase.TestPartition` is a `public const string = "TestData"`, so EVERY test class in EVERY project writes into one partition — and a partition has a single owning hub that serialises every write routed through it. Harmless while tests run one class at a time; a contention point the moment they do not. That is the root cause behind the parallelism failures in #1033 / #1035 (see #1040): the failing assertions are waits that never emit, and the suspect class is green in isolation under identical CPU pressure. The fix is to make TestPartition per-class, i.e. `protected virtual`. That does not compile today, because a `const` can only be initialised from another `const`: private const string Collection = TestPartition + "/content"; private const string SubjectsNamespace = $"{TestPartition}/Subjects"; This commit converts those — and everything that transitively depended on them — to `static readonly`. For strings the two are semantically identical; `const` only adds compile-time inlining, which nothing here relies on. Bigger than it first looks: 19 direct dependants, but 36 files across 8 test projects once the const-on-const cascade is resolved (AI, ContentCollections. Indexing.Graph, Hosting.Blazor, Hosting.Monolith, Persistence, Query, Security, Threading). Resolved to a fixpoint rather than one compiler error at a time. Verified: all 8 affected projects build clean under CI's flags (-c Release -p:CIRun=true -warnaserror). The compiler is the completeness proof — any missed const-context usage is CS0133, not a silent behaviour change. Full behavioural verification is CI running the whole suite. Next (NOT in this commit): make TestPartition `protected virtual` with a per-class default, verify with `DOTNET_PROCESSOR_COUNT=4` (which reproduces the 4-vCPU runner locally in ~100 s — see #1040), then re-enable parallelism per project. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors test code to remove const-only initialization chains stemming from MonolithMeshTestBase.TestPartition, by converting dependent string constants to static readonly so TestPartition can later become per-class without triggering CS0133 cascades.
Changes:
- Convert many test-class
const stringpath/namespace fields tostatic readonly string(includingTestPartition-derived interpolations). - Add/adjust a project-local
xunit.runner.jsonforMeshWeaver.AI.Test(note: this may affect parallelism depending on prior project configuration). - Minor mechanical updates to other
conststring fields touched by the cascade.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Threading.Test/UpdateEmitsDuringRoundTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/ThreadResumeTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/SubThreadHangRepro.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/SubThreadColdStartRepro.cs | Convert base ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/OrphanedStartingExecutionRecoveryTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/LoadConversationHistoryTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/JsonPatchThreadMessagesTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/IsExecutingLifecycleTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/ExecuteThreadMessageTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/DelegationWriteCountTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/DelegationSubThreadUsageTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/DelegationFailureTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/DelegationExecutionTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/ChatHistoryTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/CancelThreadExecutionTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Threading.Test/AutoExecuteFlowTest.cs | Convert ContextPath constant to static readonly. |
| test/MeshWeaver.Security.Test/ThreadStreamingIdentityTest.cs | Convert UserPath constant to static readonly. |
| test/MeshWeaver.Query.Test/VirtualDataSourceContentTypeTest.cs | Convert TestPartition-derived paths/namespaces to static readonly. |
| test/MeshWeaver.Query.Test/SyncedQueryTest.cs | Convert SubjectsNamespace to static readonly. |
| test/MeshWeaver.Query.Test/SyncedQueryProjectionContractTest.cs | Convert Ns to static readonly. |
| test/MeshWeaver.Query.Test/SyncedQueryInitialGateTest.cs | Convert SubjectsNamespace/GatedQuery to static readonly. |
| test/MeshWeaver.Query.Test/SyncedQueryFreshnessContractTest.cs | Convert Ns to static readonly. |
| test/MeshWeaver.Query.Test/SyncedQueryDataSourceTest.cs | Convert SubscriberPath/SubjectsNamespace to static readonly. |
| test/MeshWeaver.Query.Test/SyncedQueryCrossSiloTest.cs | Convert Namespace to static readonly. |
| test/MeshWeaver.Query.Test/RemoteStreamCacheTest.cs | Convert TargetNamespace to static readonly. |
| test/MeshWeaver.Persistence.Test/LegacyUserPartitionRepairTest.cs | Convert raw-string JSON constant to static readonly. |
| test/MeshWeaver.Hosting.Monolith.Test/NodeTypeReleaseTest.cs | Convert NodeTypePath to static readonly. |
| test/MeshWeaver.Hosting.Monolith.Test/MeshNodeStreamCacheConcurrencyTest.cs | Convert Namespace to static readonly. |
| test/MeshWeaver.Hosting.Monolith.Test/MeshHubRemoteStreamTest.cs | Convert Namespace to static readonly. |
| test/MeshWeaver.Hosting.Monolith.Test/DeleteNodeBehaviorTest.cs | Convert Root to static readonly. |
| test/MeshWeaver.Hosting.Blazor.Test/StaticContentUnmountedTest.cs | Convert UserHeader to static readonly. |
| test/MeshWeaver.ContentCollections.Indexing.Graph.Test/MeshDocumentSinkTest.cs | Convert Collection to static readonly. |
| test/MeshWeaver.ContentCollections.Indexing.Graph.Test/DocumentBlocksAreaRenderTest.cs | Convert Collection to static readonly. |
| test/MeshWeaver.ContentCollections.Indexing.Graph.Test/ContentIndexingPipelineTest.cs | Convert Collection to static readonly. |
| test/MeshWeaver.AI.Test/xunit.runner.json | Introduces a project-local xUnit runner config (potentially affecting parallelism). |
| test/MeshWeaver.AI.Test/McpCompileInFlightTruthTest.cs | Convert RunningActivityPath to static readonly. |
| test/MeshWeaver.AI.Test/DocumentNodeGetTest.cs | Convert Collection to static readonly. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1
to
+6
| { | ||
| "parallelizeAssembly": false, | ||
| "parallelizeTestCollections": true, | ||
| "maxParallelThreads": 4, | ||
| "methodTimeout": 30000 | ||
| } |
Contributor
Test Results (shard 0)685 tests 685 ✅ 13m 14s ⏱️ Results for commit 4f8a944. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 4)1 284 tests 1 279 ✅ 6m 41s ⏱️ Results for commit 4f8a944. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 3) 11 files 11 suites 5m 52s ⏱️ Results for commit 4f8a944. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 1)1 199 tests 1 199 ✅ 6m 18s ⏱️ Results for commit 4f8a944. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 5)1 725 tests 1 533 ✅ 5m 52s ⏱️ Results for commit 4f8a944. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 2)1 921 tests 1 919 ✅ 6m 40s ⏱️ Results for commit 4f8a944. ♻️ This comment has been updated with latest results. |
Contributor
Test Results 65 files 65 suites 44m 39s ⏱️ Results for commit 4f8a944. ♻️ This comment has been updated with latest results. |
…n-unblock-per-class
It was untracked when I reverted with `git checkout -- .`, so the revert did not touch it, and `git add -A` then committed it. This PR is meant to be a pure prerequisite with NO behaviour change; enabling parallelism on AI.Test is #1035's job and is blocked on the very contention this PR unblocks fixing. It also explains both red runs, neither of which was the const conversion: run 1: CrossHubPatchAtomicityTest.ConcurrentCrossHubPatches_DoNotDropAQueuedMessage run 2: ThreadTokenUsageTest.CancelledRound_... / MultipleCompletedRounds_... all three AI.Test concurrency tests, all consistent with #1040's shared-partition contention — and none reachable by a const-to-static-readonly change.
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.
Prerequisite only — no behaviour change, no parallelism enabled, no measurable speedup. It removes a compile-time constraint that currently makes per-class test isolation impossible.
Why
MonolithMeshTestBase.TestPartitionis apublic const string = "TestData", so every test class in every project writes into one partition — and a partition has a single owning hub that serialises every write routed through it. Harmless while tests run one class at a time; a contention point the moment they do not.That is the root cause behind the parallelism failures in #1033 / #1035 — see #1040 for the evidence: the failing assertions are waits that never emit, and the suspect class is green in isolation under identical CPU pressure (
ThreadTokenUsageTest: 8/8 in 5.6s alone; fails alongside 1,130 neighbours).The fix is to make
TestPartitionper-class, i.e.protected virtual. That does not compile today, because aconstcan only be initialised from anotherconst:What this does
Converts those — and everything that transitively depended on them — to
static readonly. For strings the two are semantically identical;constonly adds compile-time inlining, which nothing here relies on.Bigger than it first appears: 19 direct dependants, but 36 files across 8 test projects once the const-on-const cascade is resolved (AI, ContentCollections.Indexing.Graph, Hosting.Blazor, Hosting.Monolith, Persistence, Query, Security, Threading). Resolved to a fixpoint rather than one compiler error at a time.
Verification
All 8 affected projects build clean under CI's flags (
-c Release -p:CIRun=true -warnaserror).The compiler is the completeness proof here: a missed const-context usage is
CS0133, a hard error — not a silent behaviour change. Behavioural verification is CI running the full suite, which is stronger than anything I could do locally.Next, deliberately NOT in this PR
TestPartitionprotected virtualwith a per-class defaultDOTNET_PROCESSOR_COUNT=4— reproduces the 4-vCPU runner locally in ~100s (AI.Test is not parallel-safe: cross-class shared state (repro in 100s via DOTNET_PROCESSOR_COUNT=4) #1040), instead of a 20-minute CI guessKeeping those separate so each is independently reviewable and revertible.
No What's New entry
Test infrastructure, no user-visible effect — the
pullrequestskill's step 0.5 says to skip and say so.