Use a module initializer to install ThrowingTraceListener #47500
Conversation
c96b6b5 to
177d2b4
Compare
| <trace> | ||
| <listeners> | ||
| <remove name="Default" /> | ||
| <add name="ThrowingTraceListener" type="Microsoft.CodeAnalysis.ThrowingTraceListener, Roslyn.Test.Utilities" /> |
There was a problem hiding this comment.
At one point this file was used to ensure all of the exe that we build in our infra use a throwing trace listener vs. the default ones. How does that work now for our exe files?
There was a problem hiding this comment.
It never worked. .NET Core does not use this file.
There was a problem hiding this comment.
Yes but this is important for .NET Desktop too for the same reasons.
There was a problem hiding this comment.
The .NET Desktop version of this file has not been changed.
https://github.com/dotnet/roslyn/blob/master/eng/config/test/Desktop/app.config
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'"> | ||
| <Compile Include="$(RepositoryEngineeringDir)config\test\Core\InstallTraceListener$(DefaultLanguageSourceExtension)" | ||
| Condition="Exists('$(RepositoryEngineeringDir)config\test\Core\InstallTraceListener$(DefaultLanguageSourceExtension)')"/> | ||
| </ItemGroup> |
There was a problem hiding this comment.
Module initializers work on .NET Framework too so why aren't we just universally doing this?
There was a problem hiding this comment.
With this pull request, the following are covered:
- .NET Framework test projects (via app.config)
- .NET Core test projects written in C# (via module initializer)
- .NET Core test projects written in Visual Basic that use
<UseExportProvider>
Keeping app.config for .NET Framework prevents a test gap for some Visual Basic projects.
There was a problem hiding this comment.
Hmm, I think another way of stating is that there is a gap in VB for .NET Core projects with this change. Essentially paths that don't go through <UseExportProvider>. This gap does not exist on desktop because of the app.config work. Correct?
There was a problem hiding this comment.
Correct. Xunit doesn't offer an AssemblyInitialize extension point, and Visual Basic does not offer module initializers, so we do not have a way to guarantee that the trace listener is installed by the time the first test executes.
There was a problem hiding this comment.
K. Think we should doc that so it's clearer what this still gives us.
There was a problem hiding this comment.
Will submit a follow-up PR to document the condition here.
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| #if !NET5_0 |
There was a problem hiding this comment.
Referencing the .NET 5 TFM issue so we know to come back and change this if the design for what NET5_0 represents changes
| // Make sure we run the module initializer for Roslyn.Test.Utilities. C# projects do this via a | ||
| // build-injected module initializer, but VB projects can ensure initialization occurs by applying the | ||
| // UseExportProviderAttribute to test classes that rely on it. | ||
| RuntimeHelpers.RunModuleConstructor(typeof(TestBase).Module.ModuleHandle); |
There was a problem hiding this comment.
In the case of c# this will cause the constructor to run twice correct?
I don't think this impacts correctness really but wanted to make sure I understood the implications.
There was a problem hiding this comment.
I have no idea. I was expecting semantics similar to RunClassConstructor, where it's guaranteed to execute at most once, and exactly once by the time this call returns.
There was a problem hiding this comment.
Looking at the impl I think it only runs once
Prevents
Debug.Assertfailures from callingFailFaston the test process in .NET Core runs.