Skip to content
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.

Sign up for GitHub

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

Use a module initializer to install ThrowingTraceListener #47500

Merged
merged 1 commit into from
Sep 8, 2020
Merged

Use a module initializer to install ThrowingTraceListener #47500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions eng/config/test/Core/app.config
View file
Open in desktop

This file was deleted.

8 changes: 6 additions & 2 deletions eng/targets/XUnit.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,20 @@
<AdditionalFiles Include="$(MSBuildThisFileDirectory)..\config\test\BannedSymbols.txt" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'">
<Compile Include="$(RepositoryEngineeringDir)config\test\Core\InstallTraceListener$(DefaultLanguageSourceExtension)"
Condition="Exists('$(RepositoryEngineeringDir)config\test\Core\InstallTraceListener$(DefaultLanguageSourceExtension)')"/>
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module initializers work on .NET Framework too so why aren't we just universally doing this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K. Think we should doc that so it's clearer what this still gives us.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will submit a follow-up PR to document the condition here.


<!-- Add a default test app.config, if the project doesn't already have one-->
<Target Name="AddDefaultTestAppConfig">
<PropertyGroup>
<_AlreadyHasAppConfig Condition="'%(None.Filename)%(None.Extension)' == 'app.config'">true</_AlreadyHasAppConfig>
<_DefaultAppConfigFile Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">$(RepositoryEngineeringDir)config\test\Desktop\app.config</_DefaultAppConfigFile>
<_DefaultAppConfigFile Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'">$(RepositoryEngineeringDir)config\test\Core\app.config</_DefaultAppConfigFile>
</PropertyGroup>

<ItemGroup Condition="'$(_AlreadyHasAppConfig)' != 'true'">
<None Include="$(_DefaultAppConfigFile)">
<None Include="$(_DefaultAppConfigFile)" Condition="'$(_DefaultAppConfigFile)' != ''">
<Link>app.config</Link>
</None>
</ItemGroup>
Expand Down
1 change: 1 addition & 0 deletions src/Test/Utilities/Portable/Assert/AssertXml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.Xml.Linq;
using Roslyn.Utilities;
using Xunit;
using ReferenceEqualityComparer = Roslyn.Utilities.ReferenceEqualityComparer;

namespace Roslyn.Test.Utilities
{
Expand Down
20 changes: 20 additions & 0 deletions src/Test/Utilities/Portable/ModuleInitializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.Runtime.CompilerServices;
using Microsoft.CodeAnalysis;

namespace Roslyn.Test.Utilities
{
internal static class ModuleInitializer
{
[ModuleInitializer]
internal static void Initialize()
{
Trace.Listeners.Clear();
Trace.Listeners.Add(new ThrowingTraceListener());
}
}
}
15 changes: 15 additions & 0 deletions src/Test/Utilities/Portable/ModuleInitializerAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Licensed to the .NET Foundation under one or more agreements.
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referencing the .NET 5 TFM issue so we know to come back and change this if the design for what NET5_0 represents changes

dotnet/sdk#13377


namespace System.Runtime.CompilerServices
{
[AttributeUsage(AttributeTargets.Method, AllowMultiple = false)]
public sealed class ModuleInitializerAttribute : Attribute
{
}
}

#endif
2 changes: 1 addition & 1 deletion src/Test/Utilities/Portable/Roslyn.Test.Utilities.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<PropertyGroup>
<OutputType>Library</OutputType>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<TargetFrameworks>netcoreapp3.1;netstandard2.0;net472</TargetFrameworks>
<TargetFrameworks>net5.0;netcoreapp3.1;netstandard2.0;net472</TargetFrameworks>
<IsShipping>false</IsShipping>
</PropertyGroup>
<ItemGroup>
Expand Down
9 changes: 9 additions & 0 deletions src/Workspaces/CoreTestUtilities/MEF/UseExportProviderAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Composition.Hosting;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Text;
using System.Threading;
using Microsoft.CodeAnalysis.Editor;
Expand Down Expand Up @@ -59,6 +60,14 @@ public class UseExportProviderAttribute : BeforeAfterTestAttribute

private MefHostServices? _hostServices;

static UseExportProviderAttribute()
{
// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the impl I think it only runs once

https://github.com/dotnet/runtime/blob/54d3a76eb69143ba59fcf8532ab7600f90161522/src/coreclr/src/vm/reflectioninvocation.cpp#L1796

}

public override void Before(MethodInfo methodUnderTest)
{
MefHostServices.TestAccessor.HookServiceCreation(CreateMefHostServices);
Expand Down