Skip to content

Fix UnitTestRunner leaking some test class instances #5715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 10, 2025

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jun 9, 2025

Fixes #5711

TestMethodInfo holds _classInstance. So storing it in a dictionary that lives for the whole test session will leak the stored test class instances.

While I think we could avoid storing _classInstance in TestMethodInfo, it's still an "overkill" to store TestMethodInfo in the dictionary as we end up only accessing either TestClassInfo or TestAssemblyInfo.

So, the dictionary is now split to two dictionaries, one that stores class fixtures and the other stores assembly fixtures. This in theory is still bad because as the number of test classes grow, this dictionary can grow, and also this dictionary is only ever used when ConsiderFixturesAsSpecialTests is enabled, but we still always create the dictionary. But this is a step forward as it at least avoids rooting test class instances.

@Youssef1313 Youssef1313 marked this pull request as ready for review June 9, 2025 15:55
@Youssef1313 Youssef1313 requested a review from fhnaseer June 9, 2025 16:13
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.15%. Comparing base (97ee44f) to head (8a8256a).

Files with missing lines Patch % Lines
...ter/MSTest.TestAdapter/Execution/UnitTestRunner.cs 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5715      +/-   ##
==========================================
- Coverage   76.20%   76.15%   -0.06%     
==========================================
  Files         602      602              
  Lines       36707    36755      +48     
==========================================
+ Hits        27973    27989      +16     
- Misses       8734     8766      +32     
Flag Coverage Δ
Debug 76.15% <90.00%> (-0.06%) ⬇️
integration 76.15% <90.00%> (-0.06%) ⬇️
production 76.15% <90.00%> (-0.06%) ⬇️
unit 76.14% <90.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ter/MSTest.TestAdapter/Execution/UnitTestRunner.cs 90.88% <90.00%> (-6.16%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Youssef1313 Youssef1313 requested a review from fhnaseer June 10, 2025 07:51
@Youssef1313 Youssef1313 merged commit db8f106 into main Jun 10, 2025
8 checks passed
@Youssef1313 Youssef1313 deleted the dev/ygerges/leak branch June 10, 2025 08:32
@nohwnd
Copy link
Member

nohwnd commented Jun 10, 2025

/backport to rel/3.9

Copy link
Contributor

Started backporting to rel/3.9: https://github.com/microsoft/testfx/actions/runs/15556763980

Youssef1313 added a commit that referenced this pull request Jun 10, 2025
#5715 (backport to rel/3.9) (#5724)

Co-authored-by: Youssef1313 <youssefvictor00@gmail.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.

UnitTestRunner "leaks" Test Class instances
4 participants