Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Refine test discovery and partition during test build with new target #17331

Closed
wants to merge 2 commits into from

Conversation

4creators
Copy link

@4creators 4creators commented Mar 29, 2018

Addresses code review feedback from #17161 fixes partially #17282

FYI @BruceForstall This is general fix for getting all tests included in the build plus some improvements.

@4creators
Copy link
Author

test Windows_NT x64 Checked jitincompletehwintrinsic
test Windows_NT x64 Checked jitx86hwintrinsicnoavx2
test Windows_NT x64 Checked jitx86hwintrinsicnosimd

test Windows_NT x86 Checked jitincompletehwintrinsic
test Windows_NT x86 Checked jitx86hwintrinsicnoavx2
test Windows_NT x86 Checked jitx86hwintrinsicnosimd

test Ubuntu x64 Checked jitincompletehwintrinsic
test Ubuntu x64 Checked jitx86hwintrinsicnoavx2
test Ubuntu x64 Checked jitx86hwintrinsicnosimd

@4creators
Copy link
Author

4creators commented Mar 30, 2018

Changes from this PR allow to slightly increase test build performance (not visible in some of the runs as all jobs including unchanged coreclr runtime builds were running substantially slower than in previous #17161 PR) and enables to automatically generate coreclr/tests/src/dirs.proj file with all projects (.csrpoj and .ilproj files) present under coreclr/tests/src directory except for excluded ones.

  • Adding new project requires simple dirs.proj file regeneration by running coreclr/tests/scripts/generatedirsproj.cmd script.

  • All variables used during build are passed via run.cmd command line using new passthrough parameter ExtraTestParameters.

  • Removal of more than 12k project globbing and handling operations from MSBuild build time evaluation speeds up slightly test build

My plan for further improvements is to work on:

  • more comprehensive changes comprising test project refactoring to achieve further test build performance gains and simplification of test creation and addition.

  • porting all work to Linux what may not be as straightforward as I originally thought due functionality difference between full .NET Framowrk MSBuild used currently to build tests and dotnet MSBuild on Linux.

PTAL @AndyAyersMS @RussKeldorph @BruceForstall @weshaggard

@4creators
Copy link
Author

Unfinished OSX and Ubuntu arm CI jobs seems to be hung - they run for more than 14 hours at the moment I am writing this post.

@AndyAyersMS
Copy link
Member

I think the whole thing needs to be more or less automatic, based on declarative properties in the test projects and/or files in various points in the directory hierarchy.

That is, we should not require that people remember to run a script or do anything special when adding a new test (or removing or renaming a test, etc). Because people will forget these extra steps and then tests will not get built and so also won't get run -- and it is very easy to overlook this as tests that don't get run also don't fail.

Also these kinds of generated scripts tend to be a frequent source of merge conflicts, unless you are very careful to design the file format with ease of auto-merging in mind. So any sort of generative test project metadata needs to be built and consumed as part of the test build, not as a checked-in input to the test build.

As part of this there needs to be some automatic default that kicks in even if the test project or directory is missing whatever new property we use to partition, so that all tests are automatically put into some category and none are left out.

It is ok if because of this the execution is not optimally partitioned or can get out of balance over time as new tests are added. That we tolerate and fix when needed.

Ironically we caught a problem in the first version of this test partitioning because on arm we do keep a side test list, and so we noticed some tests weren't getting built. This side list getting out of sync is a frequent source of pain, though it is so rarely updated that merge conflicts are not a problem, instead we just miss running tests on arm until somebody notices and regenerates the thing.

@4creators
Copy link
Author

4creators commented Mar 30, 2018

we should not require that people remember to run a script or do anything special when adding a new test

Sure, will implement this action as a separate build target run during every test build as @weshaggard suggested before.

there needs to be some automatic default that kicks in even if the test project or directory is missing whatever new property we use to partition

This is implemented already in this version - all existing tests even from new directories will always be included into build. Partitioning is based on number of projects which will be built.

generated scripts tend to be a frequent source of merge conflicts, unless you are very careful to design the file format with ease of auto-merging in mind. So any sort of generative test project metadata needs to be built and consumed as part of the test build

I will try to improve on that, however, there is a trade off between how much we process before build is being run to how long build runs

@4creators 4creators changed the title Refine test discovery by scripting dirs.proj generation Refine test discovery and partition during test build with new target Mar 31, 2018

task.Exclude = new ITaskItem[]
{
new TaskItem(@"..\src\TestWrappers*\**\*.csproj"),
Copy link
Member

Choose a reason for hiding this comment

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

I don't like duplicating these sets of excluded test projects. We should figure out a way to share.

Copy link
Author

Choose a reason for hiding this comment

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

I am working on more comprehensive fix which will address @AndyAyersMS feedback and will include fix to the above duplication of excluded test projects

@jkotas
Copy link
Member

jkotas commented May 31, 2019

Closing stale PRs

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants