Fix flaky DrainDataAsync_Loop_ShouldFail (unblocks PR #9355 CI)

[Amaury Levé (Evangelink)]

Problem

The CI build for #9355 failed on the Build Windows Debug leg, but the failure is unrelated to that PR's changes (its analyzer tests all pass). The failing test is:

failed DrainDataAsync_Loop_ShouldFail (179ms)
Assertion failed. Expected exception of exact type InvalidOperationException but no exception was thrown.
from Microsoft.Testing.Platform.UnitTests.exe (net48|x64)

This is a known, intermittently-flaky test (historically tracked by #6892) and only fails on net462 under heavy CI load.

Root cause

DrainDataAsync_Loop_ShouldFail sets up two consumers that produce messages each other reacts to, creating a publisher/consumer loop. Detection relies on the background consumer tasks (started via ITask.Run) ping-ponging messages so that AsynchronousMessageBus.DrainDataAsync keeps observing newly received payloads across its drain rounds and ultimately throws.

On .NET Framework, under the assembly's method-level parallelism (32 workers competing for the slowly-growing ThreadPool), those background tasks can be starved long enough that the drain completes its attempts without observing the loop — so no exception is thrown and the assertion fails. The drain/marker logic itself is deterministic; the flakiness comes purely from ThreadPool contention with the other concurrently-running test methods.

Fix

Mark the single test [DoNotParallelize] (the same pattern already used by TerminalOutputDeviceTests) so it runs without competing for threads, keeping loop detection deterministic. This is a test-only change with no production impact.

Verified locally on net462/net8.0/net9.0: the project builds clean and all 5 AsynchronousMessageBusTests pass.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

This PR addresses intermittent flakiness in the Microsoft.Testing.Platform unit test suite on .NET Framework by ensuring a specific loop-detection test does not compete with other test methods for ThreadPool resources under method-level parallel execution.

Changes:

• Added an explanatory comment documenting why DrainDataAsync_Loop_ShouldFail can flake under .NET Framework ThreadPool contention.
• Marked DrainDataAsync_Loop_ShouldFail with [DoNotParallelize] to eliminate cross-test contention and stabilize loop detection.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Messages/AsynchronousMessageBusTests.cs	Documents the known flake cause and opts the single loop-detection test out of method-level parallel execution via [DoNotParallelize].

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 0

[Amaury Levé (Evangelink)]

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Expert Review — PR #9366 · Fix flaky DrainDataAsync_Loop_ShouldFail

Reviewed against all 22 dimensions. Changed file lives under test/UnitTests/ → priority dimensions: Test Isolation, Assertion Quality, Flakiness Patterns, Threading & Concurrency, Code Structure.

---

Verdict table

#	Dimension	Result	Notes
1	Algorithmic Correctness	✅ LGTM	Fix addresses root cause (ThreadPool starvation), not a symptom
2	Threading & Concurrency	✅ LGTM	Change itself introduces no new shared-state; see observation below
3	Security & IPC Contract Safety	➖ N/A	Test-only change
4	Public API & Binary Compatibility	➖ N/A	No public surface touched
5	Performance & Allocations	➖ N/A	Test code
6	Cross-TFM Compatibility	✅ LGTM	[DoNotParallelize] available on all supported MSTest TFMs; fix specifically targets the net462/net48 regression path
7	Resource & IDisposable Management	✅ LGTM	Pre-existing (no using on asynchronousMessageBus in the loop test); not introduced here
8	Defensive Coding at Boundaries	➖ N/A
9	Localization & Resources	➖ N/A
10	Test Isolation	✅ LGTM	Method-level [DoNotParallelize] is the correct, minimal scope — more precise than the class-level usage in TerminalOutputDeviceTests; isolates only this test
11	Assertion Quality	✅ LGTM	Assert.ThrowsExactlyAsync<InvalidOperationException> + Assert.Contains on message content are appropriate; MSTest Assert family is the correct library for this project
12	Flakiness Patterns	✅ LGTM	PR removes the flakiness trigger; no Task.Delay/Thread.Sleep-for-sync patterns introduced
13	Test Completeness & Coverage	✅ LGTM	Fix preserves existing test intent; no new branches to cover
14	Data-Driven Test Coverage	➖ N/A
15	Code Structure & Simplification	✅ LGTM	Change is minimal and coherent
16	Naming & Conventions	✅ LGTM
17	Documentation Accuracy	✅ LGTM	Comment explains why (not just what), references tracking issue #6892 — exactly the right level of detail
18	Analyzer & Code Fix Quality	➖ N/A
19	IPC Wire Compatibility	➖ N/A
20	Build Infrastructure & Dependencies	➖ N/A
21	Scope & PR Discipline	✅ LGTM	Single concern, references #6892 and blocked PR #9355, no unrelated changes
22	PowerShell Scripting Hygiene	➖ N/A

Clean dimensions: 22/22 (12 applicable, all LGTM; 10 N/A)

---

Pre-existing observation (not introduced by this PR, no action required)

LoopConsumerA._stopConsume and ConsumerB._stopConsume are plain bool fields written by the main test thread (StopConsume()) and read by background ITask.Run tasks (ConsumeAsync). Per dimension §2, cross-thread fields without volatile/Interlocked/lock are technically a data race. In this specific test the write happens only after the InvalidOperationException has already been caught (so the race cannot cause the assertion to fail), making it inconsequential in practice. Worth a volatile annotation in a future cleanup, but does not affect correctness here and is out of scope for this PR.

---

Overall: The fix is correct, minimal, and well-justified. The approach exactly mirrors the TerminalOutputDeviceTests precedent and directly eliminates the starvation condition that triggered the intermittent failure. Ready to merge.

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9366

Δ	Test	Grade	Band	Notes
mod	AsynchronousMessageBusTests. DrainDataAsync_ Loop_ ShouldFail	A	90–100	Exact exception type + message content assertions; clear AAA structure; [DoNotParallelize] addition is well-motivated by flakiness concern.

_{This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.}

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 162.4 AIC · ⌖ 13.7 AIC · ⊞ 43.7K · [◷]( · ◷)