Fix [ClassCleanup] leak when ITestFilter drops the last test of an initialized class

[Amaury Levé (Evangelink)]

Summary

Targets dev/amauryleve/custom-test-filter (#8896). Fixes a [ClassCleanup] leak in the new ITestFilter drop path, reported by an internal team using per-worker test sharding (each worker enumerates the full set per class, runs its subset, and Drops the rest).

The bug

UnitTestRunner.FinishFilteredOutTestAsync assumed a filtered-out test never loaded its type and therefore skipped class cleanup, only calling MarkClassComplete. That assumption is false when a sibling test of the same class already ran in this worker: [ClassInitialize] executed and [ClassCleanup] is still owed.

Because ClassCleanupManager counts down over the full (pre-filter) test set, isLastTestInClass is independent of run-vs-dropped. So whenever a class's last-in-order test is dropped — while earlier tests of that class ran — the worker leaks the class cleanup. ForceCleanup is not a safety net (it only fires on IsGracefulStopRequested).

The analogous assembly-cleanup asymmetry was already guarded by _assemblyInitializeWasExecuted, which is why only class cleanup leaked.

The fix

When the dropped test is the last in its class, run [ClassCleanup] if the class was initialized in this worker. Detection reuses the existing _lastRunnableTestByClass map, which is populated only for classes that both have an executable cleanup method and ran a non-filtered test. The TestClassInfo is already cached in the TypeCache, so resolving it loads no new type — preserving the feature's "a Drop pays zero type-load / init cost" guarantee. A fully-dropped class (type never loaded) still correctly skips both init and cleanup.

Validation

• Added acceptance test TestFilterClassCleanupTests reproducing the leak: a filter drops the last-in-order test of an initialized class and the only test of a fully-dropped class.
  • Against the unfixed adapter the test fails — output shows ClassInitialize + the run test but no ClassCleanup, exactly the reported symptom.
  • Against the fixed adapter it passes; the fully-dropped class runs neither init nor cleanup.
• MSTestAdapter.PlatformServices.UnitTests green across net462/net48/net8.0/net9.0/net8.0-windows.

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

[Copilot]

Pull request overview

Fixes a regression in the MSTest adapter’s new ITestFilter “drop/skip before type-load” path where [ClassCleanup] could be skipped when the last-in-order test of an already-initialized class was filtered out (common in per-worker sharding scenarios).

Changes:

• Update UnitTestRunner.FinishFilteredOutTestAsync to execute [ClassCleanup] when the filtered-out test is the last test in the class and the class was previously initialized in the current worker.
• Add an acceptance test that reproduces the leak (drop last-in-order test of a partially-run class) and verifies no over-correction for fully-dropped classes.

Show a summary per file

File	Description
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestFilterClassCleanupTests.cs	Adds an acceptance regression test covering the “last-in-order test dropped” class-cleanup leak and the “fully dropped class” non-init/non-cleanup guard.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs	Runs class cleanup from the filter drop/skip tail path when the class was initialized earlier in the worker and the dropped/skipped test is the last test in its class.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9503

Δ	Test	Grade	Band	Notes
new	TestFilterClassCleanupTests. ClassCleanup_ RunsWhenLastTestOfInitializedClassIsDropped	A	90–100	Rich assertion set covers both the positive fix (ClassCleanup runs for the initialized class) and the guard against over-correction (ClassCleanup does not run for the fully-dropped class); clear AAA structure with informative comments.

_{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. · 179.3 AIC · ⌖ 12.7 AIC · ⊞ 43.7K · [◷]( · ◷)

[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.

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
16	Naming & Conventions	💬 1 NIT
17	Documentation Accuracy	💬 1 NIT

✅ 19/22 dimensions clean.

• Test Completeness — the cleanupResult is not null branch (cleanup fails in the drop path) has no acceptance-test coverage; AssociatedUnitTestElement attribution is unvalidated
• Naming — the Test_A_Run / Test_Z_Dropped alphabetical-ordering assumption is undocumented
• Documentation — successful [ClassCleanup] output is silently discarded when filterResult is empty; no comment explains this

---

Overall assessment: The fix is algorithmically correct and well-reasoned. The _lastRunnableTestByClass sentinel is the right signal: it's populated only when both conditions are true — the class has an executable cleanup method and at least one non-filtered test ran in this worker — so a fully-dropped class is still correctly skipped. The ExecuteClassCleanupAsync semaphore and IsClassCleanupExecuted flag in TestClassInfo prevent any risk of double execution even under concurrent test scheduling. Thread safety is maintained via ConcurrentDictionary.TryGetValue. The ConfigureAwait(false) and try/finally disposal patterns mirror the surrounding code correctly. The three items above are polish suggestions; none blocks the merge.

[Amaury Levé (Evangelink)]

Summary

Targets dev/amauryleve/custom-test-filter (#8896). Fixes a [ClassCleanup] leak in the new ITestFilter drop path, reported by an internal team using per-worker test sharding (each worker enumerates the full set per class, runs its subset, and Drops the rest).

The bug

UnitTestRunner.FinishFilteredOutTestAsync assumed a filtered-out test never loaded its type and therefore skipped class cleanup, only calling MarkClassComplete. That assumption is false when a sibling test of the same class already ran in this worker: [ClassInitialize] executed and [ClassCleanup] is still owed.

Because ClassCleanupManager counts down over the full (pre-filter) test set, isLastTestInClass is independent of run-vs-dropped. So whenever a class's last-in-order test is dropped — while earlier tests of that class ran — the worker leaks the class cleanup. ForceCleanup is not a safety net (it only fires on IsGracefulStopRequested).

The analogous assembly-cleanup asymmetry was already guarded by _assemblyInitializeWasExecuted, which is why only class cleanup leaked.

The fix

When the dropped test is the last in its class, run [ClassCleanup] if the class was initialized in this worker. Detection reuses the existing _lastRunnableTestByClass map, which is populated only for classes that both have an executable cleanup method and ran a non-filtered test. The TestClassInfo is already cached in the TypeCache, so resolving it loads no new type — preserving the feature's "a Drop pays zero type-load / init cost" guarantee. A fully-dropped class (type never loaded) still correctly skips both init and cleanup.

Validation

• Added acceptance test TestFilterClassCleanupTests reproducing the leak: a filter drops the last-in-order test of an initialized class and the only test of a fully-dropped class.
  • Against the unfixed adapter the test fails — output shows ClassInitialize + the run test but no ClassCleanup, exactly the reported symptom.
  • Against the fixed adapter it passes; the fully-dropped class runs neither init nor cleanup.
• MSTestAdapter.PlatformServices.UnitTests green across net462/net48/net8.0/net9.0/net8.0-windows.

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

[Copilot]

Pull request overview

Fixes a regression in the MSTest adapter’s new ITestFilter “drop/skip before type-load” path where [ClassCleanup] could be skipped when the last-in-order test of an already-initialized class was filtered out (common in per-worker sharding scenarios).

Changes:

• Update UnitTestRunner.FinishFilteredOutTestAsync to execute [ClassCleanup] when the filtered-out test is the last test in the class and the class was previously initialized in the current worker.
• Add an acceptance test that reproduces the leak (drop last-in-order test of a partially-run class) and verifies no over-correction for fully-dropped classes.

Show a summary per file

File	Description
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestFilterClassCleanupTests.cs	Adds an acceptance regression test covering the “last-in-order test dropped” class-cleanup leak and the “fully dropped class” non-init/non-cleanup guard.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs	Runs class cleanup from the filter drop/skip tail path when the class was initialized earlier in the worker and the dropped/skipped test is the last test in its class.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9503

Δ	Test	Grade	Band	Notes
new	TestFilterClassCleanupTests. ClassCleanup_ RunsWhenLastTestOfInitializedClassIsDropped	A	90–100	Rich assertion set covers both the positive fix (ClassCleanup runs for the initialized class) and the guard against over-correction (ClassCleanup does not run for the fully-dropped class); clear AAA structure with informative comments.

_{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. · 179.3 AIC · ⌖ 12.7 AIC · ⊞ 43.7K · [◷]( · ◷)

[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.

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
16	Naming & Conventions	💬 1 NIT
17	Documentation Accuracy	💬 1 NIT

✅ 19/22 dimensions clean.

• Test Completeness — the cleanupResult is not null branch (cleanup fails in the drop path) has no acceptance-test coverage; AssociatedUnitTestElement attribution is unvalidated
• Naming — the Test_A_Run / Test_Z_Dropped alphabetical-ordering assumption is undocumented
• Documentation — successful [ClassCleanup] output is silently discarded when filterResult is empty; no comment explains this

---

Overall assessment: The fix is algorithmically correct and well-reasoned. The _lastRunnableTestByClass sentinel is the right signal: it's populated only when both conditions are true — the class has an executable cleanup method and at least one non-filtered test ran in this worker — so a fully-dropped class is still correctly skipped. The ExecuteClassCleanupAsync semaphore and IsClassCleanupExecuted flag in TestClassInfo prevent any risk of double execution even under concurrent test scheduling. Thread safety is maintained via ConcurrentDictionary.TryGetValue. The ConfigureAwait(false) and try/finally disposal patterns mirror the surrounding code correctly. The three items above are polish suggestions; none blocks the merge.

[Amaury Levé (Evangelink)]

Summary

Targets dev/amauryleve/custom-test-filter (#8896). Fixes a [ClassCleanup] leak in the new ITestFilter drop path, reported by an internal team using per-worker test sharding (each worker enumerates the full set per class, runs its subset, and Drops the rest).

The bug

UnitTestRunner.FinishFilteredOutTestAsync assumed a filtered-out test never loaded its type and therefore skipped class cleanup, only calling MarkClassComplete. That assumption is false when a sibling test of the same class already ran in this worker: [ClassInitialize] executed and [ClassCleanup] is still owed.

Because ClassCleanupManager counts down over the full (pre-filter) test set, isLastTestInClass is independent of run-vs-dropped. So whenever a class's last-in-order test is dropped — while earlier tests of that class ran — the worker leaks the class cleanup. ForceCleanup is not a safety net (it only fires on IsGracefulStopRequested).

The analogous assembly-cleanup asymmetry was already guarded by _assemblyInitializeWasExecuted, which is why only class cleanup leaked.

The fix

When the dropped test is the last in its class, run [ClassCleanup] if the class was initialized in this worker. Detection reuses the existing _lastRunnableTestByClass map, which is populated only for classes that both have an executable cleanup method and ran a non-filtered test. The TestClassInfo is already cached in the TypeCache, so resolving it loads no new type — preserving the feature's "a Drop pays zero type-load / init cost" guarantee. A fully-dropped class (type never loaded) still correctly skips both init and cleanup.

Validation

• Added acceptance test TestFilterClassCleanupTests reproducing the leak: a filter drops the last-in-order test of an initialized class and the only test of a fully-dropped class.
  • Against the unfixed adapter the test fails — output shows ClassInitialize + the run test but no ClassCleanup, exactly the reported symptom.
  • Against the fixed adapter it passes; the fully-dropped class runs neither init nor cleanup.
• MSTestAdapter.PlatformServices.UnitTests green across net462/net48/net8.0/net9.0/net8.0-windows.

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

[Copilot]

Pull request overview

Fixes a regression in the MSTest adapter’s new ITestFilter “drop/skip before type-load” path where [ClassCleanup] could be skipped when the last-in-order test of an already-initialized class was filtered out (common in per-worker sharding scenarios).

Changes:

• Update UnitTestRunner.FinishFilteredOutTestAsync to execute [ClassCleanup] when the filtered-out test is the last test in the class and the class was previously initialized in the current worker.
• Add an acceptance test that reproduces the leak (drop last-in-order test of a partially-run class) and verifies no over-correction for fully-dropped classes.

Show a summary per file

File	Description
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestFilterClassCleanupTests.cs	Adds an acceptance regression test covering the “last-in-order test dropped” class-cleanup leak and the “fully dropped class” non-init/non-cleanup guard.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs	Runs class cleanup from the filter drop/skip tail path when the class was initialized earlier in the worker and the dropped/skipped test is the last test in its class.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9503

Δ	Test	Grade	Band	Notes
new	TestFilterClassCleanupTests. ClassCleanup_ RunsWhenLastTestOfInitializedClassIsDropped	A	90–100	Rich assertion set covers both the positive fix (ClassCleanup runs for the initialized class) and the guard against over-correction (ClassCleanup does not run for the fully-dropped class); clear AAA structure with informative comments.

_{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. · 179.3 AIC · ⌖ 12.7 AIC · ⊞ 43.7K · [◷]( · ◷)

[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.

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
16	Naming & Conventions	💬 1 NIT
17	Documentation Accuracy	💬 1 NIT

✅ 19/22 dimensions clean.

• Test Completeness — the cleanupResult is not null branch (cleanup fails in the drop path) has no acceptance-test coverage; AssociatedUnitTestElement attribution is unvalidated
• Naming — the Test_A_Run / Test_Z_Dropped alphabetical-ordering assumption is undocumented
• Documentation — successful [ClassCleanup] output is silently discarded when filterResult is empty; no comment explains this

---

Overall assessment: The fix is algorithmically correct and well-reasoned. The _lastRunnableTestByClass sentinel is the right signal: it's populated only when both conditions are true — the class has an executable cleanup method and at least one non-filtered test ran in this worker — so a fully-dropped class is still correctly skipped. The ExecuteClassCleanupAsync semaphore and IsClassCleanupExecuted flag in TestClassInfo prevent any risk of double execution even under concurrent test scheduling. Thread safety is maintained via ConcurrentDictionary.TryGetValue. The ConfigureAwait(false) and try/finally disposal patterns mirror the surrounding code correctly. The three items above are polish suggestions; none blocks the merge.

[Amaury Levé (Evangelink)]

Summary

Targets dev/amauryleve/custom-test-filter (#8896). Fixes a [ClassCleanup] leak in the new ITestFilter drop path, reported by an internal team using per-worker test sharding (each worker enumerates the full set per class, runs its subset, and Drops the rest).

The bug

UnitTestRunner.FinishFilteredOutTestAsync assumed a filtered-out test never loaded its type and therefore skipped class cleanup, only calling MarkClassComplete. That assumption is false when a sibling test of the same class already ran in this worker: [ClassInitialize] executed and [ClassCleanup] is still owed.

Because ClassCleanupManager counts down over the full (pre-filter) test set, isLastTestInClass is independent of run-vs-dropped. So whenever a class's last-in-order test is dropped — while earlier tests of that class ran — the worker leaks the class cleanup. ForceCleanup is not a safety net (it only fires on IsGracefulStopRequested).

The analogous assembly-cleanup asymmetry was already guarded by _assemblyInitializeWasExecuted, which is why only class cleanup leaked.

The fix

When the dropped test is the last in its class, run [ClassCleanup] if the class was initialized in this worker. Detection reuses the existing _lastRunnableTestByClass map, which is populated only for classes that both have an executable cleanup method and ran a non-filtered test. The TestClassInfo is already cached in the TypeCache, so resolving it loads no new type — preserving the feature's "a Drop pays zero type-load / init cost" guarantee. A fully-dropped class (type never loaded) still correctly skips both init and cleanup.

Validation

• Added acceptance test TestFilterClassCleanupTests reproducing the leak: a filter drops the last-in-order test of an initialized class and the only test of a fully-dropped class.
  • Against the unfixed adapter the test fails — output shows ClassInitialize + the run test but no ClassCleanup, exactly the reported symptom.
  • Against the fixed adapter it passes; the fully-dropped class runs neither init nor cleanup.
• MSTestAdapter.PlatformServices.UnitTests green across net462/net48/net8.0/net9.0/net8.0-windows.

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

[Copilot]

Pull request overview

Fixes a regression in the MSTest adapter’s new ITestFilter “drop/skip before type-load” path where [ClassCleanup] could be skipped when the last-in-order test of an already-initialized class was filtered out (common in per-worker sharding scenarios).

Changes:

• Update UnitTestRunner.FinishFilteredOutTestAsync to execute [ClassCleanup] when the filtered-out test is the last test in the class and the class was previously initialized in the current worker.
• Add an acceptance test that reproduces the leak (drop last-in-order test of a partially-run class) and verifies no over-correction for fully-dropped classes.

Show a summary per file

File	Description
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestFilterClassCleanupTests.cs	Adds an acceptance regression test covering the “last-in-order test dropped” class-cleanup leak and the “fully dropped class” non-init/non-cleanup guard.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs	Runs class cleanup from the filter drop/skip tail path when the class was initialized earlier in the worker and the dropped/skipped test is the last test in its class.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9503

Δ	Test	Grade	Band	Notes
new	TestFilterClassCleanupTests. ClassCleanup_ RunsWhenLastTestOfInitializedClassIsDropped	A	90–100	Rich assertion set covers both the positive fix (ClassCleanup runs for the initialized class) and the guard against over-correction (ClassCleanup does not run for the fully-dropped class); clear AAA structure with informative comments.

_{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. · 179.3 AIC · ⌖ 12.7 AIC · ⊞ 43.7K · [◷]( · ◷)

[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.

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
16	Naming & Conventions	💬 1 NIT
17	Documentation Accuracy	💬 1 NIT

✅ 19/22 dimensions clean.

• Test Completeness — the cleanupResult is not null branch (cleanup fails in the drop path) has no acceptance-test coverage; AssociatedUnitTestElement attribution is unvalidated
• Naming — the Test_A_Run / Test_Z_Dropped alphabetical-ordering assumption is undocumented
• Documentation — successful [ClassCleanup] output is silently discarded when filterResult is empty; no comment explains this

---

Overall assessment: The fix is algorithmically correct and well-reasoned. The _lastRunnableTestByClass sentinel is the right signal: it's populated only when both conditions are true — the class has an executable cleanup method and at least one non-filtered test ran in this worker — so a fully-dropped class is still correctly skipped. The ExecuteClassCleanupAsync semaphore and IsClassCleanupExecuted flag in TestClassInfo prevent any risk of double execution even under concurrent test scheduling. Thread safety is maintained via ConcurrentDictionary.TryGetValue. The ConfigureAwait(false) and try/finally disposal patterns mirror the surrounding code correctly. The three items above are polish suggestions; none blocks the merge.

[Amaury Levé (Evangelink)]

Summary

Targets dev/amauryleve/custom-test-filter (#8896). Fixes a [ClassCleanup] leak in the new ITestFilter drop path, reported by an internal team using per-worker test sharding (each worker enumerates the full set per class, runs its subset, and Drops the rest).

The bug

UnitTestRunner.FinishFilteredOutTestAsync assumed a filtered-out test never loaded its type and therefore skipped class cleanup, only calling MarkClassComplete. That assumption is false when a sibling test of the same class already ran in this worker: [ClassInitialize] executed and [ClassCleanup] is still owed.

Because ClassCleanupManager counts down over the full (pre-filter) test set, isLastTestInClass is independent of run-vs-dropped. So whenever a class's last-in-order test is dropped — while earlier tests of that class ran — the worker leaks the class cleanup. ForceCleanup is not a safety net (it only fires on IsGracefulStopRequested).

The analogous assembly-cleanup asymmetry was already guarded by _assemblyInitializeWasExecuted, which is why only class cleanup leaked.

The fix

When the dropped test is the last in its class, run [ClassCleanup] if the class was initialized in this worker. Detection reuses the existing _lastRunnableTestByClass map, which is populated only for classes that both have an executable cleanup method and ran a non-filtered test. The TestClassInfo is already cached in the TypeCache, so resolving it loads no new type — preserving the feature's "a Drop pays zero type-load / init cost" guarantee. A fully-dropped class (type never loaded) still correctly skips both init and cleanup.

Validation

• Added acceptance test TestFilterClassCleanupTests reproducing the leak: a filter drops the last-in-order test of an initialized class and the only test of a fully-dropped class.
  • Against the unfixed adapter the test fails — output shows ClassInitialize + the run test but no ClassCleanup, exactly the reported symptom.
  • Against the fixed adapter it passes; the fully-dropped class runs neither init nor cleanup.
• MSTestAdapter.PlatformServices.UnitTests green across net462/net48/net8.0/net9.0/net8.0-windows.

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

[Copilot]

Pull request overview

Fixes a regression in the MSTest adapter’s new ITestFilter “drop/skip before type-load” path where [ClassCleanup] could be skipped when the last-in-order test of an already-initialized class was filtered out (common in per-worker sharding scenarios).

Changes:

• Update UnitTestRunner.FinishFilteredOutTestAsync to execute [ClassCleanup] when the filtered-out test is the last test in the class and the class was previously initialized in the current worker.
• Add an acceptance test that reproduces the leak (drop last-in-order test of a partially-run class) and verifies no over-correction for fully-dropped classes.

Show a summary per file

File	Description
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestFilterClassCleanupTests.cs	Adds an acceptance regression test covering the “last-in-order test dropped” class-cleanup leak and the “fully dropped class” non-init/non-cleanup guard.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs	Runs class cleanup from the filter drop/skip tail path when the class was initialized earlier in the worker and the dropped/skipped test is the last test in its class.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9503

Δ	Test	Grade	Band	Notes
new	TestFilterClassCleanupTests. ClassCleanup_ RunsWhenLastTestOfInitializedClassIsDropped	A	90–100	Rich assertion set covers both the positive fix (ClassCleanup runs for the initialized class) and the guard against over-correction (ClassCleanup does not run for the fully-dropped class); clear AAA structure with informative comments.

_{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. · 179.3 AIC · ⌖ 12.7 AIC · ⊞ 43.7K · [◷]( · ◷)

[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.

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
16	Naming & Conventions	💬 1 NIT
17	Documentation Accuracy	💬 1 NIT

✅ 19/22 dimensions clean.

• Test Completeness — the cleanupResult is not null branch (cleanup fails in the drop path) has no acceptance-test coverage; AssociatedUnitTestElement attribution is unvalidated
• Naming — the Test_A_Run / Test_Z_Dropped alphabetical-ordering assumption is undocumented
• Documentation — successful [ClassCleanup] output is silently discarded when filterResult is empty; no comment explains this

---

Overall assessment: The fix is algorithmically correct and well-reasoned. The _lastRunnableTestByClass sentinel is the right signal: it's populated only when both conditions are true — the class has an executable cleanup method and at least one non-filtered test ran in this worker — so a fully-dropped class is still correctly skipped. The ExecuteClassCleanupAsync semaphore and IsClassCleanupExecuted flag in TestClassInfo prevent any risk of double execution even under concurrent test scheduling. Thread safety is maintained via ConcurrentDictionary.TryGetValue. The ConfigureAwait(false) and try/finally disposal patterns mirror the surrounding code correctly. The three items above are polish suggestions; none blocks the merge.

[Amaury Levé (Evangelink)]

Summary

Targets dev/amauryleve/custom-test-filter (#8896). Fixes a [ClassCleanup] leak in the new ITestFilter drop path, reported by an internal team using per-worker test sharding (each worker enumerates the full set per class, runs its subset, and Drops the rest).

The bug

UnitTestRunner.FinishFilteredOutTestAsync assumed a filtered-out test never loaded its type and therefore skipped class cleanup, only calling MarkClassComplete. That assumption is false when a sibling test of the same class already ran in this worker: [ClassInitialize] executed and [ClassCleanup] is still owed.

Because ClassCleanupManager counts down over the full (pre-filter) test set, isLastTestInClass is independent of run-vs-dropped. So whenever a class's last-in-order test is dropped — while earlier tests of that class ran — the worker leaks the class cleanup. ForceCleanup is not a safety net (it only fires on IsGracefulStopRequested).

The analogous assembly-cleanup asymmetry was already guarded by _assemblyInitializeWasExecuted, which is why only class cleanup leaked.

The fix

When the dropped test is the last in its class, run [ClassCleanup] if the class was initialized in this worker. Detection reuses the existing _lastRunnableTestByClass map, which is populated only for classes that both have an executable cleanup method and ran a non-filtered test. The TestClassInfo is already cached in the TypeCache, so resolving it loads no new type — preserving the feature's "a Drop pays zero type-load / init cost" guarantee. A fully-dropped class (type never loaded) still correctly skips both init and cleanup.

Validation

• Added acceptance test TestFilterClassCleanupTests reproducing the leak: a filter drops the last-in-order test of an initialized class and the only test of a fully-dropped class.
  • Against the unfixed adapter the test fails — output shows ClassInitialize + the run test but no ClassCleanup, exactly the reported symptom.
  • Against the fixed adapter it passes; the fully-dropped class runs neither init nor cleanup.
• MSTestAdapter.PlatformServices.UnitTests green across net462/net48/net8.0/net9.0/net8.0-windows.

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

[Copilot]

Pull request overview

Fixes a regression in the MSTest adapter’s new ITestFilter “drop/skip before type-load” path where [ClassCleanup] could be skipped when the last-in-order test of an already-initialized class was filtered out (common in per-worker sharding scenarios).

Changes:

• Update UnitTestRunner.FinishFilteredOutTestAsync to execute [ClassCleanup] when the filtered-out test is the last test in the class and the class was previously initialized in the current worker.
• Add an acceptance test that reproduces the leak (drop last-in-order test of a partially-run class) and verifies no over-correction for fully-dropped classes.

Show a summary per file

File	Description
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestFilterClassCleanupTests.cs	Adds an acceptance regression test covering the “last-in-order test dropped” class-cleanup leak and the “fully dropped class” non-init/non-cleanup guard.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs	Runs class cleanup from the filter drop/skip tail path when the class was initialized earlier in the worker and the dropped/skipped test is the last test in its class.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9503

Δ	Test	Grade	Band	Notes
new	TestFilterClassCleanupTests. ClassCleanup_ RunsWhenLastTestOfInitializedClassIsDropped	A	90–100	Rich assertion set covers both the positive fix (ClassCleanup runs for the initialized class) and the guard against over-correction (ClassCleanup does not run for the fully-dropped class); clear AAA structure with informative comments.

_{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. · 179.3 AIC · ⌖ 12.7 AIC · ⊞ 43.7K · [◷]( · ◷)

[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.

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
16	Naming & Conventions	💬 1 NIT
17	Documentation Accuracy	💬 1 NIT

✅ 19/22 dimensions clean.

• Test Completeness — the cleanupResult is not null branch (cleanup fails in the drop path) has no acceptance-test coverage; AssociatedUnitTestElement attribution is unvalidated
• Naming — the Test_A_Run / Test_Z_Dropped alphabetical-ordering assumption is undocumented
• Documentation — successful [ClassCleanup] output is silently discarded when filterResult is empty; no comment explains this

---

Overall assessment: The fix is algorithmically correct and well-reasoned. The _lastRunnableTestByClass sentinel is the right signal: it's populated only when both conditions are true — the class has an executable cleanup method and at least one non-filtered test ran in this worker — so a fully-dropped class is still correctly skipped. The ExecuteClassCleanupAsync semaphore and IsClassCleanupExecuted flag in TestClassInfo prevent any risk of double execution even under concurrent test scheduling. Thread safety is maintained via ConcurrentDictionary.TryGetValue. The ConfigureAwait(false) and try/finally disposal patterns mirror the surrounding code correctly. The three items above are polish suggestions; none blocks the merge.

[Amaury Levé (Evangelink)]

Summary

Targets dev/amauryleve/custom-test-filter (#8896). Fixes a [ClassCleanup] leak in the new ITestFilter drop path, reported by an internal team using per-worker test sharding (each worker enumerates the full set per class, runs its subset, and Drops the rest).

The bug

UnitTestRunner.FinishFilteredOutTestAsync assumed a filtered-out test never loaded its type and therefore skipped class cleanup, only calling MarkClassComplete. That assumption is false when a sibling test of the same class already ran in this worker: [ClassInitialize] executed and [ClassCleanup] is still owed.

Because ClassCleanupManager counts down over the full (pre-filter) test set, isLastTestInClass is independent of run-vs-dropped. So whenever a class's last-in-order test is dropped — while earlier tests of that class ran — the worker leaks the class cleanup. ForceCleanup is not a safety net (it only fires on IsGracefulStopRequested).

The analogous assembly-cleanup asymmetry was already guarded by _assemblyInitializeWasExecuted, which is why only class cleanup leaked.

The fix

When the dropped test is the last in its class, run [ClassCleanup] if the class was initialized in this worker. Detection reuses the existing _lastRunnableTestByClass map, which is populated only for classes that both have an executable cleanup method and ran a non-filtered test. The TestClassInfo is already cached in the TypeCache, so resolving it loads no new type — preserving the feature's "a Drop pays zero type-load / init cost" guarantee. A fully-dropped class (type never loaded) still correctly skips both init and cleanup.

Validation

• Added acceptance test TestFilterClassCleanupTests reproducing the leak: a filter drops the last-in-order test of an initialized class and the only test of a fully-dropped class.
  • Against the unfixed adapter the test fails — output shows ClassInitialize + the run test but no ClassCleanup, exactly the reported symptom.
  • Against the fixed adapter it passes; the fully-dropped class runs neither init nor cleanup.
• MSTestAdapter.PlatformServices.UnitTests green across net462/net48/net8.0/net9.0/net8.0-windows.

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

[Copilot]

Pull request overview

Fixes a regression in the MSTest adapter’s new ITestFilter “drop/skip before type-load” path where [ClassCleanup] could be skipped when the last-in-order test of an already-initialized class was filtered out (common in per-worker sharding scenarios).

Changes:

• Update UnitTestRunner.FinishFilteredOutTestAsync to execute [ClassCleanup] when the filtered-out test is the last test in the class and the class was previously initialized in the current worker.
• Add an acceptance test that reproduces the leak (drop last-in-order test of a partially-run class) and verifies no over-correction for fully-dropped classes.

Show a summary per file

File	Description
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestFilterClassCleanupTests.cs	Adds an acceptance regression test covering the “last-in-order test dropped” class-cleanup leak and the “fully dropped class” non-init/non-cleanup guard.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs	Runs class cleanup from the filter drop/skip tail path when the class was initialized earlier in the worker and the dropped/skipped test is the last test in its class.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9503

Δ	Test	Grade	Band	Notes
new	TestFilterClassCleanupTests. ClassCleanup_ RunsWhenLastTestOfInitializedClassIsDropped	A	90–100	Rich assertion set covers both the positive fix (ClassCleanup runs for the initialized class) and the guard against over-correction (ClassCleanup does not run for the fully-dropped class); clear AAA structure with informative comments.

_{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. · 179.3 AIC · ⌖ 12.7 AIC · ⊞ 43.7K · [◷]( · ◷)

[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.

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
16	Naming & Conventions	💬 1 NIT
17	Documentation Accuracy	💬 1 NIT

✅ 19/22 dimensions clean.

• Test Completeness — the cleanupResult is not null branch (cleanup fails in the drop path) has no acceptance-test coverage; AssociatedUnitTestElement attribution is unvalidated
• Naming — the Test_A_Run / Test_Z_Dropped alphabetical-ordering assumption is undocumented
• Documentation — successful [ClassCleanup] output is silently discarded when filterResult is empty; no comment explains this

---

Overall assessment: The fix is algorithmically correct and well-reasoned. The _lastRunnableTestByClass sentinel is the right signal: it's populated only when both conditions are true — the class has an executable cleanup method and at least one non-filtered test ran in this worker — so a fully-dropped class is still correctly skipped. The ExecuteClassCleanupAsync semaphore and IsClassCleanupExecuted flag in TestClassInfo prevent any risk of double execution even under concurrent test scheduling. Thread safety is maintained via ConcurrentDictionary.TryGetValue. The ConfigureAwait(false) and try/finally disposal patterns mirror the surrounding code correctly. The three items above are polish suggestions; none blocks the merge.

[Amaury Levé (Evangelink)]

Summary

Targets dev/amauryleve/custom-test-filter (#8896). Fixes a [ClassCleanup] leak in the new ITestFilter drop path, reported by an internal team using per-worker test sharding (each worker enumerates the full set per class, runs its subset, and Drops the rest).

The bug

UnitTestRunner.FinishFilteredOutTestAsync assumed a filtered-out test never loaded its type and therefore skipped class cleanup, only calling MarkClassComplete. That assumption is false when a sibling test of the same class already ran in this worker: [ClassInitialize] executed and [ClassCleanup] is still owed.

Because ClassCleanupManager counts down over the full (pre-filter) test set, isLastTestInClass is independent of run-vs-dropped. So whenever a class's last-in-order test is dropped — while earlier tests of that class ran — the worker leaks the class cleanup. ForceCleanup is not a safety net (it only fires on IsGracefulStopRequested).

The analogous assembly-cleanup asymmetry was already guarded by _assemblyInitializeWasExecuted, which is why only class cleanup leaked.

The fix

When the dropped test is the last in its class, run [ClassCleanup] if the class was initialized in this worker. Detection reuses the existing _lastRunnableTestByClass map, which is populated only for classes that both have an executable cleanup method and ran a non-filtered test. The TestClassInfo is already cached in the TypeCache, so resolving it loads no new type — preserving the feature's "a Drop pays zero type-load / init cost" guarantee. A fully-dropped class (type never loaded) still correctly skips both init and cleanup.

Validation

• Added acceptance test TestFilterClassCleanupTests reproducing the leak: a filter drops the last-in-order test of an initialized class and the only test of a fully-dropped class.
  • Against the unfixed adapter the test fails — output shows ClassInitialize + the run test but no ClassCleanup, exactly the reported symptom.
  • Against the fixed adapter it passes; the fully-dropped class runs neither init nor cleanup.
• MSTestAdapter.PlatformServices.UnitTests green across net462/net48/net8.0/net9.0/net8.0-windows.

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

[Copilot]

Pull request overview

Fixes a regression in the MSTest adapter’s new ITestFilter “drop/skip before type-load” path where [ClassCleanup] could be skipped when the last-in-order test of an already-initialized class was filtered out (common in per-worker sharding scenarios).

Changes:

• Update UnitTestRunner.FinishFilteredOutTestAsync to execute [ClassCleanup] when the filtered-out test is the last test in the class and the class was previously initialized in the current worker.
• Add an acceptance test that reproduces the leak (drop last-in-order test of a partially-run class) and verifies no over-correction for fully-dropped classes.

Show a summary per file

File	Description
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestFilterClassCleanupTests.cs	Adds an acceptance regression test covering the “last-in-order test dropped” class-cleanup leak and the “fully dropped class” non-init/non-cleanup guard.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.cs	Runs class cleanup from the filter drop/skip tail path when the class was initialized earlier in the worker and the dropped/skipped test is the last test in its class.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9503

Δ	Test	Grade	Band	Notes
new	TestFilterClassCleanupTests. ClassCleanup_ RunsWhenLastTestOfInitializedClassIsDropped	A	90–100	Rich assertion set covers both the positive fix (ClassCleanup runs for the initialized class) and the guard against over-correction (ClassCleanup does not run for the fully-dropped class); clear AAA structure with informative comments.

_{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. · 179.3 AIC · ⌖ 12.7 AIC · ⊞ 43.7K · [◷]( · ◷)

[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.

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
16	Naming & Conventions	💬 1 NIT
17	Documentation Accuracy	💬 1 NIT

✅ 19/22 dimensions clean.

• Test Completeness — the cleanupResult is not null branch (cleanup fails in the drop path) has no acceptance-test coverage; AssociatedUnitTestElement attribution is unvalidated
• Naming — the Test_A_Run / Test_Z_Dropped alphabetical-ordering assumption is undocumented
• Documentation — successful [ClassCleanup] output is silently discarded when filterResult is empty; no comment explains this

---

Overall assessment: The fix is algorithmically correct and well-reasoned. The _lastRunnableTestByClass sentinel is the right signal: it's populated only when both conditions are true — the class has an executable cleanup method and at least one non-filtered test ran in this worker — so a fully-dropped class is still correctly skipped. The ExecuteClassCleanupAsync semaphore and IsClassCleanupExecuted flag in TestClassInfo prevent any risk of double execution even under concurrent test scheduling. Thread safety is maintained via ConcurrentDictionary.TryGetValue. The ConfigureAwait(false) and try/finally disposal patterns mirror the surrounding code correctly. The three items above are polish suggestions; none blocks the merge.