Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[ci] Parallelize and reduce overhead of MSBuild test stage. #7850

Merged
merged 11 commits into from
Mar 10, 2023

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Mar 2, 2023

Bring the AzDO parallelization from #7804 and environment setup improvements from #7832 to Xamarin.Android.Build.Tests.csproj based test suites. This includes both the main MSBuild stage and the Smoke Tests jobs.

Increases parallelization of all jobs as many were approaching ~90 minutes.

As there is no longer a place in the MSBuild stage to run Xamarin.Android.Tools.Aidl-Tests tests, it was moved to the macOS > Tests > APKs .NET stage. This suite should be fine to only run on Mac and not Windows. (Note this test assembly was also updated to .NET 7. This required moving it from Xamarin.Android-Tests.sln which is currently built with Mono which cannot build .NET 7+ projects. It now is built via Xamarin.Android.sln.)

Ensure test count stayed the same

Stage main This PR
Smoke > Mac 75 75
Smoke > Win 75 75
MSBuild > Mac 631 637
MSBuild > Win 631 637

4 excess tests are DotNetPublish(True,["net8.0", null, 33]), which gets skipped anyways because the platform is null
2 excess tests are MauiTargetFramework("net8.0",null,33), which gets skipped anyways because the platform is null

Results

Before:
image

After:
image

Note runtimes are not very stable. Sometimes you get an agent that is significantly slower than normal. 🤷‍♂️

@jpobst jpobst changed the title [ci] Parallelize and reduce overhead of MSBuiltd test stage. [ci] Parallelize and reduce overhead of MSBuild test stage. Mar 2, 2023
@jpobst jpobst force-pushed the msbuild-test-parallel branch from 36df0f8 to 0e3c2a3 Compare March 2, 2023 22:40
@jpobst jpobst force-pushed the msbuild-test-parallel branch from 0e3c2a3 to c16e1a4 Compare March 2, 2023 23:36
@jpobst jpobst force-pushed the msbuild-test-parallel branch from 0a67b86 to 4f9e917 Compare March 3, 2023 20:19
@jpobst jpobst force-pushed the msbuild-test-parallel branch 3 times, most recently from 01a3fee to a9476f6 Compare March 3, 2023 23:57
@jpobst jpobst force-pushed the msbuild-test-parallel branch from a9476f6 to 189c376 Compare March 4, 2023 01:37
@jpobst jpobst marked this pull request as ready for review March 8, 2023 20:24
Copy link
Member

@pjcollins pjcollins left a comment

Choose a reason for hiding this comment

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

One potential change related to stage-msbuild-tests.yaml might be nice, otherwise this looks good to me.

jobName: win_msbuild_tests
jobDisplayName: Windows > Tests > MSBuild
agentCount: 4
testFilter: cat != SmokeTests
Copy link
Member

Choose a reason for hiding this comment

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

I think we could move this block back into azure-pipelines.yaml to help cut down on the number of templates we use that aren't referenced more than once.

Copy link
Contributor Author

@jpobst jpobst Mar 8, 2023

Choose a reason for hiding this comment

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

My thought was that azure-pipelines.yaml is way too big and it takes too long to find the relevant stage yaml. If each stage was its own template file (stage-*.yaml) we could just go directly to the stage we want.

But that's a stylistic opinion and I'm not committed to it if others feel different. 😁

Copy link
Member

Choose a reason for hiding this comment

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

I wish yaml had some support around "go to reference" or "go to implementation", I'm often annoyed when working with multiple layers of nested templates having to jump back and forth between multiple page tabs rather than scrolling. That's also just my opinion though and am happy to go either way on this, maybe I can find some VS code extension to make my life easier...

Comment on lines +58 to +59
- name: ExcludedNUnitCategories
value: '& cat != DotNetIgnore & cat != HybridAOT & cat != MkBundle & cat != MonoSymbolicate & cat != PackagesConfig & cat != StaticProject & cat != SystemApplication'
Copy link
Member

Choose a reason for hiding this comment

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

This seems duplicate of DotNetNUnitCategories, why is the syntax here different?

Copy link
Contributor Author

@jpobst jpobst Mar 8, 2023

Choose a reason for hiding this comment

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

dotnet test uses one syntax, NUnit uses a different syntax.

When you use dotnet test the adapter does the translation. Because dotnet-test-slicer hits NUnit.Engine directly, we need to use NUnit's test selection language.

I put a little note here https://github.com/xamarin/xamarin-android/blob/f0a24bf67b3f0e56ed7a1d24c2e3789f542f0908/build-tools/automation/yaml-templates/run-sliced-nunit-tests.yaml#L3, but it doesn't bubble up to that variable specification.

@jpobst jpobst merged commit 928e7b9 into main Mar 10, 2023
@jpobst jpobst deleted the msbuild-test-parallel branch March 10, 2023 18:12
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 13, 2023
* main:
  [tests] `InstallAndroidDependenciesTest` uses `platform-tools` 34.0.1 (dotnet#7873)
  [ci] Parallelize and reduce overhead of MSBuild test stage. (dotnet#7850)
  [Xamarin.Android.Build.Tasks] Remove MAM targets from CoreResolve (dotnet#7868)
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 13, 2023
* main:
  [tests] `InstallAndroidDependenciesTest` uses `platform-tools` 34.0.1 (dotnet#7873)
  [ci] Parallelize and reduce overhead of MSBuild test stage. (dotnet#7850)
  [Xamarin.Android.Build.Tasks] Remove MAM targets from CoreResolve (dotnet#7868)
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 13, 2023
* main:
  [tests] `InstallAndroidDependenciesTest` uses `platform-tools` 34.0.1 (dotnet#7873)
  [ci] Parallelize and reduce overhead of MSBuild test stage. (dotnet#7850)
  [Xamarin.Android.Build.Tasks] Remove MAM targets from CoreResolve (dotnet#7868)
jpobst added a commit that referenced this pull request Mar 14, 2023
Context: #7850 (comment)

With the re-parallelization of our test suites (#7804, #7850), combined with the fact that most PR's end up running the full test suite anyways, the separate "Smoke Tests" jobs aren't really saving us much, if any, CI time.  We feel we would be better off using those additional agents to run the full test suite faster.

This PR removes the separate "Smoke Tests" jobs and runs all tests in the same jobs.

Additionally, as the `[Category ("SmokeTests")]` NUnit attribute is now only used by the Windows build stage to run in-tree tests, it has been cut back to the absolute bare minimum, saving ~45 minutes.  Note that all the tests will continue to be run against the Windows installer, this only runs fewer tests for the Windows in-tree tests.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants