Build Micrososoft.Extensions.DependencyInjection.Specification.Tests package #48858

ericstj · 2021-02-27T00:23:39Z

This brings back the separate assembly and package for DI.Specification.Tests.

I've removed all the ActiveIssue attributes in order to get new data from CI to see if these are all still failing on mono or not. If so I will see see about adding attributes to the derived tests classes in DI.External.Tests.

We need #48385 to complete the packaging side of this.

…ckage

ghost · 2021-02-27T00:23:47Z

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #37108

This brings back the separate assembly and package for DI.Specification.Tests.

I've removed all the ActiveIssue attributes in order to get new data from CI to see if these are all still failing on mono or not. If so I will see see about adding attributes to the derived tests classes in DI.External.Tests.

We need #48385 to complete the packaging side of this.

Author:	ericstj
Assignees:	-
Labels:	`area-Extensions-DependencyInjection`
Milestone:	-

ericstj · 2021-02-27T03:19:05Z

Looks like I had a merge issue when rebasing. Will fix.

ericstj · 2021-02-27T03:23:24Z

Yep, this change came in which adds an assembly dependency to a test assembly to this: 21ff47c

We will need to decide how to handle that if we still want to package this.

....Specification.Tests/src/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj

ViktorHofer · 2021-03-01T17:18:20Z

....Specification.Tests/src/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj

+    <EnableDefaultItems>true</EnableDefaultItems>
+    <CLSCompliant>false</CLSCompliant>
+    <IsPackable>true</IsPackable>
+    <TargetsForTfmSpecificBuildOutput>$(TargetsForTfmSpecificBuildOutput);IncludeProjectReferencesInPackage</TargetsForTfmSpecificBuildOutput>


nit: Consider encapsulating the P2P as private asset functionality as a target that can be enabled via an msbuild property.

I thought about this, but expect that @Anipik will probably have some more generic implementation based on other package requirements (EG: packaging content from other projects in different paths). I suspect analyzers/source-generators will hit this.

I'm fine with that but let's make sure that this specific code path then gets replaced by the more generic implementation. Unsure how to best track this.

@Anipik did you have a general-purpose pattern in mind for CSProj's that want to include the output of other projects?

Do we really want to do include Microsoft.Extensions.DependencyInjection.Abstractions in this package?

DI.Abstractions is added as a package dependency and not inlined into the package as its ProjectReference doesn't set Pack=true.

.../Microsoft.Extensions.DependencyInjection.Specification.Tests/src/ActivatorUtilitiesTests.cs

eerhardt · 2021-03-01T19:59:16Z

...s/DI.External.Tests/Microsoft.Extensions.DependencyInjection.ExternalContainers.Tests.csproj

  <ItemGroup>
    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.DependencyInjection\src\Microsoft.Extensions.DependencyInjection.csproj" SkipUseReferenceAssembly="true" />
    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.DependencyInjection.Abstractions\src\Microsoft.Extensions.DependencyInjection.Abstractions.csproj" SkipUseReferenceAssembly="true" />
+    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.DependencyInjection.Specification.Tests\src\Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj"/>


Do the tests in Microsoft.Extensions.DependencyInjection.Specification.Tests still get executed in our CI when we P2P them?

They are all abstract base classes. The derived classes in other assemblies are what execute them.

ericstj · 2021-03-02T17:25:20Z

Cool, so I was able to enable most of these test on mono. The only place I needed to suppress the tests were the External DI tests were failing on the mono interpreter: both desktop and WASM. This could be an interpreter bug, or could be a problem with one of the External DI implementations. I added conditions to the tests which made use of threading so that they wouldn't run on platforms where threading was absent.

I think this is ready to merge. @eerhardt can you confirm you're OK with this as implemented?

eerhardt

Looks good to me

...ies/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs

briansull · 2021-03-03T02:36:52Z

This checkin may have regressed a Generate CORE_ROOT task

104 / 261 (38%, 0 failed): launching: D:\workspace\_work\1\s\artifacts\bin\coreclr\windows.x64.Checked\crossgen.exe @D:\workspace\_work\1\s\artifacts\tests\coreclr\obj\windows.x64.Checked\crossgen.out\Microsoft.Extensions.DependencyInjection.Specification.Tests.dll.rsp Error: Could not load file or assembly 'System.TestStructs, Version=1.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified. (0x80070002 (COR_E_FILENOTFOUND))

https://dev.azure.com/dnceng/public/_build/results?buildId=1019715&view=logs&j=232206c9-9d3e-5b9e-c32e-6a35f74015d5&t=7fe4b998-5c8d-5e17-aa6c-5161847ae8fe&l=583

…n.Tests package (#48858)" This reverts commit 039b148.

ericstj · 2021-03-03T16:39:44Z

BUILDTEST: Running crossgen on framework assemblies in CORE_ROOT: D:\workspace\_work\1\s\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root
Running "D:\workspace\_work\1\s\dotnet.cmd" "D:\workspace\_work\1\s\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\R2RTest\R2RTest.dll" compile-framework -cr "D:\workspace\_work\1\s\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root" --output-directory "D:\workspace\_work\1\s\artifacts\tests\coreclr\obj\windows.x64.Checked\crossgen.out" --release --nocleanup --target-arch x64 -dop 4 --large-bubble --crossgen2-parallelism 1 --crossgen --nocrossgen2 --crossgen-path "D:\workspace\_work\1\s\artifacts\bin\coreclr\windows.x64.Checked\crossgen.exe"
...
Failed to crossgen the framework
BUILDTEST: Error: crossgen precompilation of framework assemblies failed

@briansull do you know where the input to this task is determined? The problem here is that it's considering this as "framework" but it is not, it's a shipping test assembly.

briansull · 2021-03-03T16:48:41Z

I don't know. The task probably considers everything in the CORE_ROOT directory as a candidate for crossgen2.
@BruceForstall

ericstj · 2021-03-03T16:59:38Z

I think it gets them from here:
Downloading artifact libraries_bin_windows_x64_Release from: https://dev.azure.com/dnceng//9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/1019715/artifacts?artifactName=libraries_bin_windows_x64_Release&$format=zip

Which is oddly a zip inside a zip. Inside here I see the test binary in bin\runtime\net6.0-windows-Release-x64. So the fix is not to binplace there.

ericstj · 2021-03-03T17:08:13Z

Here's what's applying:

runtime/src/libraries/Directory.Build.targets

Lines 63 to 69 in 6ec82c5

    
           <!-- binplace to directories for the target vertical --> 
        
           <BinPlaceTargetFrameworks Include="$(NetCoreAppCurrent)-$(TargetOS)" 
        
                                     Condition="'$(BinPlaceForTargetVertical)' == 'true'"> 
        
             <NativePath>$(NetCoreAppCurrentRuntimePath)</NativePath> 
        
             <RefPath>$(NetCoreAppCurrentRefPath)</RefPath> 
        
             <RuntimePath>$(NetCoreAppCurrentRuntimePath)</RuntimePath> 
        
           </BinPlaceTargetFrameworks>

briansull · 2021-03-03T17:10:38Z

@ericstj Is the exact name of the assembly that we don't want to crossgen this: System.TestStructs.dll

I see a place where we can add it to the exclude list

briansull · 2021-03-03T17:14:37Z

I submitted this PR
#49064 - Added System.TestStructs.dll to the list of excluded assemblies in CORE_ROOT

ViktorHofer · 2021-03-03T17:31:28Z

I don't know. The task probably considers everything in the CORE_ROOT directory as a candidate for crossgen2.

Why are libraries that aren't part of the shared framework being crossgend? It looks like coreclr currently relies on a "all libraries produced" representation which doesn't exist in the product. There are libraries that are part of the shared framework and then there are libraries that are part of packages.

BruceForstall · 2021-03-03T18:20:37Z

Yes, anything inside the CORE_ROOT folder gets crossgen'ed. The basic idea with these tests is to run all code (including tests) from crossgen'ed / R2R assemblies.

@trylek @janvorli @davidwrighton would know about the implementation of the R2RTest driver of this "crossgen CORE_ROOT" process.

BruceForstall · 2021-03-03T18:22:25Z

We should probably have at least one R2R leg in our PR testing, as well.

trylek · 2021-03-03T18:56:59Z

For "implementation of the R2RTest driver", there's not much difference compared to the previous script-based implementation - that one was also basically iterating over all assemblies in CORE_ROOT and just skipping native ones. The number of explicit exclusions still seems pretty low compared to maintaining a list of all eligible assemblies but if someone has ideas how to make it more dynamic, I can easily take a look.

For Bruce's comment about R2R leg in PR testing, I basically concur. When I was making the Crossgen2 changes, I didn't introduce a new dedicated leg because we hadn't had any Crossgen1-specific leg in PR runs before and with the COVID-related lab capacity limitations it simply didn't seem like the ideal time for adding completely new PR legs. We do run a handful of Crossgen2 tests in PR's even today but I agree that a full leg would make them more reliable w.r.t. validation of Crossgen2-specific changes.

BruceForstall · 2021-03-03T18:59:11Z

Even if we have a few R2R tests in PR, the one thing we're missing is any leg that crossgen compiles the CORE_ROOT directory.

trylek · 2021-03-03T19:13:38Z

This is an interesting observation - just unconditionally crossgening CORE_ROOT would take just a few minutes and would be definitely much faster than adding a full-fledged R2R leg crossgenning all the tests; moreover that's a one-line YAML script change I can easily make .

BruceForstall · 2021-03-03T19:27:42Z

We probably don't want to alter any existing leg (or maybe we should be always R2R compiling the framework, and let our ZapDisable stress legs ignore the AOT images to give us JIT-compile coverage of the libraries?), but we could think about how to add this additional coverage.

ericstj · 2021-03-03T19:29:52Z

@trylek make sure we answer the question about what needs to be part of CORE_ROOT that @ViktorHofer brought up.

Now that all tests copy locally dependencies that are not part of the shared framework perhaps we don’t need to use the entire set of runtime assemblies for CORE_ROOT?

ViktorHofer · 2021-03-03T19:32:09Z

IIRC there are very few runtime tests that need OOB APIs like System.Drawing.Common, I believe only two or so. Bit unrelated but important for the discussion of what should go into CORE_ROOT at all.

trylek · 2021-03-03T20:19:16Z

@ericstj / @ViktorHofer - looks like there are several questions here:

Why are libraries that aren't part of the shared framework being crossgend? It looks like coreclr currently relies on a "all libraries produced" representation which doesn't exist in the product. There are libraries that are part of the shared framework and then there are libraries that are part of packages.

Bit unrelated but important for the discussion of what should go into CORE_ROOT at all.

I don't really have any good answer to that beyond that's something we haven't touched semantically in the Crossgen2 work. As I described above, previous R2R logic used super-simple scripting to basically "Crossgen[1]-compile everything in CORE_ROOT and silently ignore all errors caused by hitting native DLL's'" - I agree that's hacky, unreliable and flaky to some extent and perhaps merit cleaning up.

Cleaning this up can have several forms: as Viktor pointed out, maybe the CORE_ROOT is an overused sink of garbage that was just convenient to put in one place even though it consists of logically distinct components (CoreCLR runtime, JIT, DBI / DAC components, Win32 API contract DLLs, framework assemblies, xunit.console used internally as the test harness), maybe some more.

Making its content more structured, perhaps by using subfolders (like we already do for certain components including Crossgen2), may have positive effect on its overall management including filtering as to what gets included in Helix correlation payloads which now uses one other ad hoc hardcoded list of folders and files. I think this would be a fine thing to add as a small to medium-size (2 work-weeks or so) CoreCLR infra backlog item, potentially up for grabs.

One other thing is how we identify the list of assemblies to Crossgen. Today logic of first copying the assemblies to CORE_ROOT and only then running Crossgen(1|2) on top of them is also quite hacky, unreliable and potentially non-idempotent. A cleaner way to tackle this would be to Crossgen them directly as part of copying them from artifacts/bin to CORE_ROOT. That is also a good suggestion for a one-week CoreCLR infra work item up for grabs. Two things that complicate this are a) that it's necessary to fix this in two places for the cmd | sh build scripts, and b) that the scripts have tons of "partial execution" options that need consolidating with this optimized functionality.

Even with both these cleanups in place, we still haven't answered the question of the "framework extent" present in CORE_ROOT. In fact I noticed that many devs believe that only System.Private.CoreLib is needed to run CoreCLR tests. That is obviously not the case and the set of assemblies actually needed by the tests is a great unknown that probably also happened historically by itself, so to say.

It would be a very interesting CoreCLR infra design item to think about how to make this more explicit - for instance by explicitly specifying reference assemblies on top of some well-defined minimal set in the test projects. That would let us reason much better about the needed assembly set and its propagation to CORE_ROOT. Having said that, composing CORE_ROOT in such an incremental manner would imply one more iteration over all the 10K Pri1 test build projects which is not ideal perf-wise.

In the past, IIRC @ivdiazsa and @sdmaclea worked on making the test projects more expressive in the sense of containing more property "metadata" letting the test build system dynamically decide what to do without having to stick to hard-coded lists and blindly copying around tons of unused assemblies.

Thanks

Tomas

ericstj · 2021-03-03T20:33:21Z

At least what I'm looking at is not garbage, at least not the portion that this infrastructure is using. It chose to use the library folder that contains all assemblies built in the vertical vs a different folder we already produce that contains only shared framework assemblies. We're not suggesting a huge investigation here or any large investment, just consider using a different folder.

So instead of artifacts\bin\runtime\net6.0-windows-Debug-x64

runtime/src/libraries/Directory.Build.targets

Line 68 in 6ec82c5

<RuntimePath>$(NetCoreAppCurrentRuntimePath)</RuntimePath>

Use artifacts\bin\pkg\net6.0\runtime\windows-Debug-x64

runtime/src/libraries/Directory.Build.targets

Line 79 in 6ec82c5

    
           <RuntimePath Condition="'$(IsNETCoreAppSrc)' == 'true'">$(NETCoreAppPackageRuntimePath)\..\runtime\$(TargetOS)-$(Configuration)-$(TargetArchitecture)</RuntimePath>

If you're already consuming test directories which carry the individual test assemblies then the former is redundant and latter is all that's needed. This clean-up was something @ViktorHofer already did when changing what we binplace in the libraries test shared framework, maybe this is just something that needs to be copied into the coreclr infra?

trylek · 2021-03-03T20:49:23Z

In such case we need to figure out how that maps onto CoreCLR test build. From what I recall, we start out by running the target CopyDependencyToCoreRoot which is implemented in three different places:

runtime/src/tests/Directory.Build.targets

Line 262 in 79ae74f

<Target Name="CopyDependencyToCoreRoot"

runtime/src/tests/Common/Directory.Build.targets

Line 28 in 79ae74f

<Target Name="CopyDependencyToCoreRoot"

runtime/src/tests/Common/publishdependency.targets

Line 28 in 79ae74f

I am not completely clear on their exact interplay but that's something we would need to figure out and somehow consolidate. IIRC from my work on the CoreCLR tests last year, src/tests/Directory.Build.targets is used by the actual tests whereas src/tests/Common/Directory.Build.targets is used by the auxiliary utilities internally used by the test harness that are also located under the Common folder (hostpolicymock, CoreCLRTestLibrary, ilasm etc.). The publishdependencytargets is a Jarret's tombstone, I think it's either completely unused or at least it has a F# part that's never been used.

That's the first step and then, five hundred lines below in the build.cmd / sh script, we optionally run Crossgen2 on the pre-populated CORE_ROOT folder. I believe that today we simply have no knowing by just looking at the CoreCLR test scripts what dependencies they use. If we could somehow deduce this information from the Roslyn compilation of the tests, we could leverage it in the subsequent phase of populating CORE_ROOT even though we need to keep in mind that YAML scripts run these phases separately and in this particular case even on different machines.

sdmaclea · 2021-03-03T20:58:53Z

For testing, crossgen on lots of assemblies is interesting. In the past a few assemblies would trigger crossgen errors. Since crossgen tool will be used on user code too, the extra coverage is nice to have.

I don't mean to change any direction in this thread, just make sure we don't lose lots of coverage accidentally.

ericstj added 2 commits February 26, 2021 16:14

Build Microsoft.Extensions.DependencyInjection.Specification.Tests pa…

66afcdf

…ckage

Move specification tests source and reference it.

d6ec9e7

ericstj requested a review from maryamariyan February 27, 2021 00:23

ghost added the area-Extensions-DependencyInjection label Feb 27, 2021

ericstj requested review from Anipik and ViktorHofer February 27, 2021 00:23

Base automatically changed from master to main March 1, 2021 09:08

Reference and include TestStructs.dll in package

9b94958

ViktorHofer reviewed Mar 1, 2021

View reviewed changes

....Specification.Tests/src/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj Show resolved Hide resolved

ViktorHofer reviewed Mar 1, 2021

View reviewed changes

....Specification.Tests/src/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Mar 1, 2021

View reviewed changes

ViktorHofer approved these changes Mar 1, 2021

View reviewed changes

Anipik approved these changes Mar 1, 2021

View reviewed changes

Fix DI.Tests build

919296e

maryamariyan approved these changes Mar 1, 2021

View reviewed changes

.../Microsoft.Extensions.DependencyInjection.Specification.Tests/src/ActivatorUtilitiesTests.cs Show resolved Hide resolved

eerhardt reviewed Mar 1, 2021

View reviewed changes

ericstj mentioned this pull request Mar 2, 2021

Segfault when running Microsoft.Extensions.DependencyInjection.ExternalContainers.Tests on mono interpreter #48929

Closed

ericstj added 2 commits March 1, 2021 16:33

Respond to PR feedback and fix tests.

caf747c

Disable DI external tests on WASM as well

24c333a

runfoapp bot mentioned this pull request Mar 2, 2021

Inability to unzip assets during build on Unix x64 #32805

Closed

eerhardt approved these changes Mar 2, 2021

View reviewed changes

...ies/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs Show resolved Hide resolved

ericstj merged commit 039b148 into dotnet:main Mar 2, 2021

briansull mentioned this pull request Mar 3, 2021

Don't use the CoercedConstantValue value as the key, when we have a reloc #49029

Merged

hoyosjs added a commit that referenced this pull request Mar 3, 2021

Revert "Build Micrososoft.Extensions.DependencyInjection.Specificatio…

225a488

…n.Tests package (#48858)" This reverts commit 039b148.

hoyosjs mentioned this pull request Mar 3, 2021

Revert "Build Micrososoft.Extensions.DependencyInjection.Specification.Tests package" #49046

Closed

This was referenced Mar 3, 2021

Disable binplacing for DependencyInjection.Specification.Tests #49067

Merged

Added System.TestStructs.dll to the list of excluded assemblies in CORE_ROOT #49064

Closed

ericstj mentioned this pull request Mar 3, 2021

System.TestStructs ships with Micrososoft.Extensions.DependencyInjection.Specification.Tests but builds from test directory #49069

Closed

ghost locked as resolved and limited conversation to collaborators Apr 2, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Build Micrososoft.Extensions.DependencyInjection.Specification.Tests package #48858

Build Micrososoft.Extensions.DependencyInjection.Specification.Tests package #48858

Uh oh!

Conversation

ericstj commented Feb 27, 2021

Uh oh!

ghost commented Feb 27, 2021

Uh oh!

ericstj commented Feb 27, 2021

Uh oh!

ericstj commented Feb 27, 2021

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj commented Mar 2, 2021

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

briansull commented Mar 3, 2021

Uh oh!

ericstj commented Mar 3, 2021

Uh oh!

briansull commented Mar 3, 2021

Uh oh!

ericstj commented Mar 3, 2021

Uh oh!

ericstj commented Mar 3, 2021

Uh oh!

briansull commented Mar 3, 2021

Uh oh!

briansull commented Mar 3, 2021

Uh oh!

ViktorHofer commented Mar 3, 2021

Uh oh!

BruceForstall commented Mar 3, 2021

Uh oh!

BruceForstall commented Mar 3, 2021

Uh oh!

trylek commented Mar 3, 2021

Uh oh!

BruceForstall commented Mar 3, 2021

Uh oh!

trylek commented Mar 3, 2021

Uh oh!

BruceForstall commented Mar 3, 2021

Uh oh!

ericstj commented Mar 3, 2021

Uh oh!

ViktorHofer commented Mar 3, 2021

Uh oh!

trylek commented Mar 3, 2021

Uh oh!

ericstj commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trylek commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdmaclea commented Mar 3, 2021

Uh oh!

Reviewers

ericstj commented Mar 3, 2021 •

edited

Loading

trylek commented Mar 3, 2021 •

edited

Loading