Skip to content

test(931): remove scheduler and file-handle dependence from breadcrumb thread-affinity and FileInfoWrapper tests - #939

Merged
drmoisan merged 17 commits into
mainfrom
bug/tests-depend-on-uncontrolled-environment-931
Sep 30, 2026
Merged

drmoisan merged 17 commits into
mainfrom
bug/tests-depend-on-uncontrolled-environment-931

Conversation

@drmoisan

Copy link
Copy Markdown
Owner

Summary

  • Removes the scheduler dependence from two thread-affinity guard tests in QuickFiler.Test. Each test now runs its guarded call on a dedicated, joined Thread and asserts inside that thread that it is not the owner thread, instead of relying on Task.Run to provide a different thread.
  • Removes the file-handle dependence from FileInfoWrapper_Tests in UtilitiesCS.Test. The tests no longer locate or open the repository's TaskMaster.sln; OpenRead is exercised through the existing internal FileInfoWrapper(IFileInfo) seam with a test-owned FileStream, and the three metadata tests use a rooted literal path.
  • Adds one shared internal test helper, QuickFiler.Test.TestSupport.DedicatedWorkerThread.Run(Action), and splits ItemViewerBreadcrumbThreadAffinityTests into two partial files so each stays under the 500-line limit.
  • Test-only change: no production file, no runsettings file, and no .claude or config file is modified. The parallel regime (Workers zero, Scope ClassLevel) is unchanged.
  • Each rewritten guard test was observed failing against a deliberately broken guard and passing after the revert (four negative controls, committed as Markdown projections).

Why

Both defects share one root cause: a unit test depended on environment state it does not control.

  • Distinct-thread defect. Task.Run guarantees a thread-pool work item, not a thread different from the caller. Under the parallel run every test executes on a pool thread, and a blocking wait on a not-yet-started task can execute the work item inline on the waiting thread. A guard that decides by thread identity then either admits the call (the test passes without exercising the guard) or produces a spurious failure. Of the 22 Task.Run sites in QuickFiler.Test, the research triage classified exactly two as affected: BreadcrumbPopupBoundaryCoverageTests.Dispatcher_OwnerOnlyWorker_ReportsWithoutRunningAction and ItemViewerBreadcrumbThreadAffinityTests.InitializeBreadcrumbPipeline_NullOwningDispatcher_DoesNotThrow. The other 20 reach guards that decide by SynchronizationContext reference or only complete a TaskCompletionSource, so they are left unchanged.
  • File-handle defect. OpenRead_ShouldReturnReadableStreamForWrappedFile opened TaskMaster.sln with the default FileShare.Read. Any other process holding that file with a share mode excluding readers (for example a resident MSBuild node) made the test throw IOException, so its outcome depended on build history rather than on the wrapper.

The fix does not serialise the run: no [DoNotParallelize], Workers change, retry, sleep, timeout or wall-clock wait is introduced.

What Changed

Tests (QuickFiler.Test):

  • TestSupport/DedicatedWorkerThread.cs (new): internal static Exception Run(Action action) starts a background Thread, joins it without a timeout, and returns the exception the delegate threw or null. It carries no assertion; each test states its own distinctness precondition.
  • Viewers/BreadcrumbPopupBoundaryCoverageTests.cs: Dispatcher_OwnerOnlyWorker_ReportsWithoutRunningAction captures the owner thread id, runs Dispatch through DedicatedWorkerThread.Run, and asserts in-thread that Environment.CurrentManagedThreadId differs from the owner id. The two existing assertions are unchanged.
  • Viewers/ItemViewerBreadcrumbThreadAffinityTests.cs and Viewers/ItemViewerBreadcrumbThreadAffinityTests.Part2.cs (new): the class is now partial. The three cross-thread tests and ClearViewerDispatcher move to Part2 and call the shared helper; the private RunOnDedicatedWorkerThread is removed. InitializeBreadcrumbPipeline_NullOwningDispatcher_DoesNotThrow captures the owning Dispatcher and asserts CheckAccess() is false inside the dedicated thread. No test is renamed or removed.
  • QuickFiler.Test.csproj: two Compile Include entries for the new files.

Tests (UtilitiesCS.Test):

  • HelperClasses/FileInfoWrapper_Tests.cs: GetSolutionFile() and every TaskMaster.sln reference removed. OpenRead_ShouldReturnReadableStreamForWrappedFile uses a strict Mock<IFileInfo> returning a FileStream opened over the test assembly's own location with FileAccess.Read and FileShare.ReadWrite, and asserts same-instance identity, CanRead, and Length > 0. The three metadata tests use the rooted literal C:\Repo\fixture.sln. No file is created, written or deleted.

Docs:

  • Feature folder docs/features/active/2026-09-28-tests-depend-on-uncontrolled-environment-931/: research, spec (19 acceptance criteria), atomic plan, baseline, regression-testing and QA-gate evidence projections, and the policy-audit, code-review and feature-audit artifacts.
  • docs/features/potential/promoted/2026-09-28-tests-depend-on-uncontrolled-environment.md: promotion record written by the promotion lifecycle.

Merge: origin/main (c4ff0e2) was merged at 55a50e9 with no conflicts. The only item-scope file it touched is QuickFiler.Test.csproj, and only its Analyzer Include version paths (taken from main).

Architecture / How It Fits Together

  • The two rewritten QuickFiler tests exercise the thread-identity branch of BreadcrumbUiDispatcher.IsCurrentBoundary() and the null-owner escape in ItemViewer's UI-boundary guard. A Thread constructed by the test is distinct from every live thread by construction, so the in-thread precondition holds under any scheduler, and an untimed Join() does not park a pool slot waiting on another pool slot.
  • FileInfoWrapper's contract for OpenRead is pure delegation, so the test verifies it through the internal seam (reachable via the existing InternalsVisibleTo("UtilitiesCS.Test")). The public constructor path and the explicit DirectoryInfoWrapper cast remain covered by the three metadata tests.

Verification

Completed (evidence committed under the feature folder):

  • Negative controls, each observed failing with the predicted message and then passing after revert, with porcelain status proving no residual change: owner-only dispatcher guard forced true; null-owner escape replaced by the pre-fix context-reference throw; action() inserted as the first statement of DedicatedWorkerThread.Run (all four dedicated-thread tests fail at their precondition); OpenRead mock pointed at a second stream.
  • Full toolchain at the pre-merge head, one clean pass: dotnet tool run csharpier check . exit 0; analyzer msbuild ... /t:Rebuild ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true exit 0; msbuild ... /t:Rebuild ... /p:TreatWarningsAsErrors=true exit 0; zero "Skipping target CoreCompile" lines in both rebuilds; MSTest-with-coverage route exit 0.
  • Post-merge re-run at 55a50e9 of every gate: csharpier check exit 0 (1625 files); analyzer and nullable rebuilds exit 0, 0 errors, 0 warnings; QuickFiler.Test 1469 run, 0 failed; UtilitiesCS.Test 4924 run, 0 failed (local shell-icon stall classes excluded by the recorded filter; CI runs them); coverage run 7323 tests, 0 failed.
  • Coverage: first-party 85.31% lines and 79.73% branches post-merge (85.32% / 79.73% pre-merge); both floors met. Pre-merge per-package UtilitiesCS and QuickFiler figures are not lower than baseline.
  • Feature review: policy-audit, code-review and feature-audit each report 0 blocking findings; 19 of 19 acceptance criteria verified.

Recommended:

  • Required CI checks on this pull request's head.

Backward Compatibility / Migration Notes

  • None. Test-only change; no public API or configuration change.

Risks and Mitigations

  • A new test file not registered in the project would silently drop its tests from the run. Mitigation: the committed QuickFiler.Test suite projection lists all four dedicated-thread test names as executed and passed.
  • Post-merge, the UtilitiesCS package line rate reads 0.893789 against the pre-merge baseline 0.893884. The per-file analysis in post-merge-coverage-final.md attributes this to production edits already on main (ILGlobals.cs, UiThread.cs) plus run-to-run collector variance in unchanged files; this change modifies no production line. Recorded as a non-blocking disclosure in the audits.
  • Rollback: revert the merge commit of this pull request to restore the previous tests.

Review Guide

  1. QuickFiler.Test/TestSupport/DedicatedWorkerThread.cs (48 lines).
  2. QuickFiler.Test/Viewers/BreadcrumbPopupBoundaryCoverageTests.cs (one test body).
  3. QuickFiler.Test/Viewers/ItemViewerBreadcrumbThreadAffinityTests.cs and .Part2.cs: mostly a mechanical move of three tests and one helper; review the rewritten InitializeBreadcrumbPipeline_NullOwningDispatcher_DoesNotThrow body and remark.
  4. UtilitiesCS.Test/HelperClasses/FileInfoWrapper_Tests.cs (four test bodies).
  5. QuickFiler.Test/QuickFiler.Test.csproj (two Compile entries).
  6. Evidence and audits under the feature folder; the plan and research files are large and can be skimmed.

Follow-ups

Deferred, not fixed here. They are not filed from this branch because the promotion record would fall outside this change's footprint criterion; they are recorded in evidence/qa-gates/p4-t13-follow-up-handoff.2026-09-29T09-46.md and in the spec's Rollout and Follow-up section for filing from a separate branch:

  1. UtilitiesCS.Test/HelperClasses/PhysicalFileSystemAdapters_Tests.cs locates and opens the repository solution file and swallows IOException; same defect class.
  2. UtilitiesCS.Test/HelperClasses/DirectoryInfoWrapper_Tests.cs asserts that TaskMaster.sln is enumerated from the repository root; same defect class.
  3. QuickFiler.Test/Viewers/BreadcrumbUiThreadDispatchTests.cs: the asserted message wording "cannot marshal cross-thread UI work" is broader than the DispatchValue mechanism (wording only).
  4. A documentation correction in the earlier breadcrumb thread-affinity follow-up handoff record, which attributes one DispatchValue site to the owner-thread-id check.

Also noted by the review: quality-tiers.yml is absent at the repository root (pre-existing).

GitHub Auto-close

🤖 Generated with Claude Code

drmoisan and others added 17 commits September 28, 2026 20:02
…an active full-bug feature folder

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KoweznWqwJTkNCf6756FoF
…rapper fixture

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KoweznWqwJTkNCf6756FoF
…nd acceptance criteria

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KoweznWqwJTkNCf6756FoF
…eInfoWrapper test fixes

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
… test fix

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
…b affinity and FileInfoWrapper tests

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
…ntrolled-environment test fix

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
…ironment-931

Brings in the analyzer-path hotfix 89e202e and the merged sibling items. The only item-scope file touched is QuickFiler.Test/QuickFiler.Test.csproj, and only its Analyzer Include version paths changed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Policy audit, code review and feature audit at 2026-09-29T20-15 against head ce744bb. All 19 acceptance criteria verified; no remediation cycle required.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s with a placeholder

The reviewer's final pass rewrote one line in each of the policy and feature audits after they were committed, so the committed evidence carried the account name. No figure or verdict changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant