Use `-noBuild` in second build steps of test jobs #39998

dougbu · 2022-02-04T22:15:52Z

building again in Publish target adds nothing in most cases
use -noBuildRepoTasks more consistently
- again, building the repo tasks wastefully repeated work done in previous build step
use -noBuildJava more consistently in quarantined pipelines
- Java tests aren't worth testing or building in these pipelines because they cannot be quarantined
remove -noRestore and -noBuildDeps when -noBuild specified; redundant
work around dotnet publish --no-build over-publishes from referenced projects #43835
- do not publish .cshtml files of most test asset projects when $(NoBuild) is set
update FunctionalTestWithAssets.targets to help Publish succeed when $(NoBuild) is set
- fixes build jobs in quarantined-pr pipeline where --no-build was already specified
for IIS test assets, perform all builds needed for their profiles within Build target
- move ReferenceTestTasks=false and comments about it to where property matters
- merge separate targets in FunctionalTest.props to make items available during (now separate) Publish

nits:

touch up RunHelix.ps1 console output
reorder command line arguments for consistency in changed parts of the YAML files
move NoBuild=true setting out of FunctionalTestWithAssets.props
use BeforeTargets in FunctionalTest.props to improve determinism

dougbu · 2022-02-07T21:39:24Z

Reasoning from a sample set of one, I'd say this PR speeds up PR validation significantly. Build #20220207.20 took only 1:20 while rolling builds of 'main' almost-always take 1:40 or more. Will rerun to see if this was a fluke since all that should have been removed was up-to-date checks.

dougbu · 2022-02-07T21:39:34Z

/azp run

azure-pipelines · 2022-02-07T21:39:57Z

Azure Pipelines successfully started running 3 pipeline(s).

dougbu · 2022-02-07T21:44:00Z

/fyi @mmitche (Mr. Build Perf 😺)

dougbu · 2022-02-07T21:47:09Z

eng/scripts/RunHelix.ps1

@@ -52,7 +52,8 @@ $env:BUILD_REPOSITORY_NAME="aspnetcore"
 $env:SYSTEM_TEAMPROJECT="aspnetcore"

 Write-Host -ForegroundColor Yellow "If running tests that need the shared Fx, run './build -pack -all' before this."
-Write-Host -ForegroundColor Yellow "And if packing for a different platform, add '/p:CrossgenOutput=false'."
+Write-Host -ForegroundColor Yellow "If everything is up-to-date, add '/p:NoBuild=true' to this command."
+Write-Host -ForegroundColor Yellow "Or, if only the test project is out-of-date, add '/p:BuildProjectReferences=false'."


@BrennanConroy these possibilities should speed up use of this script even more than setting $(p:DoNotRequireSharedFxHelix) 😸

dougbu · 2022-02-07T21:48:59Z

src/Mvc/test/Mvc.FunctionalTests/Microsoft.AspNetCore.Mvc.FunctionalTests.csproj

@@ -19,7 +19,8 @@
    <ProjectReference Include="..\..\shared\Mvc.Core.TestCommon\Microsoft.AspNetCore.Mvc.Core.TestCommon.csproj" />
    <ProjectReference Include="..\WebSites\*\*.csproj"
        Exclude="..\WebSites\ControllersFromServicesClassLibrary\ControllersFromServicesClassLibrary.csproj;
-          ..\WebSites\RazorBuildWebSite.*\RazorBuildWebSite.*.csproj" />
+          ..\WebSites\RazorBuildWebSite.*\RazorBuildWebSite.*.csproj;
+          ..\WebSites\RazorPagesClassLibrary\RazorPagesClassLibrary.csproj" />


minor nit I noticed while in the neighbourhood

dougbu · 2022-02-07T21:49:04Z

src/Mvc/test/WebSites/Directory.Build.targets

+<Project>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory)..\, Directory.Build.targets))\Directory.Build.targets" />
+
+  <!-- Work around https://github.com/dotnet/sdk/issues/23777. Reset ContentWithTargetPath items if not building. -->


Ugly bug I noticed while working on this PR and this workaround likely doesn't cover all the real-world cases. /cc @marcpopMSFT @dsplaisted

dougbu · 2022-02-07T21:50:36Z

src/Servers/IIS/IIS/test/FunctionalTest.props

  <ItemGroup>
    <Content Include="..\Common.FunctionalTests\Infrastructure\*.config" CopyToOutputDirectory="PreserveNewest" />
  </ItemGroup>

-  <Target Name="BuildAssets" AfterTargets="Build" Condition="'$(ExcludeFromBuild)' != 'true'">
-    <MSBuild Projects="@(ProjectReference)" Targets="PublishTestsAssets" SkipNonexistentTargets="true" BuildInParallel="True">
+  <Target Name="CopyAssets" BeforeTargets="Publish" Condition=" '$(ExcludeFromBuild)' != true ">


doing the two parts in separate targets worked when Publish always depended on Build

Is there any noticeable change here in behavior that we need to be aware of or is this just implementation details?

Nothing I could detect because the published files from the test assets aren't used in local tests.

.azure/pipelines/ci.yml

.azure/pipelines/helix-matrix.yml

.azure/pipelines/quarantined-pr.yml

.azure/pipelines/quarantined-tests.yml

HaoK · 2022-02-07T22:24:19Z

src/Servers/IIS/IIS/test/testassets/InProcessNewShimWebSite/InProcessNewShimWebSite.csproj

@@ -59,4 +63,13 @@
    </PackageReference>
    <Reference Include="xunit.assert" />
  </ItemGroup>
+
+  <!-- Repeat Build target for win-x64 to allow later Publish w/ NoBuild=true. -->


Just confirming that this already covers the Win32/arm64 cases as well, or if I'll need to revisit this section for arm64 later

If you add an arm64 profile to the @(TestAssetPublishProfile) item group, will need another target here too. Not sure how much the Condition will need to change w/o seeing what profile(s) you need to test. Of course, tests might have enough coverage w/o doing the self-contained thing for each possible Windows platform.

dougbu · 2022-02-08T00:16:27Z

Speed-up much less noticeable in build #20220207.25. That ran for 1:44 which at least is below the 2w average for these builds.

- building again in Publish target adds nothing in most cases - use `-noBuildRepoTasks` more consistently - again, building the repo tasks wastefully repeated work done in previous build step - use `-noBuildJava` more consistently in quarantined pipelines - Java tests aren't worth testing _or building_ in these pipelines because they cannot be quarantined - remove `-noRestore` and `-noBuildDeps` when `-noBuild` specified; redundant - work around https://github.com/dotnet/sdk/issues/23777 - do not publish .cshtml files of most test asset projects when `$(NoBuild)` is set - update FunctionalTestWithAssets.targets to help Publish succeed when `$(NoBuild)` is set - fixes build jobs in quarantined-pr pipeline where `--no-build` was already specified - for IIS test assets, perform all builds needed for their profiles within Build target - move `ReferenceTestTasks=false` and comments about it to where property matters - merge separate targets in FunctionalTest.props to make items available during (now separate) Publish nits: - touch up RunHelix.ps1 console output - reorder command line arguments for consistency in changed parts of the YAML files - move `NoBuild=true` setting out of FunctionalTestWithAssets.props - use `BeforeTargets` in FunctionalTest.props to improve determinism

dougbu · 2022-02-08T02:11:19Z

/fyi all, last validation build #20220207.35 took 1:29 overall (including waiting 6 minutes to get the Helix job a build agent). Seems there's some speed-up here but, like the old world, the builds don't all take the same amount of time.

ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 4, 2022

dougbu force-pushed the dougbu/dont.repeat.yourself branch 8 times, most recently from b8f2192 to c8780f0 Compare February 6, 2022 07:25

dougbu changed the title ~~Do not build when publishing in Helix CI jobs~~ Use -noBuild in second build steps of test jobs Feb 6, 2022

dougbu force-pushed the dougbu/dont.repeat.yourself branch 4 times, most recently from c182bc9 to 7c1f389 Compare February 7, 2022 19:31

dougbu requested review from HaoK and a team February 7, 2022 21:35

dougbu marked this pull request as ready for review February 7, 2022 21:35

dougbu requested review from BrennanConroy, halter73, javiercn, pranavkm and Tratcher as code owners February 7, 2022 21:35

dougbu commented Feb 7, 2022

View reviewed changes

wtgodbe reviewed Feb 7, 2022

View reviewed changes

.azure/pipelines/ci.yml Show resolved Hide resolved

wtgodbe reviewed Feb 7, 2022

View reviewed changes

.azure/pipelines/helix-matrix.yml Show resolved Hide resolved

wtgodbe reviewed Feb 7, 2022

View reviewed changes

.azure/pipelines/quarantined-pr.yml Show resolved Hide resolved

wtgodbe reviewed Feb 7, 2022

View reviewed changes

.azure/pipelines/quarantined-tests.yml Show resolved Hide resolved

wtgodbe approved these changes Feb 7, 2022

View reviewed changes

HaoK reviewed Feb 7, 2022

View reviewed changes

dougbu added 2 commits February 7, 2022 16:21

Add comments per review feedback

898347f

dougbu force-pushed the dougbu/dont.repeat.yourself branch from 7c1f389 to 898347f Compare February 8, 2022 00:26

dougbu enabled auto-merge (squash) February 8, 2022 00:29

dougbu merged commit 633b3d7 into dotnet:main Feb 8, 2022

dougbu deleted the dougbu/dont.repeat.yourself branch February 8, 2022 02:06

ghost added this to the 7.0-preview2 milestone Feb 8, 2022

dougbu mentioned this pull request Feb 23, 2022

Add missing BeforeTargets for IIS local testing #40379

Merged

halter73 mentioned this pull request Apr 5, 2022

Remove BuildX64 target #41040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `-noBuild` in second build steps of test jobs #39998

Use `-noBuild` in second build steps of test jobs #39998

dougbu commented Feb 4, 2022 •

edited

Loading

dougbu commented Feb 7, 2022

dougbu commented Feb 7, 2022

azure-pipelines bot commented Feb 7, 2022

dougbu commented Feb 7, 2022

dougbu Feb 7, 2022

dougbu Feb 7, 2022

dougbu Feb 7, 2022

dougbu Feb 7, 2022

HaoK Feb 7, 2022

dougbu Feb 8, 2022

HaoK Feb 7, 2022

dougbu Feb 8, 2022

dougbu commented Feb 8, 2022

dougbu commented Feb 8, 2022

Use -noBuild in second build steps of test jobs #39998

Use -noBuild in second build steps of test jobs #39998

Conversation

dougbu commented Feb 4, 2022 • edited Loading

dougbu commented Feb 7, 2022

dougbu commented Feb 7, 2022

azure-pipelines bot commented Feb 7, 2022

dougbu commented Feb 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Feb 8, 2022

dougbu commented Feb 8, 2022

Use `-noBuild` in second build steps of test jobs #39998

Use `-noBuild` in second build steps of test jobs #39998

dougbu commented Feb 4, 2022 •

edited

Loading