This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Build xunit wrappers the same way on windows and unix #18695
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
build-test.sh
Outdated
| fi | ||
|
|
||
| echo "Starting the Managed Tests Build..." | ||
|
|
||
| build_Tests_internal "Tests_Managed" "$__ProjectDir/tests/build.proj" "$__up" "Managed tests build (build tests)" | ||
| # build_Tests_internal "Tests_Managed" "$__ProjectDir/tests/build.proj" "Managed tests build (build tests)" "$__up" |
There was a problem hiding this comment.
If this call isn't needed, can it be removed instead of commented out? Alternatively, you could comment why it is commented out.
There was a problem hiding this comment.
This should be uncommented. It was used to quickly iterate on building the test wrappers.
There was a problem hiding this comment.
Yeah, left over from testing. Will fix shortly.
| @@ -0,0 +1,35 @@ | |||
| <Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
There was a problem hiding this comment.
@chsienki Does this align with your work as well?
| @@ -0,0 +1,35 @@ | |||
| <Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
There was a problem hiding this comment.
If this is being included in SDK project types, should we have a property for the TargetFramework. Then we can define it once for all test projects?
There was a problem hiding this comment.
Put this in a common props file - see my comment below.
|
Benchmark tests under tests\src\JIT\performance run two ways -- as normal tests and also as performance tests. As normal tests they just run like any other test in the tree. As performance tests they get run today via tests\script\run-xunit-perf.py. For the stuff under tests\src\performance let's loop in @jorive and @noahfalk. |
tests/dir.sdkbuild.props
Outdated
| <PropertyGroup> | ||
| <OSPlatformConfig>$(BuildOS).$(Platform).$(Configuration)</OSPlatformConfig> | ||
|
|
||
| <TestSrcDir>$(CoreclrDir)/tests/src</TestSrcDir> |
There was a problem hiding this comment.
I like how many of these paths are being unified for testing, but it seems odd that TestSrcDir and TestWorkingDir aren't defined together.
There was a problem hiding this comment.
It looks like TestWorkingDir is set in coreclr/dir.props, and used by the helix projects and runtest.cmd. For this change, I only factored out the properties used by SDK-style projects. None of the stuff in coreclr/dir.props is used by the new projects. Perhaps @chsienki's work will move more (everything?) over to a single props file.
tests/dir.sdkbuild.props
Outdated
|
|
||
| <!-- BaseIntermediateOutputPath is used by the SDK as the location | ||
| for the lock file, and props/targets from nuget packages. --> | ||
| <BaseIntermediateOutputPath>$(CoreclrDir)/bin/tests/obj/$(OSPlatformConfig)/Managed/$(BuildProjectRelativeDir)</BaseIntermediateOutputPath> |
There was a problem hiding this comment.
Can we use RootBinDir here?
There was a problem hiding this comment.
I might be confused on how these targets files are being included so if they are in different import chains ignore me :-)
There was a problem hiding this comment.
They're on different import chains. :) coreclr/dir.props has a different BaseIntermediateOutputPath and a bunch of other properties that the SDK test projects don't need. BaseIntermediateOutputPath for SDK projects needs to be different because it's used in a very specific way by the SDK. See this: dotnet/msbuild#1603 (comment).
tests/runtest.proj
Outdated
| <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.props" /> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>netcoreapp2.0</TargetFramework> |
There was a problem hiding this comment.
This is the property I mentioned above. I think this should be a property defined in one of the previous imports.
There was a problem hiding this comment.
Took this suggestion, thanks! I repurposed dir.sdkbuild.props as a place for SDK-only props, as I think that's clearer.
| <Error Condition="!Exists('$(PackagesDir)$(XUnitRunnerPackageId)\$(XUnitPackageVersion)\tools\xunit.console.exe')" | ||
| Text="Error: looks the package $(PackagesDir)$(XUnitRunnerPackageId)\$(XUnitPackageVersion) not restored or missing xunit.console.exe." | ||
| <Target Name="AddXunitConsoleRunner" BeforeTargets="ResolveReferences"> | ||
| <Error Condition="!Exists('$(PackagesDir)\$(XUnitRunnerPackageId)\$(XUnitPackageVersion)\tools\netcoreapp1.0\xunit.console.dll')" |
There was a problem hiding this comment.
Should netcoreapp1.0 be defined in a props file or this is the version we use now and forever?
And below.
There was a problem hiding this comment.
I updated this to use a property in the same file for now, since this is specific to the xunit console runner package layout. The package ships with assets for netcoreapp1.0 and netcoreapp2.0. I'm not sure where else the runner might be used and I see a bunch of projects in our tree that haven't been updated to netcoreapp2.0, so I'm using this as a lowest common denominator.
tests/src/dir.common.props
Outdated
| <Configuration Condition="'$(Configuration)' ==''">$(BuildType)</Configuration> | ||
| <Platform Condition="'$(Platform)'==''">$(BuildArch)</Platform> | ||
| </PropertyGroup> | ||
| <Import Project="..\dir.sdkbuild.props" /> |
There was a problem hiding this comment.
Should this be included in the dirs.props or is there a specific reason a new one was created?
There was a problem hiding this comment.
The SDK-style projects need to only import dir.sdkbuild.props, not dir.props. My hope is that with @chsienki's test changes, we can further unify these.
There was a problem hiding this comment.
It's currently a bit confusing since the props files are in an intermediate state - some projects build using bulidtools and some against the SDK. dir.sdkbuild.props specifically has the common properties that were shared between the SDK projects and also the old projects. So it's currently not a good place to put props that apply only to SDK projects.
There was a problem hiding this comment.
I'll try to clarify this by adding some comments to the props files.
There was a problem hiding this comment.
I did some more clean-up of props files - see this commit message: 1d67302.
jashook
reviewed
Jun 29, 2018
There was a problem hiding this comment.
@sbomer great change thank you for the work. It looks good to me; however, I have a few questions.
- You mention dropping build tools dependency to build the tests, are we now expecting a system install of dotnet to build? If so we should add documentation and we may want to expand the conversation on whether that is what we want going forward.
- It would be extremely useful now that there has been a deep dive through our project system to get some documentation on how it works and all of the interconnected parts.
Before this change goes in the following should be done to validate it is correct.
- Innerloop CI run green
- Run outerloop pri1 jobs for each OS green
- Private official build run, see me on how to set this up, note the above should be done first
- Manually verify that all the tests before the change are still run after the change on all OS.
build-test.sh
Outdated
| fi | ||
|
|
||
| echo "Starting the Managed Tests Build..." | ||
|
|
||
| build_Tests_internal "Tests_Managed" "$__ProjectDir/tests/build.proj" "$__up" "Managed tests build (build tests)" | ||
| # build_Tests_internal "Tests_Managed" "$__ProjectDir/tests/build.proj" "Managed tests build (build tests)" "$__up" |
There was a problem hiding this comment.
This should be uncommented. It was used to quickly iterate on building the test wrappers.
|
|
||
| echo "Building step '$stepName' slice=$slice via $buildCommand" | ||
|
|
||
| # Invoke MSBuild | ||
| eval $buildCommand | ||
| "$__ProjectRoot/run.sh" build "${buildArgs[@]}" |
sbomer
force-pushed
the
unify_test_runner
branch
from
1b5ef93
to
7296dfb
Compare
|
@jashook and @AaronRobinsonMSFT, thanks for the review!
Thanks for the validation tips - I'll definitely talk to you once I get CI jobs green. |
sbomer
force-pushed
the
unify_test_runner
branch
from
c70730c
to
9d5542d
Compare
Merged
|
@dotnet-bot test Windows_NT x64 Checked Innerloop Build and Test |
|
@dotnet-bot help |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
@dotnet-bot test Windows_NT x64 Checked Innerloop Build and Test |
|
@dotnet-bot OSX10.12 x64 Checked Innerloop Build and Test |
|
@dotnet-bot help |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
@dotnet-bot test Windows_NT arm64 Cross Checked normal Build and Test |
sbomer
force-pushed
the
unify_test_runner
branch
from
96a11ef
to
16f7c56
Compare
|
@dotnet-bot Windows_NT x64 Checked CoreFX Tests |
tests/src/dir.common.props was only used for the desktop-specific xunit wrapper helper library. There's no need for it any more, so its properties have been moved into tests/src/dir.props. dir.sdkbuild.props has been renamed to dir.common.props, since it contains properties used by SDK projects and buildtools projects. This change also re-enables the test build.
With this, some properties shared by SDK projects can go in a global location. The TargetFramework is shared by all SDK projects in the test tree. This change also uses a property for the xunit package directory that contains the xunit.console.dll we copy to core_root.
This fixes a failure in the windows build.
On windows, the use of run.exe, config.json, and msbuild.cmd uses msbuild.exe on the path. This change will build wrappers using the local SDK via "dotnet msbuild", bypassing run.exe. Run.exe will go away entirely with the move from buildtools to arcade, so other build invocatios should follow suit.
UseBuildTools used to be true all the time. Now that we are building wrappers on core, UseBuildTools becomes false. However, the rest of the runtest.proj expects to build using buildtools, so we keep UseBuildTools true until we switch to arcade. The CSharpCoreTargetsPath was imported when running on core only. This used to happen only on unix, but now it also happens when building runtest.proj for the xunit wrappers on windows. On unix, this targets file was a symlink to itself to work around some buildtools logic that expected the file to exist. This workaround no longer appears necessary, and on windows, this was never used in the first place, so this change removes it.
UseRoslynCompilers was introduced in buildtools by dotnet/buildtools#947, with different behaviors on windows/unix. It was removed by dotnet/buildtools#1974, so we can unify our roslyn imports now.
The corefx tests run on specific versions of xunit dlls, defined in CoreFX.depproj. We want to use these versions in the test host, not those in CORE_ROOT, so exclude these from being copied to the test host directory. This fixes the failing corefx tests.
These arguments get passed along to the xunit wrapper build as unprocessed build args. They need to work for "dotnet msbuild" (used for the wrapper build) as well as for run.exe.
UnprocessedBuildArgs should contain arguments in the format expected by msbuild, not by run.exe.
The "--" syntax is used by run.exe to pass everything following to msbuild directly. It should not be a part of unprocessed args.
Helix builds tests on windows and runs them on unix using the xunit wrappers. When cross-building the wrappers like this, TargetsWindows is set to false by the test build pipeline. This variable ensures that the wrapper uses correct directory separators when invoking the test .sh file.
Helix builds xunit wrappers on windows, and runs them on unix. The BuildTestsAgainstPackages should currently be set to true in the windows wrapper build to properly filter the .cmd files based on exclusions in issues.targets.
sbomer
force-pushed
the
unify_test_runner
branch
from
8e0190d
to
f167a4a
Compare
Merged
adamsitnik
reviewed
Aug 23, 2018
| @@ -47,13 +47,13 @@ public SearchLoops() | |||
| } | |||
|
|
|||
| [Benchmark(InnerIterationCount = 20000000)] | |||
| public void LoopReturn() | |||
| public void LoopReturnIter() | |||
There was a problem hiding this comment.
@sbomer Namespace, type name, method name and argument names are part of benchmark ID. When you change it, you are introducing a new benchmark.
This change has introduced new benchmark to BenchView.
Please don't rename benchmarks in the future.
/cc @jorive @AndyAyersMS
There was a problem hiding this comment.
Noted, thanks. Just FYI, these were renamed to satisfy the xunit analyzer in the new xunit runner - it didn't like benchmarks methods that had an overload with the same name. The analyzer should enforce this for new benchmarks going forward.
echesakov
pushed a commit
that referenced
this pull request
Oct 23, 2018
echesakov
added a commit
that referenced
this pull request
Oct 24, 2018
…lix queue Port (partially) the following changes: * Build xunit wrappers the same way on windows and unix (#18695) * Update vc-runtime package to support ARM and ARM64 with current builds (#19265) * build-test - fix TestWrapper CS warnings (#19180) * Clean up build.cmd/build-test.cmd/runtest.cmd (#19291) * Use runtest.py to run tests for all platforms (#19213) * Updating runtest.py so that it works with Python 3 (#19768) * Fix build-test.sh wrapper build (#19779) * Move ARM64 Windows boxen to be Helix-provisioned (#20204) * Runtest.py on Windows Arm(64) (#20227) * Use runtest.cmd for arm(64) windows testing (#20301) * Do not restore or initialize buildtools for x86/arm64 (#20370) * Add hack for arm64/x86 to skip build tools restore (#20390) Update Xunit to 2.4.1 prerelease version Use the latest version (dbf0bf1) of tests/runtest.py
echesakov
added a commit
that referenced
this pull request
Oct 24, 2018
…lix queue Port (partially) the following changes: * Build xunit wrappers the same way on windows and unix (#18695) * Work around cmd command length limit in xunit Exec task (#19095) * Update vc-runtime package to support ARM and ARM64 with current builds (#19265) * build-test - fix TestWrapper CS warnings (#19180) * Clean up build.cmd/build-test.cmd/runtest.cmd (#19291) * Use runtest.py to run tests for all platforms (#19213) * Updating runtest.py so that it works with Python 3 (#19768) * Fix build-test.sh wrapper build (#19779) * Move ARM64 Windows boxen to be Helix-provisioned (#20204) * Runtest.py on Windows Arm(64) (#20227) * Use runtest.cmd for arm(64) windows testing (#20301) * Do not restore or initialize buildtools for x86/arm64 (#20370) * Add hack for arm64/x86 to skip build tools restore (#20390) Update Xunit to 2.4.1 prerelease version Use the latest version (dbf0bf1) of tests/runtest.py
echesakov
added a commit
that referenced
this pull request
Oct 25, 2018
…lix queue Port (partially) the following changes: * Build xunit wrappers the same way on windows and unix (#18695) * Work around cmd command length limit in xunit Exec task (#19095) * Update vc-runtime package to support ARM and ARM64 with current builds (#19265) * build-test - fix TestWrapper CS warnings (#19180) * Clean up build.cmd/build-test.cmd/runtest.cmd (#19291) * Use runtest.py to run tests for all platforms (#19213) * Updating runtest.py so that it works with Python 3 (#19768) * Fix build-test.sh wrapper build (#19779) * Move ARM64 Windows boxen to be Helix-provisioned (#20204) * Runtest.py on Windows Arm(64) (#20227) * Use runtest.cmd for arm(64) windows testing (#20301) * Do not restore or initialize buildtools for x86/arm64 (#20370) * Add hack for arm64/x86 to skip build tools restore (#20390) Update Xunit to 2.4.1 prerelease version Use the latest version (dbf0bf1) of tests/runtest.py
echesakov
added a commit
that referenced
this pull request
Oct 26, 2018
…lix queue Port (partially) the following changes: * Build xunit wrappers the same way on windows and unix (#18695) * Work around cmd command length limit in xunit Exec task (#19095) * Update vc-runtime package to support ARM and ARM64 with current builds (#19265) * build-test - fix TestWrapper CS warnings (#19180) * Clean up build.cmd/build-test.cmd/runtest.cmd (#19291) * Use runtest.py to run tests for all platforms (#19213) * Updating runtest.py so that it works with Python 3 (#19768) * Fix build-test.sh wrapper build (#19779) * Move ARM64 Windows boxen to be Helix-provisioned (#20204) * Runtest.py on Windows Arm(64) (#20227) * Use runtest.cmd for arm(64) windows testing (#20301) * Do not restore or initialize buildtools for x86/arm64 (#20370) * Add hack for arm64/x86 to skip build tools restore (#20390) Update Xunit to 2.4.1 prerelease version Use the latest version (dbf0bf1) of tests/runtest.py
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change removes the old net45 generated xunit wrapper projects, and replaces them with SDK projects targeting netcoreapp2.0, built using the SDK instead of buildtools. The new wrappers are built the same way during the windows and unix test builds. Some details:
/cc @RussKeldorph