Fix TraceListenerManager thread safety issue #5750
Conversation
35fedc7 to
1ac0e14
Compare
be4013a to
3543c3e
Compare
fb55cd2 to
5a12d2b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5750 +/- ##
==========================================
- Coverage 73.89% 73.79% -0.10%
==========================================
Files 602 605 +3
Lines 37449 36682 -767
==========================================
- Hits 27672 27070 -602
+ Misses 9777 9612 -165
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Is there a possibility to prove that this works via a test? All the changed tests seem to just change because of the approach change, but there is no test specific to proving that we can safely write from many threads. |
src/Adapter/MSTestAdapter.PlatformServices/Execution/ConsoleErrorCapturer.cs
Show resolved
Hide resolved
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestAssemblyInfo.cs
Show resolved
Hide resolved
I think we had some example of this but I don't recall where. I think the story was around the out being set by user in parallel of our code running which was causing the |
Evangelink
left a comment
There was a problem hiding this comment.
As discussed offline, adding a test is tricky but some tests were being disabled because of their flakiness related to this issue so we should be able to see if that's enough through multiple runs
Fixes #2616
Instead of repeatedly setting Console.Out/Console.Error and resetting to original in a thread-unsafe way, we now set Conosle.Out/Console.Error once in the UnitTestRunner constructor.
Additionally, the logic of capturing is way simplified now.
Each step of the test lifecycle (asm init, asm cleanup, class init, test, class cleanup, and asm cleanup) will set its current TestContext. The current TestContext is async local, so it's isolated properly when running in parallel. Then when each step creates its test result, it just needs to get the data from the current test context.
The redirected out/error will simply write to the current test context if there is one, or write to the original if there isn't one. This simplifies the logic a lot and makes it much easier to follow.
Note that there is a lot of code that can be deleted with this change, but we cannot do so now as it affects public API. I kept the public API the same. We simply just are not using it in the implementation.