Add support for building just a subset of runtime tests #57142

trylek · 2021-08-10T16:27:26Z

This change adds four new options to the runtime test build command:

test <test project path relative to src\tests> - build just that
one test.
dir <directory relative to src\tests> - build all tests in the
directory.
tree <directory relative to src\tests> - build all tests in the
given subtree.
all - use clean rebuild (i.e. don't apply incrementalism)
when building the tests.

Thanks

Tomas

/cc @dotnet/runtime-infrastructure

src/tests/build.sh

ghost · 2021-08-10T16:38:59Z

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

Issue Details

This change adds four new options to the runtime test build command:

test <test project path relative to src\tests> - build just that
one test.
dir <directory relative to src\tests> - build all tests in the
directory.
tree <directory relative to src\tests> - build all tests in the
given subtree.
all - use clean rebuild (i.e. don't apply incrementalism)
when building the tests.

Thanks

Tomas

/cc @dotnet/runtime-infrastructure

Author:	trylek
Assignees:	-
Labels:	`area-Infrastructure-coreclr`
Milestone:	-

jkoritzinsky · 2021-08-10T16:54:02Z

What's the expected behavior if multiple of these new commands are supplied? Does one override the others, do multiple take effect, or does the script error out?

naricc · 2021-08-10T17:42:11Z

What's the expected behavior if multiple of these new commands are supplied? Does one override the others, do multiple take effect, or does the script error out?

It looks like multiple seperate commands take a effect (-dir/-tree), but using for example -test twice for two different tests doesn't. I'm not sure this is wrong exactly, but it is a little odd?

trylek · 2021-08-10T18:00:13Z

Right now only the last command of each type is honored; I can trivially change them to item groups by using the semicolon separator and honoring all instances of each; I'll update the PR after our team meeting.

This change adds four new options to the runtime test build command: 1) test <test project path relative to src\tests> - build just that one test. 2) dir <directory relative to src\tests> - build all tests in the directory. 3) tree <directory relative to src\tests> - build all tests in the given subtree. 4) all - use clean rebuild (i.e. don't apply incrementalism) when building the tests. Thanks Tomas

src/tests/build.sh

AntonLapounov · 2021-08-10T21:50:06Z

all - use clean rebuild (i.e. don't apply incrementalism)

Would it not be better to name this option -clean then?

trylek · 2021-08-10T21:56:39Z

Would it not be better to name this option -clean then?

@AntonLapounov - I don't know; I'm not too happy with all either but I've thought about this for quite a while and I haven't found any other ideal name:

src\tests\build clean looks like a request to just clean up the test builds, not to rebuild everything.
src\tests\build rebuild is what I originally had but to my dismay I found out that src\tests\build.sh already implements this option by deleting all test artifacts prior to the build.

I'll be happy for suggestions.

naricc · 2021-08-10T22:38:25Z

Would it not be better to name this option -clean then?

@AntonLapounov - I don't know; I'm not too happy with all either but I've thought about this for quite a while and I haven't found any other ideal name:

src\tests\build clean looks like a request to just clean up the test builds, not to rebuild everything.

src\tests\build rebuild is what I originally had but to my dismay I found out that src\tests\build.sh already implements this option by deleting all test artifacts prior to the build.

I'll be happy for suggestions.

Maybe -cleanbuild ?

AntonLapounov · 2021-08-10T23:45:24Z

Do we actually need the -all option then? One can use either -rebuild (I think it should also be added to build.cmd for consistency and maybe renamed to -clean) or just directly specify /t:rebuild argument on the command line, which should be propagated to MSBuild.

trylek · 2021-08-10T23:58:07Z

I think you're right, that sounds reasonable to me.

… feedback It also turns out that the rebuild cleanup was in the wrong place in the build.sh script - it was done as part of generate_layout i.e. after the native and managed test components have been built i.o.w. using it ended up building the tests and deleting them afterwards. Thanks Tomas

AntonLapounov

Looks good, thank you!

fanyang-mono

I am so happy to see this change. No more hacking for this. Please update the corresponding doc for this new capability.

trylek · 2021-08-11T23:09:17Z

@fanyang-mono - I have added description of the new options to the workflow docs per your suggestion. Can you please take another look when you have a chance and let me know if you find that sufficient or possibly suggest additional doc updates?

fanyang-mono · 2021-08-12T00:43:16Z

docs/workflow/testing/coreclr/unix-test-instructions.md

+lengthy (especially in `-priority1` mode) and unnecessary when working on a particular test.
+
+1) `-test:<test-project>` - build a particular test project specified by its project file path,
+either absolute or relative to `src/tests`. The option can be specified multiple times on the command


Please mention that the option that they could provide multiple items and separate them by semicolon.

Updated in 9th commit.

fanyang-mono · 2021-08-12T00:43:57Z

docs/workflow/testing/coreclr/unix-test-instructions.md

+**Example**: `src/tests/build.sh -test:JIT/Methodical/divrem/div/i4div_cs_do.csproj`
+
+2) `-dir:<test-folder>` - build all test projects within a given directory path, either absolute
+or relative to `src/tests`. The option can be specified multiple times on the command line to request


Updated in 9th commit.

fanyang-mono · 2021-08-12T00:44:09Z

docs/workflow/testing/coreclr/unix-test-instructions.md

+**Example**: `src/tests/build.sh -dir:JIT/Methodical/Arrays/huge`
+
+3) `-tree:<root-folder>` - build all test projects within the subtree specified by its root path,
+either absolute or relative to `src/tests`. The option can be specified multiple times on the command


Updated in 9th commit.

am11 · 2021-08-15T13:06:52Z

Seems like it is not compatible with all projects under src/tests.
e.g. ILVerification.Tests.csproj

$ git clean -xdf
$ ./build.sh -c release
$ src/tests/build.sh -release -test:ilverify/ILVerification.Tests.csproj 
...
...
Managed tests build success!
Creating test wrappers...
Microsoft (R) Build Engine version 17.0.0-preview-21411-04+6ca861613 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

/Users/am11/projects/runtime/.dotnet/sdk/6.0.100-rc.1.21411.28/MSBuild.dll -distributedlogger:Microsoft.DotNet.Tools.MSBuild.MSBuildLogger,/Users/am11/projects/runtime/.dotnet/sdk/6.0.100-rc.1.21411.28/dotnet.dll*Microsoft.DotNet.Tools.MSBuild.MSBuildForwardingLogger,/Users/am11/projects/runtime/.dotnet/sdk/6.0.100-rc.1.21411.28/dotnet.dll -maxcpucount -verbosity:m /bl:/Users/am11/projects/runtime/artifacts/log/Release/build_test_wrappers_coreclr.binlog /consoleloggerparameters:Summary /fileloggerparameters1:WarningsOnly;LogFile=/Users/am11/projects/runtime/artifacts/log/Check_Test_Build.OSX.x64.Release.wrn /fileloggerparameters2:ErrorsOnly;LogFile=/Users/am11/projects/runtime/artifacts/log/Check_Test_Build.OSX.x64.Release.err /fileloggerparameters:Verbosity=normal;LogFile=/Users/am11/projects/runtime/artifacts/log/Check_Test_Build.OSX.x64.Release.log /nodereuse:false /p:BuildWrappers=true /p:TestBuildMode= /p:TargetsWindows= /p:TargetOS=OSX /p:Configuration=Release /p:TargetArchitecture=x64 /p:RuntimeFlavor=coreclr /Users/am11/projects/runtime/src/tests/run.proj
CSC : warning CS2008: No source files specified. [/Users/am11/projects/runtime/src/tests/Common/test_dependencies/test_dependencies.csproj]
  test_dependencies -> /Users/am11/projects/runtime/artifacts/tests/coreclr/OSX.x64.Release/Common/test_dependencies/test_dependencies/test_dependencies.dll
/Users/am11/projects/runtime/src/tests/run.proj(342,9): error MSB4184: The expression "[MSBuild]::MakeRelative('', /**/*.sh)" cannot be evaluated. The path is empty. (Parameter 'path')

Build FAILED.

CSC : warning CS2008: No source files specified. [/Users/am11/projects/runtime/src/tests/Common/test_dependencies/test_dependencies.csproj]
/Users/am11/projects/runtime/src/tests/run.proj(342,9): error MSB4184: The expression "[MSBuild]::MakeRelative('', /**/*.sh)" cannot be evaluated. The path is empty. (Parameter 'path')
    1 Warning(s)
    1 Error(s)

Without -test:... argument ilverification test project builds fine and we can run it with:

bash artifacts/tests/coreclr/OSX.x64.Release/ilverify/ILVerification.Tests.sh \
    -coreroot=$(pwd)/artifacts/tests/coreclr/OSX.x64.Release/Tests/Core_Root

am11 · 2021-08-15T13:22:37Z

docs/workflow/testing/coreclr/unix-test-instructions.md

+line to request building several individual projects; alternatively, a single occurrence of the option
+can represent several project paths separated by semicolons.
+
+**Example**: `src/tests/build.sh -test:JIT/Methodical/divrem/div/i4div_cs_do.csproj;JIT/Methodical/divrem/div/i8div_cs_do.csproj`


In all Unix-y shells including powershell, semicolon is a statement terminator. This example only builds i4div_cs_do.csproj. I think having just one approach of multiple -test: occurrences was enough really. Now we support both approaches to achieve the same thing, with different workwounds each.

trylek · 2021-08-16T19:48:24Z

@am11 - for the ILVerification.Tests.csproj, sorry about that; as I described in the accompanying documentation, the new options are currently orthogonal to the priority value; as you can easily see the test ILVerification.Tests is Pri1,

runtime/src/tests/ilverify/ILVerification.Tests.csproj

Line 5 in 26b20d0

and so it won't build unless you specify -priority1 in the src/tests/build.sh command line. I understand it's somewhat countra-intuitive but the other option - forcibly setting priority to 1 whenever a -test / -dir / -tree option is specified it not ideal either because it creates even more complex internal logic where seemingly identical things work differently - e.g. "-tree:." would end up building something different than when -tree is not specified and / or -tree / -dir containing a single test project would behave differently than specifying -test with the same project name.

My hope is that, based on my proposal

#54512

we should be able to reduce the overall number of CoreCLR tests and ultimately get rid of the Pri0 / Pri1 distinction; I plan to start preparatory cleanup work in this direction during the oncoming quality week.

For the other comment regarding inappropriate use of semicolon in Unix, perhaps we can just change the semicolon to colon on Unix akin to how search paths are normally stored.

trylek requested review from AntonLapounov, naricc and fanyang-mono August 10, 2021 16:27

am11 reviewed Aug 10, 2021

View reviewed changes

src/tests/build.sh Outdated Show resolved Hide resolved

dotnet-issue-labeler bot added the area-Infrastructure-coreclr label Aug 10, 2021

trylek force-pushed the BuildTestSubset branch from 61e0c30 to 2369d08 Compare August 10, 2021 20:17

Allow multiple specifications of 'test', 'dir' and 'tree'

28830f9

trylek force-pushed the BuildTestSubset branch from 2369d08 to 28830f9 Compare August 10, 2021 20:18

Fix Unix version of the script

59768be

AntonLapounov reviewed Aug 10, 2021

View reviewed changes

src/tests/build.sh Outdated Show resolved Hide resolved

Fix copy & paste bug in help string per Anton's PR feedback

7f22d86

trylek added 3 commits August 11, 2021 02:14

Replace the 'all' option with 'rebuild' in build.cmd help text

df04d2c

Fix overlooked $ sign in build.sh

11c0cca

AntonLapounov approved these changes Aug 11, 2021

View reviewed changes

fanyang-mono requested changes Aug 11, 2021

View reviewed changes

naricc approved these changes Aug 11, 2021

View reviewed changes

Describe the new options in test workflow docs per Fan's PR feedback

9b73df3

fanyang-mono reviewed Aug 12, 2021

View reviewed changes

Further expand documentation per Fan's PR feedback

99e0cf9

fanyang-mono approved these changes Aug 12, 2021

View reviewed changes

trylek merged commit 3b40c7c into dotnet:main Aug 12, 2021

trylek deleted the BuildTestSubset branch August 12, 2021 13:54

am11 reviewed Aug 15, 2021

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Sep 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for building just a subset of runtime tests #57142

Add support for building just a subset of runtime tests #57142

trylek commented Aug 10, 2021

ghost commented Aug 10, 2021

jkoritzinsky commented Aug 10, 2021

naricc commented Aug 10, 2021

trylek commented Aug 10, 2021

AntonLapounov commented Aug 10, 2021

trylek commented Aug 10, 2021

naricc commented Aug 10, 2021

AntonLapounov commented Aug 10, 2021 •

edited

Loading

trylek commented Aug 10, 2021

AntonLapounov left a comment

fanyang-mono left a comment

trylek commented Aug 11, 2021

fanyang-mono Aug 12, 2021

trylek Aug 12, 2021

fanyang-mono Aug 12, 2021

trylek Aug 12, 2021

fanyang-mono Aug 12, 2021

trylek Aug 12, 2021

am11 commented Aug 15, 2021

am11 Aug 15, 2021

trylek commented Aug 16, 2021

Add support for building just a subset of runtime tests #57142

Add support for building just a subset of runtime tests #57142

Conversation

trylek commented Aug 10, 2021

ghost commented Aug 10, 2021

jkoritzinsky commented Aug 10, 2021

naricc commented Aug 10, 2021

trylek commented Aug 10, 2021

AntonLapounov commented Aug 10, 2021

trylek commented Aug 10, 2021

naricc commented Aug 10, 2021

AntonLapounov commented Aug 10, 2021 • edited Loading

trylek commented Aug 10, 2021

AntonLapounov left a comment

Choose a reason for hiding this comment

fanyang-mono left a comment

Choose a reason for hiding this comment

trylek commented Aug 11, 2021

fanyang-mono Aug 12, 2021

Choose a reason for hiding this comment

trylek Aug 12, 2021

Choose a reason for hiding this comment

fanyang-mono Aug 12, 2021

Choose a reason for hiding this comment

trylek Aug 12, 2021

Choose a reason for hiding this comment

fanyang-mono Aug 12, 2021

Choose a reason for hiding this comment

trylek Aug 12, 2021

Choose a reason for hiding this comment

am11 commented Aug 15, 2021

am11 Aug 15, 2021

Choose a reason for hiding this comment

trylek commented Aug 16, 2021

AntonLapounov commented Aug 10, 2021 •

edited

Loading