Skip to content

Test build target agnostic separately from target specific tests - #34658

Merged
sdmaclea merged 10 commits into
dotnet:masterfrom
sdmaclea:TestDecouple
Apr 9, 2020
Merged

Test build target agnostic separately from target specific tests#34658
sdmaclea merged 10 commits into
dotnet:masterfrom
sdmaclea:TestDecouple

Conversation

@sdmaclea

Copy link
Copy Markdown
Contributor

Part of #33066

Modifies build-test to add options to allow building target agnostic tests separately from target specific tests.

  • Audited the TraitTags found a few cases where TraitTags were missed in Added OS and Bit Specific tags to accordingly .csproj files. #637. Also added ArchSpecific
  • @VSadov is working on a generic filtering mechanism. While waiting for it this PR add an extra property CLRTestNeedTarget to indicate a test which is not target agnostic.
  • I had originally tried to build the agnostic tests with AnyOS and AnyCPU, but I ran into lots of issues. So I reverted that.

@sdmacleasdmaclea added this to the 5.0 milestone Apr 7, 2020
@sdmacleasdmaclea self-assigned this Apr 7, 2020
@ghost

ghost commented Apr 7, 2020

Copy link
Copy Markdown

Tagging @ViktorHofer as an area owner

@sdmaclea

Copy link
Copy Markdown
ContributorAuthor

I tested manually building on Linux. It looks good to me.
I will try building tests on Windows too now.

@sdmaclea

sdmaclea commented Apr 7, 2020

Copy link
Copy Markdown
ContributorAuthor

The simple filtering used in this PR is relatively slow. It takes a while for MSBuild to find the tests to build (when finding the small set of target specific tests).

Generic filtering may be slower.

It is possible we may need to build test lists to make this faster....

@sdmacleasdmaclea added area-Infrastructure-coreclr Only use for closed issues and removed area-Infrastructure labels Apr 7, 2020

@jkoritzinskyjkoritzinsky left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do a string for CLRTestNeedTargetToBuilds value instead of an opaque number that has to be documented somewhere else?

@sdmaclea

sdmaclea commented Apr 7, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we do a string for CLRTestNeedTargetToBuilds

You're going to make me do the hard work of naming the states.

Effectively right now the states are

  • CLRTestNeedTargetToBuild == 0 : False
  • CLRTestNeedTargetToBuild == 1: True
  • CLRTestNeedTargetToBuild == ''BuildAll

@jkoritzinsky How does BuildTargetGenericTests and BuildTargetSpecificTests sound for strings. Keeping an empty string for the default behavior.

@jkoritzinsky

Copy link
Copy Markdown
Member

Those work. We could also shorten them to "TargetGeneric" and "TargetSpecific" if you would like.

@sdmaclea

sdmaclea commented Apr 7, 2020

Copy link
Copy Markdown
ContributorAuthor

What about the script option? Sounds like they should change to

  • -buildTargetSpecific
  • -buildTargetGeneric

or

  • -targetSpecific
  • -targetGeneric

@ivdiazsa

Copy link
Copy Markdown
Contributor

I agree with @jkoritzinsky's suggestions. Other than that, LGTM! I'm assuming the test lists building would be done in a separate PR?

@ivdiazsa

Copy link
Copy Markdown
Contributor

What about the script option? Sounds like they should change to

-buildTargetSpecific
-buildTargetGeneric

or

-targetSpecific
-targetGeneric

Both are fine by me. I would vote for the latter choices. Since the script is already called build-test, the former ones are a bit redundant.

@sdmaclea

Copy link
Copy Markdown
ContributorAuthor

I'm assuming the test lists building would be done in a separate PR?

@VSadov Is working on generic filtering. I was assuming we would wait and see what he comes up with before engineering another solution,

@sdmaclea

Copy link
Copy Markdown
ContributorAuthor

I made the renames per feedback.

I also did a quick benchmark of test building on my local Hyper-V Ubuntu machine. It took 10 minutes to build all the tests. When using testSpecific, the build time dropped to 3 minutes.

@sdmaclea

Copy link
Copy Markdown
ContributorAuthor

quick benchmark - Windows results were about the same as Linux Hyper-V on the same machine.

@sdmaclea
sdmaclea merged commit 0b02d28 into dotnet:masterApr 9, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@sdmaclea@jkoritzinsky@ivdiazsa@Dotnet-GitSync-Bot