[tests] Add support for running tests on helix #1917

radical · 2024-01-28T01:11:50Z

Why?

Using helix to run tests would mainly be useful for End2End integration tests which would need controlled versions of dependencies, and that would be easier to do on helix than the build machines. This could be used to setup custom docker containers, and networks to simulate various scenarios and run e2e tests.

With this PR, tests will be run in two steps on CI:

On the build machine - which is the same as they are run currently.
This will be run in a single step, from the source repo with built
artifacts.
On helix - for this the tests are archived to .zip files, and then
sent to helix with any dependencies.

all the tests will still continue to use dotnet-coverage. But since
tests can run on more than one machine now, there is an additional CI
step that merges all the coverage xml files.

Details

This adds new properties that affect CI builds:

$(IsTestSupportProject) - which is for a project that should be run
as a test project itself.
$(RunTestsOnHelix) - which defaults to false
Also, a default .runsettings file is used to allow easier setting of
loggers, and other configuration for the tests.
The dashboard tests are explicitly marked for running on helix, just so we have at least one such test suite. This PR explicitly avoids supporting zero helix tests to ensure that the support does not rot, and helix step cannot silently fail because nothing got zipped.
Increase the timeout for CI from 30mins to 45mins.

With this PR, tests will be run in two steps on CI: 1. On the build machine - which is the same as they are run currently. This will be run in a single step, from the source repo with built `artifacts`. 2. On helix - for this the tests are archived to a `.zip` file, and then sent to helix with any dependencies. Additionally, tests can be marked to always be run on helix or the build machine. Other tests can be run on either. - a new property `$(DefaultToRunningTestsOnHelix)` when true would run all the tests that are not build-machine-only to run on helix. - when the property is !true, all the tests that can run build-machine will run there. This is the default. This adds new properties that affect CI builds: - `$(IsTestSupportProject)` - which is for a project that should be run as a test project itself. - `$(IsHelixOnlyTestProject)` - such a test can only be run on helix. - `$(IsNonHelixOnlyTestProject)` - such a project must always run on the build machine, IOW, with the source repo. - Also, a default `.runsettings` file is used to allow easier setting of loggers, and other configuration for the tests.

All the tests will still continue to use dotnet-coverage. But since tests can run on more than one machine now, there is an additional CI step that merges all the coverage xml files.

….json

radical · 2024-01-29T22:43:34Z

Ready for review

… agents

radical · 2024-01-30T18:31:22Z

Helix step can be slow to get agents sometimes, so increased the timeout to 45mins.

eng/pipelines/templates/BuildAndTest.yml

joperezr · 2024-02-01T05:44:06Z

eng/pipelines/templates/send-to-helix.yml

@@ -0,0 +1,94 @@
+# Please remember to update the documentation if you make changes to these parameters!


Loved the docs :).

Did we get these from some other repo? Mainly asking to see if we need to try to keep in sync.

The template is from eng/common/templates/steps/send-to-helix.yml, slightly modified here.

mmm I see, is there a way to instead use that template file and pass in some parameters to accommodate for our small modifications? If possible, I'd love to reduce the amount of yaml that we have to maintain.

This eng/common/templates/steps/send-to-helix.yml is a sample template from arcade, and it's not used directly. But I can make changes in that file itself.

Not sure what you mean when you say a "sample template". Tipically the templates under eng\common\templates are there so that repos can use them if they want/need as part of their build process. I don't want to block this PR on this, but I do want to make sure this is fixed. Can we at least log a tracking bug so that we go and add the necessary hooks in dotnet/arcade repo so that we can later ingest those changes here, and remove this file entirely so that we instead leverage the files from eng\common?

Opened dotnet/arcade#14470 .

joperezr · 2024-02-01T05:45:24Z

tests/Directory.Build.props

@@ -10,6 +10,16 @@
      CS1712: Type parameter 'type_parameter' has no matching typeparam tag in the XML comment on 'type_or_member' (but other type parameters do)
    -->
    <NoWarn>$(NoWarn);1573;1591;1712</NoWarn>
+
+    <IsHelixOnlyTestProject Condition="'$(IsHelixOnlyTestProject)' == ''">false</IsHelixOnlyTestProject>
+    <IsNonHelixOnlyTestProject Condition="'$(IsNonHelixOnlyTestProject)' == ''">false</IsNonHelixOnlyTestProject>


Given the above is a boolean, do we actually need this property? Can't we just infer based on the above?

We have 3 possible values - 1. helix only; 2. build only; 3. any . The two properties represent (1), and (2), and both being false means that (3) - no restrictions on where the tests can run.

We could use a string property with some fixed values like ['helix', 'non-helix', 'any'], but then you have the value strings in multiple places where you want to check, and that could get misspelled and cause tests to get silently skipped.

I'm not attached to any specific way of doing it though.

The name isn't great either, for the second property - IsNonHelixOnlyTestProject. An alternative would be IsBuildMachineOnlyTestProject sounds less clear.

In dotnet/aspnetcore, we have C# attributes to control whether a test runs on Helix or not.

https://github.com/dotnet/aspnetcore/blob/515987ac92f801fb995d88a20282a61571523aab/src/Testing/src/xunit/SkipOnCIAttribute.cs
https://github.com/dotnet/aspnetcore/blob/515987ac92f801fb995d88a20282a61571523aab/src/Testing/src/xunit/SkipOnHelixAttribute.cs
https://github.com/dotnet/aspnetcore/blob/515987ac92f801fb995d88a20282a61571523aab/src/Testing/src/xunit/SkipNonHelix.cs

Would it be possible to do this that way? Then we wouldn't need to worry about misspelling, since it would be C# code.

I see, yeah I think that as @eerhardt pointed out in a separate comment, I guess that the name is a bit confusing. I like his suggestion of dropping Only, or alternatively we can have something like OnlyRunOnHelix and DoNotRunOnHelix or something along those lines. Don't feel strongly about my suggestion haha, but I do think that the current version might be misleading.

tests/Directory.Build.targets

tests/send-to-helix.proj

tests/Aspire.Components.Common.Tests/Aspire.Components.Common.Tests.csproj

eng/pipelines/templates/BuildAndTest.yml

radical · 2024-02-01T06:45:14Z

I want to double check something about the test results, so please don't merge yet.

radical · 2024-02-01T08:25:40Z

I want to double check something about the test results, so please don't merge yet.

All good.

eerhardt · 2024-02-01T16:41:16Z

tests/Aspire.Hosting.Tests/Directory.Build.props

@@ -1,4 +1,7 @@
 <Project>
+  <PropertyGroup>
+    <IsNonHelixOnlyTestProject>true</IsNonHelixOnlyTestProject>


The name feels weird. It would seem that IsHelixOnlyTestProject=false should be the same thing as this.

If we want to say "this test should never run on Helix", I would drop "Only" in the name. IsNonHelixTestProject=true.

My thinking for having the only bit was to have a way to allow running tests on the build machine, or helix and be able to easily move those around in case we don't want to run on helix at all, or want to completely switch to helix. But we can skip that whole thing.

Without that behavior:

we can have a single property - RunTestsOnHelix - a bool, and it would control switching between helix, and the build machine. And this can default to false.

And I'll drop defaultToRunningTestsOnHelix

how does that sound?

Do we want a mixed mode too - one for which the test project is run in both places, and attributes on individual tests determine which ones to skip? If we don't control this at the project level, then we have to send all the tests to helix and let the attributes help with the skips.

(My opinion) - I would prefer to do it as close as possible to how dotnet/aspnetcore does it. Most of the engineers in this repo work in dotnet/aspnetcore as well. So it would be great if the mechanics are the same, so they don't have to relearn things jumping between repos.

eerhardt · 2024-02-01T16:47:03Z

tests/Directory.Build.props

@@ -10,6 +10,16 @@
      CS1712: Type parameter 'type_parameter' has no matching typeparam tag in the XML comment on 'type_or_member' (but other type parameters do)
    -->
    <NoWarn>$(NoWarn);1573;1591;1712</NoWarn>
+
+    <IsHelixOnlyTestProject Condition="'$(IsHelixOnlyTestProject)' == ''">false</IsHelixOnlyTestProject>
+    <IsNonHelixOnlyTestProject Condition="'$(IsNonHelixOnlyTestProject)' == ''">false</IsNonHelixOnlyTestProject>


In dotnet/aspnetcore, we have C# attributes to control whether a test runs on Helix or not.

https://github.com/dotnet/aspnetcore/blob/515987ac92f801fb995d88a20282a61571523aab/src/Testing/src/xunit/SkipOnCIAttribute.cs
https://github.com/dotnet/aspnetcore/blob/515987ac92f801fb995d88a20282a61571523aab/src/Testing/src/xunit/SkipOnHelixAttribute.cs
https://github.com/dotnet/aspnetcore/blob/515987ac92f801fb995d88a20282a61571523aab/src/Testing/src/xunit/SkipNonHelix.cs

Would it be possible to do this that way? Then we wouldn't need to worry about misspelling, since it would be C# code.

.. and use a single property named `$(RunTestsOnHelix)` that defaults to `false`.

…-helix.yml

eerhardt · 2024-02-05T23:04:00Z

eng/common/templates/steps/send-to-helix.yml

@@ -5,6 +5,8 @@ parameters:
  HelixBuild: $(Build.BuildNumber)       # required -- the build number Helix will use to identify this -- automatically set to the AzDO build number
  HelixTargetQueues: ''                  # required -- semicolon-delimited list of Helix queues to test on; see https://helix.dot.net/ for a list of queues
  HelixAccessToken: ''                   # required -- access token to make Helix API requests; should be provided by the appropriate variable group
+  HelixProjectPath: ''                   # required -- path to the project file to build


This is in eng/common, which is maintained in dotnet/arcade. Are we going to send a PR to arcade for this?

Does this get overwritten by arcade updates? If so, then I'll move this to a separate file as before, and just reference this original template in a comment. Even if not that maybe not touching this file would be good.

@joperezr I'm reverting to the earlier way of using a copy of that file. Lemme know if you have an objections to that.

eerhardt · 2024-02-06T00:04:54Z

eng/pipelines/azure-pipelines.yml

@@ -98,7 +98,7 @@ stages:
      jobs:

      - job: windows
-        timeoutInMinutes: 30
+        timeoutInMinutes: 45


FWIW, 30 mins was already pretty low and super close to the time it actually takes to build the repo in AzDO, so we were going to have to do this sooner rather than later.

that's from a recent CI run.

eerhardt · 2024-02-06T00:06:02Z

eng/pipelines/templates/BuildAndTest.yml

+    - ${{ if eq(parameters.isWindows, 'true') }}:
+      - pwsh: |
+            $_cov_filelist=$((Get-ChildItem -Path ${{ parameters.repoTestResultsPath }} -Recurse -Filter *.cobertura.xml -File | % { $_.FullName }) -join ' ')
+            echo "${{ parameters.dotnetScript }} dotnet-coverage merge $_cov_filelist -f cobertura -o ${{ parameters.repoTestResultsPath }}/$(Agent.Os)_$(Agent.JobName).cobertura.xml" > merge-cov.cmd
+            cmd /c merge-cov.cmd
+            rm merge-cov.cmd
+        displayName: Merge code coverage results
+
+    - ${{ if ne(parameters.isWindows, 'true') }}:


Why do we need to split here? Why can't the same script run on both windows and non-windows?

Might need a comment explaining why we are doing this.

I ran into my lack of pwsh skills :D

Looking at the yml that generates the report, it handles merging already. So, this can be simplified.

eerhardt · 2024-02-06T00:11:17Z

tests/send-to-helix.proj

+    <DotNetCliPackageType>sdk</DotNetCliPackageType>
+
+    <GlobalJsonContent>$([System.IO.File]::ReadAllText('$(RepoRoot)global.json'))</GlobalJsonContent>
+    <DotNetCliVersion>$([System.Text.RegularExpressions.Regex]::Match($(GlobalJsonContent), '(%3F&lt;="dotnet": ").*(%3F=")'))</DotNetCliVersion>


Who reads this value?

This is read by helix targets in Arcade.

eerhardt · 2024-02-06T00:12:40Z

tests/send-to-helix.proj

+  <PropertyGroup>
+    <Language>msbuild</Language>
+
+    <_workItemTimeout>00:20:00</_workItemTimeout>


What's the difference between this and the TestSessionTimeout in the .runsettings file? Why are they 2 different values?

The timeout defined in the yml is for the CI step, so the whole step that sends items to helix and waits on them. It would include the time taken while waiting for agents to be assigned, and the time take to run the tests on helix.

The timeout here is only for the time taken on helix, per helix item.

The timeout for the step is larger mainly to account for helix wait times, which can vary depending on how busy the queue is.

What is the timeout in the .runsettings file for?

What is the timeout in the .runsettings file for?

IIUC, that is a timeout for individual tests.

Also, it should have been 5minutes but I had bumped it up when I was getting timeouts and forgot to revert that.

…s/templates

The second stage processes the *cobertura.xml files to generate a report. And for that it collects these xml files from all the jobs. Earlier commits in this PR did not have unique names across jobs, like for linux, and windows. Which meant that the report generation step only handled one set of the xml files. Fix that by using the original unique filenames for the merged xml generated from the helix, and non-helix tests.

eerhardt · 2024-02-06T19:01:48Z

eng/pipelines/templates/BuildAndTest.yml

            /bl:${{ parameters.repoLogPath }}/build.binlog
            $(_OfficialBuildIdArgs)
    displayName: Build

  - ${{ if ne(parameters.skipTests, 'true') }}:
    - script: ${{ parameters.dotnetScript }} dotnet-coverage collect
              --settings $(Build.SourcesDirectory)/eng/CodeCoverage.config
+              --output ${{ parameters.repoTestResultsPath }}/NonHelixTests.cobertura.xml
+              "${{ parameters.buildScript }} -testnobuild -test -configuration ${{ parameters.buildConfig }} /bl:${{ parameters.repoLogPath }}/tests.binlog $(_OfficialBuildIdArgs)"
+      displayName: Run non-helix tests


Is it possible to run the non-helix tests and the helix tests in parallel? There shouldn't be a reason to serialize them, and that could speed up our CI legs.

We could add a wrapper project that builds both in parallel from msbuild. Should that be in a follow up PR?

Why would we need to build them in parallel? I'm talking about running them in parallel.

Why would we need a wrapper project? Does azdo allow for running 2 pipeline tasks at the same time?

By "build" in parallel I meant run the Test, and send-to-helix targets in parallel.

Does azdo allow for running 2 pipeline tasks at the same time?

AFAIK, this isn't supported.

Yeah, the thing that is allowed is to run two jobs or stages at the same time, but not steps within a job AFAIK. While I do think that could speed up things a bit, I also think it might be overkill to gain 9 mins or so that it takes in average to run the non helix tests first. I propose keeping things as is, and then perhaps look in the future here to see if this is something we need to invest on.

radical · 2024-02-06T22:13:33Z

@akoeplinger would you mind taking a look at this PR to check that we are doing this in an idiomatic way?

radical · 2024-02-08T17:24:04Z

Should we go ahead with this?

joperezr

Changes LGTM. Thanks @radical!

radical · 2024-02-09T02:32:56Z

I have merged main into the branch, and will wait for the CI results to confirm that this didn't break since the last build 2 days ago.

radical added 2 commits January 27, 2024 19:52

[tests] Add the CI side of support for running tests on helix

a906d21

All the tests will still continue to use dotnet-coverage. But since tests can run on more than one machine now, there is an additional CI step that merges all the coverage xml files.

dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Jan 28, 2024

radical requested review from eerhardt and joperezr January 28, 2024 02:16

radical marked this pull request as ready for review January 29, 2024 22:13

radical added 2 commits January 29, 2024 17:14

Merge remote-tracking branch 'origin/main' into helix-tests

3041206

send-to-helix.proj: extract dotnet-coverage version from dotnet-tools…

f3fdac2

….json

radical added 2 commits January 30, 2024 13:23

Merge remote-tracking branch 'origin/main' into helix-tests

1742b93

CI: Increase timeout from 30->45mins, to account for helix waiting on…

af66351

… agents