[tests] Add support for running EndToEnd tests on helix with the `aspire` workload #2534

radical · 2024-02-29T04:07:51Z

Problem:

We want to run some EndToEnd tests on CI, which can dotnet run an aspire project,
and allow individual tests to interact with the services.
This requires:
- Ability to build, and run an aspire project - IOW, a sdk with the aspire workload installed.
- docker
Also, allow using TestProject.* in tests/testproject, in two modes:
- in-tree test run which directly reference aspire projects, and repo targets
- out-of-tree test runs which uses a SDK+workload based on local build output

Solution:

SDK+workload

SDK with version from global.json is installed into artifacts/bin/dotnet-{none,latest}
- The aspire workload is installed into the sdk using the nuget packages in artifacts/packages with a version like 8.0.0-{dev,ci}
this also allows running tests that do dotnet new aspire-starter; dotnet run same as a user
this utilizes an internal nuget from dotnet/runtime

TestProject

This can switch between the two test run modes using the msbuild property $(TestsRunningOutsideRepo)
- when running in-tree the test project directly references hosting targets, and aspire projects via ProjectReference
- when running out-of-tree the ProjectReferences and imports are replaced with PackageReferences to the Aspire nugets
Default is to run in-tree

Helix

The tests are built on the build machine
The testproject, and the sdk+workload is sent to helix
- where the tests run using dotnet from the sdk+workload path
Since docker is needed to helix, this is enabled only for Linux in this PR. Blocked on Need docker installed on the helix images for Windows, to allow individual tests to create containers dnceng#2067 for windows support.

TL;dr or How do I use this?

Steps to prepare for `out-of-tree` runs

.\build.cmd -pack - to build all the nugets
dotnet build tests/Aspire.EndToEnd.Tests/Aspire.EndToEnd.csproj /t:InstallWorkloadUsingArtifacts /p:Configuration=<config>
- this will install the sdk, and the aspire workload using the nugets from artifacts/packages

at this point the sdk is usable outside the repo too. Note: you need to add artifacts/packages/$(Configuration)/Shipping as a nuget source so the locally built packages can be picked up.

Using it from VS

No difference when running the EndToEnd tests in in-tree mode
For switching to out-of-tree add <TestsRunningOutsideRepo>true</TestsRunningOutsideRepo> to tests/Aspire.EndToEnd.Tests/Directory.props before any imports.
- tests cannot be run at this point as they will fail complaining about artifacts/bin/dotnet-latest being missing
- Install the sdk+workload following the steps above
- Run/debug the tests normally now, and they will be using the sdk
- Also note that in this case the testproject is run from the bindir for Aspire.EndToEnd.Tests, so a path like artifacts/bin/Aspire.EndToEnd.Tests/Debug/net8.0/testassets/testproject/

Using it from command line

No difference when running the EndToEnd tests in in-tree mode
Install the sdk+workload following the steps above
When running the tests you can either:
- set <TestsRunningOutsideRepo>true</TestsRunningOutsideRepo> to tests/Aspire.EndToEnd.Tests/Directory.props before any imports
- or set the environment variable TestsRunningOutsideRepo=true

Other details

tests/Shared/WorkloadTesting - has sources essentially from dotnet/runtime's Wasm.Build.Tests
TestProject supports skipping individual resources like --skip-resources oracledatabase,cosmos
.runsettings with 15min timeout
xunit.runner.json - to get diagnostic messages
The core infrastructure is not specific for Aspire.EndToEnd.Tests, and will get used by other test projects in the future

Microsoft Reviewers: Open in CodeFlow

- add a xunit.runner.json, .runsettings - update tests to skip resources as needed - update test fixture to work with sdk+workload for testing

- reference workload.testing nuget from dotnet/runtime - Extra some workload properties needed to a Workload.props, used in more than once place now - Microsoft.NET.Sdk.Aspire: set `PackageType=''` to the manifest nuget is not `dotnetplatform`, and can be installed directly by the workload testing tasks.

radical · 2024-02-29T04:11:56Z

Questions:
- should a forced container cleanup be done at the end?
- nuget-cache - Is it OK to expect the dev to manually delete aspire packages from the cache?
  - PR uses a new nuget-cache path for every run, which is slow on slow connections, but good on fast ones
- Microsoft.NET.Sdk.Aspire: set PackageType='' to the manifest nuget is notdotnetplatform, and can be installed directly by the workload testing tasks. Check that nothing else is affected, and this is an acceptable change.

TODO:

nuget.helix.config - Use the root nuget.config and insert the built-nugets source

joperezr · 2024-03-01T00:21:58Z

Talked offline with @radical, marking as ready for review as this is ready for feedback.

eerhardt · 2024-03-01T15:46:02Z

NuGet.config

  </packageSources>
  <packageSourceMapping>
+    <!-- used for installing test workloads for resolving aspire workload manifest -->
+    <packageSource key="nuget-local">


Where is this package source defined?

https://github.com/dotnet/runtime/blob/f135699310bccfa83b3b441d2f4f1571fe8df480/src/tasks/WorkloadBuildTasks/InstallWorkloadFromArtifacts.cs#L262

I agree that it does seem odd that you can't directly see what this links up to in the repo. I can have the package source name be passed to the task, which would be in aspire repo.

Does this throw a warning at all on the initial restore before the task has a chance to replace the source? Did we also consider manually passing in -s <path> to a manual invocation of restore instead of having this in the config?

No warnings. Wouldn't -s path only add a package source but not the mapping, and because of that restoring from the source would fail. That is what I hit when I removed this mapping.

Once dotnet/runtime#99277 is merged this entry can be removed, and it would be added at build time.

Directory.Packages.props

tests/nuget.helix.config

eerhardt · 2024-03-01T15:57:39Z

tests/send-to-helix-workload-tests.proj

+    <HelixPreCommand Condition="'$(OS)' == 'Windows_NT'" Include="set SDK_FOR_WORKLOAD_TESTING_PATH=%HELIX_CORRELATION_PAYLOAD%\%SDK_DIR_NAME%" />
+    <HelixPreCommand Condition="'$(OS)' != 'Windows_NT'" Include="export SDK_FOR_WORKLOAD_TESTING_PATH=$HELIX_CORRELATION_PAYLOAD/$SDK_DIR_NAME" />
+
+    <HelixPreCommand Condition="'$(OS)' == 'Windows_NT'" Include="set TestsRunningOutOfTree=true" />


I'm not sure how much I like the term "running out of tree". I think there are 2 things about it I don't like:

I think I'd rather use the term "repo" than "tree".

"Running out of" actually makes it sound like that's where the tests are from. Like the phrase "fighting out of the blue corner".

Possible suggestions:

TestingOutsideOfSourceRepo

TestsNotInRepo

TestsNotContainedInRepo

I think at a minimum change OutOf to OutsideOf and I'll be OK with the name. TestsRunningOutsideOfTree

tests/Aspire.EndToEnd.Tests/ProjectInfo.cs

eerhardt · 2024-03-01T16:14:44Z

tests/Aspire.EndToEnd.Tests/Aspire.EndToEnd.Tests.csproj

+    <!-- no docker support on helix/windows yet -->
+    <RunTestsOnHelix Condition="'$(OS)' != 'Windows_NT'">true</RunTestsOnHelix>
+    <SkipTests Condition="'$(OS)' == 'Windows_NT'">true</SkipTests>


Can we just say RunTestsOnHelix=true all the time, and then do these checks inside the tests? Similar to how I did it in RabbitMQ in #2345. It just feels cleaner that way.

Yes we can do it like that also. The reason for "also" is that if we don't have any tests that would get run, then not skipping the whole thing would mean:

installing sdk+workload - extra step on CI

sending this work item to helix with the big payload (sdk) for nothing to run, which dnceng kinda frowns upon.

I think we can add the attributes also, and for the specific case (windows) where nothing can run, we skip in the project file too. Thoughts?

How long until we are able to run these tests on Windows?

eerhardt · 2024-03-01T16:17:30Z

tests/Aspire.EndToEnd.Tests/Aspire.EndToEnd.Tests.csproj

+
+    <BuiltNuGetsDir>$(ArtifactsShippingPackagesDir)</BuiltNuGetsDir>
+    <PackageVersionForWorkloadManifests>$(PackageVersion)</PackageVersionForWorkloadManifests>
+    <DefineConstants Condition="'$(TestsRunningOutOfTree)' == 'true'">TESTS_RUNNING_OUT_OF_TREE;$(DefineConstants)</DefineConstants>


Can we get this from an env var when the test runs so we don't have to have an #if in the code?

I had to add this just for the VS run, so you need to set only one property in one file to have the tests switch.
Using env var works perfectly well when running from command line, but I don't know what the workflow can be for setting it in case of VS. Open to suggestions.

eerhardt · 2024-03-01T16:20:09Z

eng/pipelines/templates/BuildAndTest.yml

+                -t:InstallWorkloadUsingArtifacts
+                -bl:${{ parameters.repoLogPath }}/WorkloadInstallForTesting.binlog
+                $(Build.SourcesDirectory)/tests/Aspire.EndToEnd.Tests/Aspire.EndToEnd.Tests.csproj


Is it weird to pick the EndToEnd tests to have this "InstallWorkloadUsingArtifacts" target? It seems like something that should be coming from an "infrastructure" project. Not one of the test projects.

Yes, I was thinking of fixing it in a follow up though. But I can try to do it here too.

I'll keep this for a follow up PR, unless you think it should be done here itsefl

eerhardt · 2024-03-01T16:24:58Z

tests/testproject/TestProject.AppHost/TestProgram.cs


 public class TestProgram : IDisposable
 {
    private TestProgram(string[] args, Assembly assembly, bool includeIntegrationServices, bool includeNodeApp, bool disableDashboard)
    {
-        if (args.Contains("--disable-dashboard"))
+        IList<TestResourceNames> resourcesToSkip = [];


Do we actually need this resourcesToSkip? If we just skipped the cosmos and oracle tests, wouldn't that be enough? Yes it would still spin up those containers during the tests, but nothing would try to connect to them.

I don't really like all these if conditions everywhere. It really clutters up the code.

We have one test that health checks on the whole thing, and that would continue to fail.

I don't really like all these if conditions everywhere. It really clutters up the code.

agree!

tests/Aspire.EndToEnd.Tests/IntegrationServicesFixture.cs

…review.3.24151.4)

# Conflicts: # NuGet.config

eng/Version.Details.xml

eng/Versions.props

joperezr · 2024-03-01T22:17:29Z

eng/workloads/workloads.csproj

@@ -10,8 +10,6 @@
    <VSTemp>$(WorkloadIntermediateOutputPath)VS/</VSTemp>
    <WorkloadOutputPath>$(ArtifactsBinDir)workloads/</WorkloadOutputPath>
    <WorkloadOutputPath Condition="'$(workloadArtifactsPath)' != ''">$(workloadArtifactsPath)/</WorkloadOutputPath>
-    <PackageSource>$(ArtifactsShippingPackagesDir)</PackageSource>
-    <PackageSource Condition="'$(workloadPackagesPath)' != ''">$(workloadPackagesPath)/</PackageSource>


Does this mean that this cannot get overriden any longer? cc: @danegsta in case he is aware if we depend on this.

Where does $(workloadPackagesPath) get set? I didn't find any reference to it in the repo.

Should be fine as long as the workload still builds correctly

eng/dcppack/dcppack.csproj

eng/dashboardpack/dashboardpack.csproj

This reverts commit 5be48e0.

radical · 2024-03-06T02:51:12Z

@eerhardt I tried to add oracledb back, but it failed because the test timeout was 5minutes. I think we continue with that in a follow up PR, so reverting that change.

joperezr · 2024-03-06T17:34:56Z

eng/dashboardpack/dashboardpack.csproj

@@ -26,7 +26,7 @@

  <Target Name="Build" />

-  <Target Name="BeforeBuild" BeforeTargets="Build">
+  <Target Name="BeforeBuild" BeforeTargets="_GetPackageFiles">


Why do we need this change? In general, it's best to avoid depending on "private" tragets (Targets that start with an underscore)

I wanted to limit this target to only when we are generating a nuget.

Is this target needed only for generating the zip file? Is the zip file used for the dashboard docker image?

This target mainly accomplishes two things:

Adds the right content to the each of the dashboard packs (which actually may no longer be needed now that we have the dashboard as a tool) cc: @danegsta

Zips up the published output as that will be uploaded as a blob so that it can then be used to build the docker image for the dashboard.

eng/dcppack/dcppack.csproj

joperezr · 2024-03-06T18:12:02Z

eng/pipelines/templates/BuildAndTest.yml

@@ -21,6 +21,7 @@ parameters:
 steps:
  - script: ${{ parameters.buildScript }}
            -restore -build
+            -pack


We are already running pack as a separate step (check line 98 in this file). Can we either change the ordering so that happens before the steps that run the tests, or alternatively just remove that whole step if it is no longer needed?

Good point. This pack could conflict with the subsequent pack, since that one is signed. I will investigate this in a separate PR. And it will really be needed only when we have this running on windows again.
For now, I will add -pack only for linux, where the separate pack step is not defined at all.

joperezr

Added a few comments/suggestions but the rest LGTM.

https://learn.microsoft.com/en-us/nuget/reference/msbuild-targets#targetsfortfmspecificcontentinpackage addresses review feedback from @ joperezr

…eview feedback from @ joperezr

…ning tests

…testing

radical · 2024-03-07T00:52:59Z

Follow-up work is being tracked in #2687 .

…et stuck waiting for i t

# Conflicts: # tests/testproject/TestProject.IntegrationServiceA/Program.cs

radical added 10 commits February 28, 2024 22:45

testproject: Switch between Project and Package references

9770c7d

testproject: Add support for skipping resources

f62530d

Aspire.EndToEnd.Tests: update

99f6a42

- add a xunit.runner.json, .runsettings - update tests to skip resources as needed - update test fixture to work with sdk+workload for testing

Add common infrastructure code for workload testing

116f4d0

Add bits to support preparing tests for helix

ce61861

remaining tidbits

39472c8

CI: Build sdk+workload for sending to helix

949294f

testproject.AppHost: use Debug logging

ef410ff

cleanup

9aaf1ec

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

radical requested review from eerhardt and joperezr February 29, 2024 04:08

joperezr marked this pull request as ready for review March 1, 2024 00:22

eerhardt reviewed Mar 1, 2024

View reviewed changes

radical added 7 commits March 1, 2024 14:43

Use the version property in Directory.Packages.props

8649005

address review feedback from @ eerhardt

a976089

Rename TestsRunningOutOfTree -> TestsRunningOutsideOfRepo

94a50c4

Bump Microsoft.NET.Runtime.WorkloadTesting.Internal to latest(9.0.0-p…

56c4f2d

…review.3.24151.4)

address review feedback from @ eerhardt

800ed52

Merge remote-tracking branch 'origin/main' into e2e-testing-helix

28430fd

# Conflicts: # NuGet.config

fix merge

e5afe1b