-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
Seems like we're hitting MAX_PATH in MSBuild. |
c971e60
to
2bab039
Compare
Good job! |
After enabling all 250+ tests that run on CoreFX test PRs, we're failing some of them because of missing dependencies. Commit with dependencies and test exclusions to follow. |
57b6bf7
to
6480390
Compare
All dependencies are now restored correctly. From all executing tests we have ~20 that are failing. The end-to-end test run takes around 3 hours on my local machine. |
This feels way too long. What is the runtime flavor used for this? Do the tests run in parallel? |
@jkotas Xunit itself parallelizes test runs, but the script does not - I ran into the issue of the runner locking assemblies. |
The above comment was a bit off the mark. We can parallelize execution of the runners, but it would have to be managed by the CoreCLR test scripts. |
The last commit adds a test helper to run tests in parallel. A full run takes around 90 minutes (x64 release) on my local machine. All tests pass with a few fact failures and the notable exception of System.Data.SqlClient.* tests, which hit this issue https://github.com/dotnet/corefx/issues/5252 . |
@@ -27,3 +28,38 @@ Use the following instructions to test a change to the dotnet/coreclr repo using | |||
|
|||
[run-corefx-tests.py](https://github.com/dotnet/coreclr/blob/master/tests/scripts/run-corefx-tests.py) will clone dotnet/corefx and run steps 2-4 above automatically. It is primarily intended to be run by the dotnet/coreclr CI system, but it might provide a useful reference or shortcut for individuals running the tests locally. | |||
|
|||
## Using the built CoreCLR testhost | |||
**These instructions are currenly Windows only** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currenly [](start = 25, length = 8)
typo: currenly
@@ -27,3 +28,38 @@ Use the following instructions to test a change to the dotnet/coreclr repo using | |||
|
|||
[run-corefx-tests.py](https://github.com/dotnet/coreclr/blob/master/tests/scripts/run-corefx-tests.py) will clone dotnet/corefx and run steps 2-4 above automatically. It is primarily intended to be run by the dotnet/coreclr CI system, but it might provide a useful reference or shortcut for individuals running the tests locally. | |||
|
|||
## Using the built CoreCLR testhost | |||
**These instructions are currenly Windows only** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currenly [](start = 25, length = 8)
grammar: missing trailing period.
For example to run .NET Core Windows tests from System.Collections.Tests with an x64 Release build of CoreCLR. | ||
``` | ||
pushd C:\corefx\bin\tests\System.Collections.Tests | ||
C:\coreclr\bin\tests\Windows_NT.x64.Release\testhost\dotnet.exe .\xunit.console.netcore.exe .\System.Collections.Tests.dll -notrait category=nonnetcoretests -notraint category=nonwindowstests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-notraint [](start = 157, length = 9)
typo? -notraint
``` | ||
|
||
### CI Script | ||
CoreCLR has an alternative way to run CoreFX tests, built for PR CI jobs. To run tests against pre-built binaries you can exscute the following from the CoreCLR repo root: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exscute [](start = 122, length = 7)
typo: exscute
tests/runtest.cmd
Outdated
if /i "%1" == "CoreFXTests" (exit /b 0) | ||
if /i "%1" == "CoreFXTests" (set __CoreFXTests=true&shift&goto Arg_Loop) | ||
if /i "%1" == "CoreFXTestsAll" (set __CoreFXTests=true&shift&set __CoreFXTestsRunAllAvailable=true&shift&goto Arg_Loop) | ||
if /i "%1" == "CoreFXTestList" (set __CoreFXTests=true&set __CoreFXTestList=%2&shift&shift&goto Arg_Loop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shifts twice, so skips the next argument
tests/runtest.cmd
Outdated
set _CoreFX_TestLogFileName=testResults.xml | ||
set _CoreFX_TestRunScriptName=CoreCLR_RunTest.cmd | ||
echo All coreFX tests set to %__CoreFXTestsRunAllAvailable% | ||
if "%__CoreFXTestsRunAllAvailable%" == "true" ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this output is not necessary, since it is an argument in the dotnet command that is shown below
tests/runtest.cmd
Outdated
|
||
|
||
echo Downloading and Running CoreFX Test Binaries | ||
echo call "%_dotnet%" "%_CoreFXTestUtilitiesOutputPath%\%_CoreFXTestSetupUtilityName%.dll" --clean --outputDirectory "%_CoreFXTestBinariesPath%" --testListJsonPath "%__CoreFXTestList%" --testUrl "!_CoreFXTestRemoteURL!" %_CoreFX_RunCommand% --dotnetPath "%_CoreFXTestHost%\dotnet.exe" --executable %_CoreFXTestExecutable% --logPath %_CoreFXLogsDir% %_CoreFXTestExecutableArgs% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All "echo" to the screen should be prefixed by %__MsgPrefix%
tests/runtest.cmd
Outdated
if not exist %_CoreFXTestHost%\dotnet.exe echo CoreFX test host not found, please run runtest.cmd again && exit /b 1 | ||
|
||
set /p _CoreFXTestRemoteURL=< "%__ProjectFilesDir%\CoreFX\CoreFXTestListURL.txt" | ||
if not defined __CoreFXTestList ( set __CoreFXTestList=%__ProjectFilesDir%\CoreFX\TopN.CoreFX.Windows.issues.json ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TopN.CoreFX.Windows.issues.json [](start = 82, length = 31)
regarding "TopN.CoreFX.Windows.issues.json":
- should there instead be a single file, with per-OS, per-arch, per-configuration (e.g., stress mode) exclusions (kind of like smarty's .lst file Categories), OR,
- should this filename also have arch (e.g., x86, x64), and maybe config (e.g., stress mode) in it?
I would hope we could do #1, because if a test fails in all architectures/OS/stress combinations, I wouldn't want to add it to 15+ different files. Conversely, I hope we can disable on a relatively fine granularity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 is completely reasonable (and ideal) - the helper projects are fairly simple and the schema is easily extensible. For the time being I've renamed the files to include architecture.
If this proves to be a viable approach and the python script is fully deprecated, architectures/OS/stress combinations should definitely be included as well.
@@ -0,0 +1,47 @@ | |||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these files have either (1) a full description at the top of the file format, as a comment, or (2) a link to documentation for this format?
using System; | ||
using System.Collections.Generic; | ||
using System.CommandLine; | ||
using System.IO; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this (and other source files) need license headers.
And also comments (a file header comment describing the purpose of the code, minimally)
arguments.Append(Path.Combine(testDirectory, Path.GetFileName(testDirectory))); | ||
arguments.Append(".rsp"); | ||
arguments.Append("\""); | ||
arguments.Append(" "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a place where C# interpolated strings would make it easier, e.g.:
arguments.Append($"@\"{Path.Combine(testDirectory, Path.GetFileName(testDirectory))}.rsp\" ");
@BruceForstall Thanks for taking a look! |
I agree that Checked x64 + Checked x86 is a better mix to trigger by default. It provides more coverage than Checked x64 + Release x64 for the same cost. |
That might add a bit of complexity - we would have to rely on our own xunit console runner, so as to not maintain two versions of CoreFX assemblies in blob storage. |
Yes, that's fine. |
* Add test list CL switch * End-To-End Test Run on Windows * Cleanup * MAX_PATH Workaround * Set Execution directory for CoreFX tests * Add All CoreFX PR Tests * Add test dependencies * Add extra dependencies * Add parallel test execution * Disable OuterLoop tests and System.Data.SqlClient.* tests * Initialize maximum degree of parallelization to Environment.ProcessCount * Remove unnecessary cli option * Update Dependencies * Add "enabled" property to tests * Remove exclusions due to TestUtilities mismatch * Add capability to run all tests for running Helix test lists directly * Refactor build script to build testhost when skipping managed tests * Disable failing System.Threading.Tests.EventWaitHandleTests.Ctor_InvalidMode * Add switch to skip native test build * Add testing documentation * Don't run tests marked as "disabled" when running all available tests * Add switch to build only testhost and remove Core_Root_Stage * Clean up TopN.CoreFX.Windows.issues.json * Refactor build-test.cmd * PR feedback - build pipeline and documentation * PR Feedback - Test Helper headers and comments * Fix buildtesthost option for only building CoreFX test dependencies * Disable intermittently failing test DrawBezier_PointFs Commit migrated from dotnet/coreclr@d3772d9
This is connected to #18156
The following commit builds a test host and uses as helper projectt to download and run CoreFX binaries.
There are no Windows-specifics to the implementation other than the batch scripts.
The test lists are included as an example - I've added a command-line parameter to specify a test list according to scenario.
The test urls currently point to CoreRT's CI - again just to be able to test binary download.
cc @RussKeldorph @BruceForstall @jkotas @sergiy-k