Standardize our test references by jaredpar · Pull Request #45690 · dotnet/roslyn

jaredpar · 2020-07-06T17:30:10Z

This change standardizes the reference assemblies we use for compilation in our unit test to be the actual .NET Framework reference assemblies.

Prior to this change they were a mix of reference and implementation assemblies from non-RTM versions of the .NET Framework. In order to map the existing assemblies to their respective target framework version I worked with the servicing team and we mapped them to the closest possible TFM based on their file versions. The bulk of the change is essentially mechanically moving from names like mscorlib.v4_30319_17929 to Net451.mscorlib.

Unfortunately in several cases the non-RTM assemblies didn't cleanly map to a RTM TFM. This was particularly problematic when migrating the v4_30319 references as this is a mix of a net40 and net451 versions. This caused issues in our code base because these references were freely mixed with actual net40 and net451 references in the code base. Most of the time this went unnoticed because the test didn't expose the gaps in the APIs. In a lot of cases though, particularly when mixing a version of System.Core that did or didn't have ExtensionsAttribute, this required some manual inspection on my part and a fix. In pretty much every case this was a straight forward fix but it did mean the change was less mechanical than I would have preferred.

To make this change easier to review I've broken it up into three commits:

The actual changes to Roslyn.Test.Utilities that move us to the new references.
The changes to our test code which required more than a simple straight forward reference update or explicitly involved retargetting code. These are changes I think deserve stronger scrutiny than the more mechanical aspects of this PR.
The mechanical changes to move from the old references to the new ones. This is the bulk of the change but is for the most part renames.

The benefits of this change are the following:

Have our test more closely mirror customer scenarios by using official reference assemblies.
Improve the readability of the tests by referring to assemblies by their target framework, which is familiar to most developers, instead of their file version, which is familiar to servicing team only.
Reduces the build output size of Roslyn by ~5GB

The internal PR is here https://github.com/dotnet/roslyn-internal/pull/1954

Note: I will be making more changes in this area in future PRs. But I wanted to keep the mechanical portion of this change into its own PR. Further PRs will be smaller but less mechanical.

This changes our test utilities library to use the standard reference assemblies for the desktop target frameworks that we support in our unit tests. This also upgrades us to a new version of the proprietary library where the duplicate references are removed

These were changes which were less mechanical or involevd re-targeting. These should be given a more thorough inspection than the more mechanical changes in the pull request

jaredpar · 2020-07-07T16:25:34Z

@dotnet/roslyn-compiler PTAL

jaredpar · 2020-07-07T16:26:41Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs

-                Diagnostic(ErrorCode.ERR_EscapeLocal, "local").WithArguments("local").WithLocation(14, 22),
-                // warning CS1685: The predefined type 'ExtensionAttribute' is defined in multiple assemblies in the global alias; using definition from 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089'
-                Diagnostic(ErrorCode.WRN_MultiplePredefTypes).WithArguments("System.Runtime.CompilerServices.ExtensionAttribute", "mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089").WithLocation(1, 1)
+                Diagnostic(ErrorCode.ERR_EscapeLocal, "local").WithArguments("local").WithLocation(14, 22)


The diagnostics removed in this file all just appear to be a result of mixing incorrect references. This changed fixed up the code to have the correct System.Core.dll reference and hence the diagnostics were removed.

jaredpar · 2020-07-07T16:27:16Z

src/Compilers/CSharp/Test/Symbol/DocumentationComments/CrefTests.cs

-                "SRETW");
+                "SRETW",
+                "Outer",
+                "Microsoft");


This appears to just be a result of the diff between the ref and impl assemblies we were using. Wanted someone else to look at this to verify though.

jaredpar · 2020-07-07T16:27:43Z

src/Compilers/CSharp/Test/Symbol/Symbols/Metadata/MetadataTypeTests.cs

-            Assert.Equal(8, type1.Interfaces().Length);
+            Assert.Equal(10, type1.Interfaces().Length);

            var fullName = "System.Collections.Generic.Dictionary<TKey, TValue>";


This are just differences between the frankenstein mscorlib we were using and the correct reference assembly.

jaredpar · 2020-07-07T16:29:44Z

src/Compilers/CSharp/Test/Symbol/Symbols/Metadata/MetadataTypeTests.cs

        }

-        [Fact]
+        [Fact(Skip = "PROTOTYPE")]


I'm unsure what to do with this test. The test here revolves around the type EventingTestBase. That is a type that only exists in the beta version of the .NET Framework, it was never shipped in any RTM release. Hence there is no direct port of this to the standard reference assemblies.

Is there an equivalent to this type that I can use in this test? Or if there isn't should we just delete the test? I'd much prefer finding an equivalent type to test against.

@AlekseyTs as you're most likely to have the context here.

Is there an equivalent to this type that I can use in this test? Or if there isn't should we just delete the test? I'd much prefer finding an equivalent type to test against.

From reading the original test, I believe it wasn't really intended to test something special about the EventProviderBase. It looks like the goal was to verify the API behaviors around array types imported from metadata, and the EventProviderBase type conveniently referenced types like that. I think we can simply define the EventProviderBase, as it used to be, in source in a separate compilation, then emit the compilation as image reference, add the reference, and test against that type instead.

In reply to: 450993234 [](ancestors = 450993234)

Took that approach and it looks to have worked out well. Thanks.

jaredpar · 2020-07-07T16:30:27Z

src/Compilers/VisualBasic/Test/Semantic/Binding/LookupTests.vb

            Dim ns = DirectCast(info.Symbol, NamespaceSymbol)

-            Assert.Equal(NamespaceKind.Compilation, ns.NamespaceKind)
+            Assert.Equal(NamespaceKind.Module, ns.NamespaceKind)


Wanted to make sure this change was reviewed and signed off on.

tmat · 2020-07-07T16:39:34Z

src/Test/Utilities/Portable/Generated.targets

+        <EmbeddedResource Include="$(Pkgjnm2_ReferenceAssemblies_net35)\build\.NETFramework\v3.5\\System.Core.dll">
+          <LogicalName>net35.System.Core.dll</LogicalName>
+        </EmbeddedResource>
+        <EmbeddedResource Include="$(PkgMicrosoft_NETFramework_ReferenceAssemblies_net40)\build\.NETFramework\v4.0\\mscorlib.dll">


\ [](start = 114, length = 1)

Extra \

tmat · 2020-07-07T16:41:03Z

    () => ModuleMetadata.CreateFromImage(TestResources.MetadataTests.NetModule01.ModuleCS00).GetReference(display: "ModuleCS00.mod"),

Nit: Indent.

Refers to: src/Test/Utilities/Portable/Mocks/TestReferences.cs:23 in 5e9a9f5. [](commit_id = 5e9a9f5, deletion_comment = False)

tmat · 2020-07-07T16:44:10Z

src/Compilers/Core/CodeAnalysisTest/MetadataReferences/MetadataReferenceTests.cs

        public void CreateFromImage()
        {
-            var r = MetadataReference.CreateFromImage(TestResources.NetFX.v4_0_30319.mscorlib);
+            var r = MetadataReference.CreateFromImage(ResourcesNet451.mscorlib);


ResourcesNet451 [](start = 54, length = 15)

Why not keep the original TestResources static class? E.g. TestResources.Net451

I initially tried that but eventually moved away because I wanted a clean separation between what was coming from the proprietary resource DLL and what was coming from the "new" approach.

How about TestMetadata? Or some other container type like that. Having it directly under Utilities namespace puts names like BuildExtensions and ValueTask out of context.

That name works for me.

jaredpar · 2020-07-07T16:45:00Z

Note: I will have a follow up change to do the same flip for our .NET Standard and .NET Core app references. This change was already getting unweildy though hence wanted to separate the efforts

tmat · 2020-07-07T16:47:48Z

src/Test/Utilities/Portable/Generated.cs

+        private static byte[] _NetStandard461;
+        public static byte[] NetStandard461 => ResourceLoader.GetOrCreateResource(ref _NetStandard461, "BuildExtensions.netstandard.dll");
+    }
+    public static class BuildExtensions


BuildExtensions [](start = 24, length = 15)

This name does not indicate that it's a PE reference created for metadata in a resource.

Have a better suggestion here? I'd prefer one that we could apply as a pattern to these one off reference cases. There are two right now but looking forward there will likely be 1-2 more. Hence if we had a nice prefix or suffix it could be applied to all of them.

As noted above I'd encapsulate all of these types in something like TestMetadata static class. Then the names are ok as nested types.

jaredpar · 2020-07-08T00:43:34Z

@tmat addressed your feedback

tmat

chsienki

LGTM, Modulo test failures

ghost

Auto-approval

AlekseyTs · 2020-07-08T18:28:35Z

src/Compilers/CSharp/Test/Symbol/Symbols/Metadata/MetadataTypeTests.cs

+            compilation1.VerifyDiagnostics();
+
+            var source2 = "public class A {}";
+            var compilation2 = CreateEmptyCompilation(source2, new MetadataReference[] { TestMetadata.Net40.mscorlib, TestMetadata.Net40.SystemCore, compilation1.ToMetadataReference() },


ToMetadataReference [](start = 162, length = 19)

This should be EmitToImageReference in order to test metadata types #Closed

jaredpar added the Area-Compilers label Jul 6, 2020

jaredpar force-pushed the testref2 branch 3 times, most recently from 5d28894 to d4f227f Compare July 7, 2020 06:49

jaredpar added 3 commits July 7, 2020 09:24

Move to standard references

5e9a9f5

This changes our test utilities library to use the standard reference assemblies for the desktop target frameworks that we support in our unit tests. This also upgrades us to a new version of the proprietary library where the duplicate references are removed

Changes to review

30cde77

These were changes which were less mechanical or involevd re-targeting. These should be given a more thorough inspection than the more mechanical changes in the pull request

Mostly mechanical changes

c015527

jaredpar force-pushed the testref2 branch from d4f227f to c015527 Compare July 7, 2020 16:24

jaredpar marked this pull request as ready for review July 7, 2020 16:25

jaredpar requested review from a team as code owners July 7, 2020 16:25

jaredpar commented Jul 7, 2020

View reviewed changes

tmat reviewed Jul 7, 2020

View reviewed changes

PR feedback

fe28d67

tmat approved these changes Jul 8, 2020

View reviewed changes

chsienki approved these changes Jul 8, 2020

View reviewed changes

jaredpar added 3 commits July 7, 2020 21:04

Merge remote-tracking branch 'origin/master' into testref2

bb0fd61

Post merge fixup

e17dc0a

Formatting change

673a900

jaredpar added the auto-merge label Jul 8, 2020

ghost approved these changes Jul 8, 2020

View reviewed changes

jaredpar removed the auto-merge label Jul 8, 2020

Fix x86 test failure

bcca1d6

AlekseyTs reviewed Jul 8, 2020

View reviewed changes

PR feedback

2de683b

jaredpar merged commit 6ebca39 into dotnet:master Jul 8, 2020

ghost added this to the Next milestone Jul 8, 2020

jaredpar deleted the testref2 branch July 8, 2020 23:02

JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020

Conversation

jaredpar commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Jul 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat commented Jul 7, 2020

Uh oh!

tmat Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Jul 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Jul 8, 2020

Uh oh!

tmat left a comment

Choose a reason for hiding this comment

Uh oh!

chsienki left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jaredpar commented Jul 6, 2020 •

edited

Loading

tmat Jul 7, 2020 •

edited

Loading

AlekseyTs Jul 8, 2020 •

edited

Loading