-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Restore Seperate test builds, but fix broken pipe #35378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4b91016
ca27831
5b2f164
58e6116
2154df3
e76acf9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,10 @@ parameters: | |
| variables: {} | ||
| pool: '' | ||
| runtimeFlavorDisplayName: 'CoreCLR' | ||
| # If true, tests were built in two phases | ||
| # We will depend on both the TargetGeneric tests which are for AnyOS AnyCPU | ||
| # as well as the TargetSpecific test. Both sets of tests will be run | ||
| testBuildPhased : false | ||
|
|
||
| ### Test run job | ||
|
|
||
|
|
@@ -57,6 +61,12 @@ jobs: | |
| - '${{ parameters.runtimeFlavor }}_common_test_build_p0_${{ parameters.managedTestBuildOsGroup }}${{ parameters.managedTestBuildOsSubgroup }}_${{ parameters.archType }}_${{parameters.buildConfig }}' | ||
| - ${{ if ne(parameters.testGroup, 'innerloop') }}: | ||
| - '${{ parameters.runtimeFlavor }}_common_test_build_p1_${{ parameters.managedTestBuildOsGroup }}${{ parameters.managedTestBuildOsSubgroup }}_${{ parameters.archType }}_${{parameters.buildConfig }}' | ||
| - ${{ if eq(parameters.testBuildPhased, true) }}: | ||
| # Also depend on AnyOS AnyCPU tests | ||
| - ${{ if eq(parameters.testGroup, 'innerloop') }}: | ||
| - '${{ parameters.runtimeFlavor }}_common_test_build_p0_AnyOS_AnyCPU_${{parameters.buildConfig }}' | ||
| - ${{ if ne(parameters.testGroup, 'innerloop') }}: | ||
| - '${{ parameters.runtimeFlavor }}_common_test_build_p1_AnyOS_AnyCPU_${{parameters.buildConfig }}' | ||
| - ${{ if ne(parameters.stagedBuild, true) }}: | ||
| - ${{ format('{0}_product_build_{1}{2}_{3}_{4}', parameters.runtimeFlavor, parameters.osGroup, parameters.osSubgroup, parameters.archType, parameters.buildConfig) }} | ||
| - ${{ if ne(parameters.liveLibrariesBuildConfig, '') }}: | ||
|
|
@@ -189,6 +199,16 @@ jobs: | |
|
|
||
| # Download and unzip managed test artifacts | ||
| - ${{ if ne(parameters.corefxTests, true) }}: | ||
| # First download and unzip AnyOS AnyCPU test (if needed) | ||
| - ${{ if eq(parameters.testBuildPhased, true) }}: | ||
| - template: /eng/pipelines/common/download-artifact-step.yml | ||
| parameters: | ||
| unpackFolder: '$(managedTestArtifactRootFolderPath)' | ||
| artifactFileName: '$(managedGenericTestArtifactName).tar.gz' | ||
| artifactName: '$(managedGenericTestArtifactName)' | ||
| displayName: 'generic managed test artifacts' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be worth 1-2 more comments explaining the generic vs. specific test management. It would be probably too nitpicky to alternate the displayName of the pre-existing download between "managed test artifacts" and "generic managed test artifacts" but we should try to keep these scripts easy to understand and possibly refactor. |
||
|
|
||
| # Download and unzip target specific tests | ||
| - template: /eng/pipelines/common/download-artifact-step.yml | ||
| parameters: | ||
| unpackFolder: '$(managedTestArtifactRootFolderPath)' | ||
|
|
||
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.
Why do we have to still depend on the non-phased build if this is to 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.
testBuildPhasedhere means that the tests were built in two phases.TargetGenericandTargetSpecifictests. We join them here by unzipping into the same directory.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.
So if testBuildPhased==true we still depend on:
- '${{ parameters.runtimeFlavor }}_common_test_build_p0_${{ parameters.managedTestBuildOsGroup }}${{ parameters.managedTestBuildOsSubgroup }}_${{ parameters.archType }}_${{parameters.buildConfig }}'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.
Yes
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.
@trylek And I discussed this briefly during the original review.
We may eventually refactor and build the
TargetSpecifictests as part ofrun-test-job. He was suggesting collecting stats to determine whether the extra build legs added value or not.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.
That sounds like a reasonable, interesting idea.
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.
In fact, I was mostly worried (based on earlier discussions with @jashook) that adding too many legs can be counterproductive by increasing the load on machine allocation and provisioning. I however think that having the specific test build legs initially separate is probably a good idea to get an initial reading on the perf characteristics.
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.
Right, but if we refactor and just have build-test legs for the generic tests and then the target specific builds are done at the run-test-job, before sending to helix, we would be reducing the number of legs we will have by the number of target specific tests we currently have, so I think it would improve perf and reliability.
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.
I suspect you are right.
Filtering to build the TargetSpecific tests is currently slow. But if we make msbuild improvements this may go away.