Update xunit to official 2.4.0 runner by ViktorHofer · Pull Request #31260 · dotnet/corefx

ViktorHofer · 2018-07-23T00:44:05Z

Fixes https://github.com/dotnet/corefx/issues/31319
Fixes https://github.com/dotnet/corefx/issues/31761
Fixes https://github.com/dotnet/corefx/issues/31886
Fixes https://github.com/dotnet/corefx/issues/21483

Update XUnit runner to 2.4.0

Depends on package publish of dotnet/arcade#352
Depends on dotnet/buildtools#2106
Depends on package publish of microsoft/xunit-performance#269 (which would depend on commandlineparser/commandline#307 but I have pushed a package to nuget till the PR is merged and an updated package is uploaded)
Depends on package publish of microsoft/xunit-performance#270 (same but for Wheelies/MarkdownLog#16)

Scenarios tested:

Update XUnit UWP runner

Depends on dotnet/arcade#355

Runs the uap tests in a console application which supports multi-instancing. Remove specific UWP RemoteExecutor code in corefx and the whole service project in buildtools
(https://github.com/dotnet/buildtools/tree/master/src/xunit.runner.uap - I won't delete that until everything's merged). This allows CI integration of our uap test suite and also enables setting different cultures as RemoteExecutor code now truly lives in a separate process instead of a single process.

Scenarios tested:

uap
CI

Enable Perf Tests for NetFX and promote perf projects to netstandard where applicable (95%).
Add UAP test leg to CI

cc @danmosemsft

weshaggard · 2018-07-23T16:50:43Z

  <PropertyGroup>
    <RestoreSources Condition="'$(DotNetBuildOffline)' != 'true'">
      https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json;
+      https://dotnetfeed.blob.core.windows.net/dotnet-tools-internal/index.json;


We should put this package on an existing feed instead of adding this feed to the list.

safern · 2018-07-26T21:53:50Z

-        Include="$(PackagesDir)$(XUnitRunnerPackageId)\$(XUnitPackageVersion)\tools\*.*"
-        Exclude="$(PackagesDir)$(XUnitRunnerPackageId)\$(XUnitPackageVersion)\tools\xunit.console.exe.config"
+        Include="$(PackagesDir)$(XUnitRunnerPackageId)\$(XUnitPackageVersion)\tools\net472\*.*"
+        Exclude="$(PackagesDir)$(XUnitRunnerPackageId)\$(XUnitPackageVersion)\tools\net472\xunit.console.exe.config;$(PackagesDir)$(XUnitRunnerPackageId)\$(XUnitPackageVersion)\tools\net472\xunit.console.x86.exe.config"


This can be written with * -- net472\xunit.console.*exe.config?

safern · 2018-07-26T22:06:18Z

-    <TargetsNetStandard Condition="$(TargetGroup.StartsWith('netstandard'))">true</TargetsNetStandard>
-    <TargetsNetCoreApp Condition="$(TargetGroup.StartsWith('netcoreapp'))">true</TargetsNetCoreApp>
    <TargetsAot Condition="$(TargetGroup.EndsWith('aot'))">true</TargetsAot>
+    <!-- <TargetFrameworkIdentifier Condition="'$(TargetsNetCoreApp)' == 'true' OR '$(TargetsUap)' == 'true' OR '$(TargetsAot)' == 'true'">.NETCoreApp</TargetFrameworkIdentifier> -->


Why are we adding this comments?

joperezr · 2018-08-22T19:00:15Z

  <PropertyGroup>
    <UAPvNextVersion>10.0.16300</UAPvNextVersion>
-    <UAPvNextTFMFull>UAP,Version=v$(UAPvNextVersion)</UAPvNextTFMFull>
+    <UAPvNextTFI>UAP</UAPvNextTFI>


Why did we have to do this? do we expect the Moniker name to change?

I just felt like it make sense to extract it and avoid the duplication. I don't know if it will ever change...

joperezr · 2018-08-22T19:01:27Z

    <!-- Output directories -->
    <BinDir Condition="'$(BinDir)'==''">$(ProjectDir)bin/</BinDir>
-
-    <ObjDir Condition="'$(ObjDir)'==''">$(BinDir)obj/</ObjDir>


Makes me a bit worried the fact that we are not defining this property anymore. I wonder if there will be any side effects like stuff binplaced now on the source tree.

ObjDir isn't used anywhere in buildtools

weshaggard · 2018-08-22T21:46:28Z

@@ -0,0 +1,105 @@
+Microsoft (R) Build Engine version 15.7.179.6572 for .NET Framework
+Copyright (C) Microsoft Corporation. All rights reserved.


Looks like you accidentally included this file.

It was me 😆 I just deleted it in the last commit.

ViktorHofer · 2018-08-23T10:24:17Z

@dotnet-bot test OSX x64 Debug Build please

ViktorHofer · 2018-08-23T14:06:27Z

This PR is now ready to be merged as soon as microsoft.dotnet.uap.testtools.1.0.19.nupkg are pushed to our myget feed and CI is green.

ViktorHofer · 2018-08-23T17:10:00Z

@dotnet-bot test UWP CoreCLR x64 Debug Build please

safern · 2018-08-23T17:16:10Z

+    TODO: Fix VB test project reference to Test SDK.
+  -->
+  <Import Condition="'$(IncludeVSTestReferences)' == 'true' AND '$(Language)' != 'VB'" Project="$(PackagesDir)\microsoft.net.test.sdk\$(MicrosoftDotNetTestSdkVersion)\buildMultiTargeting\Microsoft.Net.Test.Sdk.props" />
+  <Import Condition="'$(IncludeVSTestReferences)' == 'true' AND ('$(BuildingNETCoreAppVertical)' == 'true' OR '$(BuildingNETFxVertical)' == 'true')" Project="$(PackagesDir)\microsoft.codecoverage\$(MicrosoftDotNetTestSdkVersion)\build\netstandard1.0\Microsoft.CodeCoverage.props" />


Should we put this condition into a property to avoid duplication?

Same as XUnitExtensions. Not going to do this now, let's consider it for a later clean up.

safern · 2018-08-23T17:17:20Z

 - `nonosxtests` for tests that don't run on OS X

-**[Available Test Platforms](https://github.com/dotnet/buildtools/blob/master/src/xunit.netcore.extensions/TestPlatforms.cs#L10)**
+**[Available Test Platforms](https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.XUnitExtensions/TestPlatforms.cs#L10)**


Should this point to arcade? Or not yet?

safern · 2018-08-23T17:20:37Z

@@ -185,8 +188,8 @@
    </UpdateStep>
    <XmlUpdateStep Include="BuildTools">


Isn't this package coming from arcade now?

safern · 2018-08-23T17:22:20Z

    </PackageReference>
-    <PackageReference Include="Microsoft.DotNet.BuildTools.TestSuite">
-      <Version>$(XunitNetcoreExtensionsVersion)</Version>
+    <PackageReference Include="Microsoft.DotNet.XUnitExtensions">


I feel like this package id is used in various places, should we define a property for it?

Will not converge these for now as I don't think it's worth introducing a separate property. Let's deal with this at a later point.

I think it is worth because if at some point we rename this package again, we just change one property instead of 20 different sites.

we use it in 3 places in corefx and once in buildtools. I don't want to touch it now as we are moving the test framework to arcade and this will change. also I don't want to change buildtools again.

Fair enough. Something to keep in mind for next iteration though.

safern · 2018-08-23T17:26:59Z

    <Reference Include="System.Runtime.InteropServices" />
    <ReferenceFromRuntime Include="xunit.core" />
-    <ReferenceFromRuntime Include="Xunit.NetCore.Extensions" />
+    <ReferenceFromRuntime Include="Microsoft.DotNet.XUnitExtensions" />


Another usage of this guy.

safern · 2018-08-23T17:27:31Z

-        protected  static readonly string HostRunner = TestConsoleApp;
-
-        private static readonly string ExtraParameter = "";
+        protected static readonly string HostRunnerName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "dotnet.exe" : "dotnet";


Should we use PlatformDetection instead?

https://github.com/dotnet/corefx/blob/master/src/CoreFx.Private.TestUtilities/src/System/PlatformDetection.Windows.cs#L73

Add necessary Test SDK assemblies Enable test sdk props & targets only for netcoreapp

safern · 2018-08-23T17:58:53Z

  <PropertyGroup>
    <BuildConfigurations>
-      netcoreapp;
+      netstandard;


Why are we changing the perf tests configurations to netstandard?

Discussed offline. Promoting them to netstandard allows running in different configurations.

safern · 2018-08-23T18:01:18Z

    public partial class ProcessTests : ProcessTestBase
    {
        [Fact]
+        [ActiveIssue(31908, TargetFrameworkMonikers.Uap)]


Should this be added to the class instead?

As it's a partial class I didn't put it on the class as that leads to disabling all the ProcessTests for uap.

But you're already adding it to ProcessTests.cs (Also weird enough, this file is named .netcoreapp.cs and disabling tests for uap in here?)

oh you are right. the netcoreapp file is also used in uap...

safern

Some minor feedback and questions. Once addressed LGTM.

Disable/Enable tests for clean uap CI leg

weshaggard · 2018-08-23T18:24:42Z

    <!-- uap is an alias for uapvNext any/coreclr runtime -->
    <TargetGroups Include="uap">
+      <TargetFramework>$(UAPvNextTFM)</TargetFramework>
+      <TargetFrameworkIdentifier>$(UAPvNextTFI)</TargetFrameworkIdentifier>


Why do you need the Identifier and Version in some both not others?

They are currently only inferred by the SDK for netcoreapp, netstandard and net: https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets#L18

ViktorHofer · 2018-08-23T19:16:55Z

@dotnet-bot test this please

4creators · 2018-08-24T09:25:15Z

@ViktorHofer Any plans on upgrading xunit to 2.4.0 in coreclr repo?

ViktorHofer · 2018-08-24T12:45:21Z

Isn't coreclr already using xunit 2.4? https://github.com/dotnet/coreclr/blob/master/dependencies.props#L41

ahsonkhan · 2018-10-24T20:12:43Z

 **Step # 2:** Change directory to the performance tests directory: ```cd path/to/library/tests/Performance```

 **Step # 3:** Build and run the tests:
- - Windows (using admin command shell): ```msbuild /t:BuildAndTest /p:Performance=true /p:ConfigurationGroup=Release /p:TargetOS=Windows_NT```


I am curious, why was /p:Performance=true removed from the instructions here?

It looks like we need it to actually run the tests. Otherwise, they just build.

Without:

E:\GitHub\Fork\corefx\src\System.Memory\tests\Performance>dotnet msbuild /t:BuildAndTest /p:ConfigurationGroup=Release Microsoft (R) Build Engine version 15.9.19+g938f3292a0 for .NET Core Copyright (C) Microsoft Corporation. All rights reserved. PerfRunner -> E:\GitHub\Fork\corefx\bin\AnyOS.AnyCPU.Release\PerfRunner\netstandard\PerfRunner.exe System.Memory.Performance.Tests -> E:\GitHub\Fork\corefx\bin\AnyOS.AnyCPU.Release\System.Memory.Performance.Tests\netcoreapp\System.Memory.Performance.Tests.dll

With:

E:\GitHub\Fork\corefx\src\System.Memory\tests\Performance>dotnet msbuild /t:BuildAndTest /p:ConfigurationGroup=Release /p:Performance=true Microsoft (R) Build Engine version 15.9.19+g938f3292a0 for .NET Core Copyright (C) Microsoft Corporation. All rights reserved. PerfRunner -> E:\GitHub\Fork\corefx\bin\AnyOS.AnyCPU.Release\PerfRunner\netstandard\PerfRunner.exe System.Memory.Performance.Tests -> E:\GitHub\Fork\corefx\bin\AnyOS.AnyCPU.Release\System.Memory.Performance.Tests\netcoreapp\System.Memory.Performance.Tests.dll Using E:\GitHub\Fork\corefx\bin\testhost\netcoreapp-Windows_NT-Release-x64\ as the test runtime folder. Executing in E:\GitHub\Fork\corefx\bin\tests\System.Memory.Performance.Tests\netcoreapp-Windows_NT-Release-x64\ ----- start 13:11:21.67 =============== To repro directly: ===================================================== ... results ...

Should we add it back?

I removed it from the docs because with that PR I enabled running perf test projects without the /p:Performance=true flag. Unfortunately @weshaggard had to default the performance flag to false when he switched to the new bootstrapping model: 66392f5#diff-4b2f402501d23abbede6eac0f47783e4R127

I'll update the docs.

krwq · 2018-10-24T23:55:22Z

  <PropertyGroup>
    <UAPvNextVersion>10.0.16300</UAPvNextVersion>
-    <UAPvNextTFMFull>UAP,Version=v$(UAPvNextVersion)</UAPvNextTFMFull>
+    <UAPvNextTFI>UAP</UAPvNextTFI>


out of curiosity: what's TFI? Target Framework Identifier?

ViktorHofer added area-Infrastructure-libraries * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) labels Jul 23, 2018

ViktorHofer self-assigned this Jul 23, 2018

ViktorHofer requested review from safern and weshaggard July 23, 2018 00:44

ViktorHofer mentioned this pull request Jul 23, 2018

Remove xunit test assets from buildtools dotnet/buildtools#2107

Merged

ViktorHofer force-pushed the XUnitRunner branch 3 times, most recently from 3a8ab75 to d3b500d Compare July 23, 2018 14:30

weshaggard reviewed Jul 23, 2018

View reviewed changes

ViktorHofer force-pushed the XUnitRunner branch from d3b500d to 5edae94 Compare July 23, 2018 23:57

ViktorHofer mentioned this pull request Jul 25, 2018

Make BaseIntermediateOutputPath project specific #31364

Closed

safern reviewed Jul 26, 2018

View reviewed changes

ViktorHofer force-pushed the XUnitRunner branch 16 times, most recently from 8b79f9b to e40c121 Compare August 12, 2018 17:22

joperezr reviewed Aug 22, 2018

View reviewed changes

ViktorHofer mentioned this pull request Aug 22, 2018

Update BuildTools, CoreClr, CoreFx, CoreSetup, ProjectNTfs, ProjectNTfsTestILC to preview1-03124-01, preview1-26824-01, preview1-26824-01, preview1-26823-04, beta-26823-00, beta-26823-00, respectively (master) #31890

Merged

weshaggard reviewed Aug 22, 2018

View reviewed changes

safern reviewed Aug 23, 2018

View reviewed changes

ViktorHofer added 4 commits August 23, 2018 19:53

Update xunit to official 2.4.0 runner

f528b66

Add necessary Test SDK assemblies Enable test sdk props & targets only for netcoreapp

Remove uap specific RemoteExecutor code

bfb6d9c

Enable netfx perf runs and promote to netstandard

1b33f6c

Xunit 2.4 test changes

591f0b5

safern reviewed Aug 23, 2018

View reviewed changes

safern approved these changes Aug 23, 2018

View reviewed changes

Add uap test leg to CI

c54d4a2

Disable/Enable tests for clean uap CI leg

weshaggard reviewed Aug 23, 2018

View reviewed changes

ViktorHofer mentioned this pull request Aug 26, 2018

Remove old PerformanceTests #31958

Merged

ahsonkhan reviewed Oct 24, 2018

View reviewed changes

krwq reviewed Oct 24, 2018

View reviewed changes

ViktorHofer mentioned this pull request Jan 31, 2020

OpenCover code coverage broken dotnet msbuild dotnet/runtime#26902

Closed

Anipik mentioned this pull request Jan 31, 2020

FileVersionInfo.GetVersionInfo(*.cs) Internal name should return the name with extension .dll rather than.exe on Unix dotnet/runtime#27196

Closed

		@@ -0,0 +1,105 @@
		Microsoft (R) Build Engine version 15.7.179.6572 for .NET Framework
		Copyright (C) Microsoft Corporation. All rights reserved.

		@@ -185,8 +188,8 @@
		</UpdateStep>
		<XmlUpdateStep Include="BuildTools">

Conversation

ViktorHofer commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

ViktorHofer commented Aug 23, 2018

Uh oh!

ViktorHofer commented Aug 23, 2018

Uh oh!

ViktorHofer commented Aug 23, 2018

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!

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!

ViktorHofer Aug 23, 2018 • 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!

safern left a comment

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!

ViktorHofer commented Aug 23, 2018

Uh oh!

4creators commented Aug 24, 2018

Uh oh!

ViktorHofer commented Aug 24, 2018

ViktorHofer commented Jul 23, 2018 •

edited

Loading

ViktorHofer Aug 23, 2018 •

edited

Loading

ahsonkhan Oct 24, 2018 •

edited

Loading