[tests] Extract support for building tests outside the repo into re-usable props+targets by radical · Pull Request #4923 · microsoft/aspire

radical · 2024-07-16T06:25:58Z

What

Extract bits required for running tests outside of repo into Aspire.RepoTesting.{props,targets}. And consolidate the various support bits (tasks/targets) spread around the repo. When running tests outside the repo they need some extra msbuild bits to allow building with aspire nugets instead of direct ProjectReferences to source projects.

Why

There is a need to have some other projects run out-of-repo too, for example, Aspire.Hosting.Testing.Tests, and upcoming Aspire.Playground.Tests.

Details

This provides support for running tests outside of the repo, for example on a helix agent.

For this you need the source of the tests, and any dependencies.
Instead of direct ProjectReferences to the various Aspire hosting, and component projects use
@(AspireProjectOrPackageReference).
- These are converted to ProjectReference when $(TestsRunningOutsideOfRepo) != true.
- But converted to PackageReference when $(TestsRunningOutsideOfRepo) == true.
To allow building such test projects, the build is isolated and patched to build outside the
repo by adding appropriate Directory.Build.{props,targets}, and Directory.Packages.props
- and using a custom nuget.config which resolves the Aspire packages from the locally built packages
- and a Directory.Packages.Versions.props is generated with PackageVersions taken from the repo
  - This also adds properties named in @(PropertyForHelixRun) from the repo, like $(NetCurrent)

Also:

Update other project to track the new changes

Microsoft Reviewers: Open in CodeFlow

… for more common use

…evant properties, and remove old custom tasks

…jects

.. `Aspire.Testing.Repo.{props,targets}`. From the comments: ``` This provides support for running tests outside of the repo, for example on a helix agent. - For this you need the source of the tests, and any dependencies. - Instead of direct `ProjectReferences` to the various Aspire hosting, and component projects use `@(ComponentReferenceForTests)`, and @(NonComponentReferenceForTests)`. - These are converted to `ProjectReference` when `$(TestsRunningOutsideOfRepo) != true`. - But converted to `PackageReference` when `$(TestsRunningOutsideOfRepo) == true`. - To allow building such test projects, the build is isolated and patched to build outside the repo by adding appropriate `Directory.Build.{props,targets}`, and `Directory.Packages.props` - and using a custom `nuget.config` which resolves the Aspire packages from the locally built packages - and a `Directory.Packages.Versions.props` is generated with PackageVersions taken from the repo - This also adds properties named in `@(PropertyForHelixRun)` from the repo, like `$(NetCurrent)` ```

.. which were there essentially to import Hosting.props/targets . Instead, now the `Aspire.Testing.Repo.{props,targets}` are imported by default, which decide how to get the hosting targets.

- `TestsRunningOutsideOfRepo` now means that the whole project builds from source outside the repo. - But for `EndToEnd` tests only supporting test project is built in that mode. - So, update the property and the corresponding `#define` to reflect that - And track changes from adding the new props/targets

.. as it is only used by that.

eerhardt

Some questions

eerhardt · 2024-07-16T22:10:02Z

Directory.Build.props

+    <TestsSharedDir>$([MSBuild]::NormalizeDirectory($(MSBuildThisFileDirectory), 'tests', 'Shared'))</TestsSharedDir>
+    <TestsSharedRepoTestingDir>$([MSBuild]::NormalizeDirectory($(TestsSharedDir), 'RepoTesting'))</TestsSharedRepoTestingDir>


(nit) can/should these go into tests/Directory.Build.props?

I had to put these here to be accessible from playground apps, which would get similar changes to remove the hosting targets import.

eerhardt · 2024-07-16T22:11:56Z

tests/Directory.Build.targets

+  <!-- Used for running one helix job per test class -->
+  <Target Name="_ExtractTestClassNames"


Why do we want one helix job per test class? That seems extreme, isn't it?

This is used for workload tests, where the tests are expensive. I'm just making it available for other projects.

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

eerhardt · 2024-07-16T22:15:17Z

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


+  <ItemGroup Condition="'$(_BuildForTestsRunningOutsideOfRepo)' == 'true'">
+    <None Include="..\testproject\**\*" Link="testassets\testproject\%(RecursiveDir)%(FileName)%(Extension)" CopyToOutputDirectory="PreserveNewest" />
+    <None Include="..\.editorconfig" Link="testassets\%(FileName)%(Extension)" CopyToOutputDirectory="PreserveNewest" />


Should copying the tests.editorconfig be part of the "infra"?

This exists in main. IIRC, this was become some test seemed to indirectly depend on this file being available. So, we are copying it here as part of preparing the output dir to be usable for out-of-repo builds.

And I didn't hit a need for this other projects that use out-of-repo (like future playground, and hosting.testing tests).

tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj

eerhardt · 2024-07-16T22:17:36Z

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

@@ -1,8 +1,7 @@
 <Project>
+  <PropertyGroup>
+    <IsAspireHost>true</IsAspireHost>


Is there a way to make it so this stays defined in the .csproj?

We can keep it there also! It is needed basically for this in tests/Shared/RepoTesting/Aspire.Testing.Repo.props which gets imported early - before the project file itself:

 <ImportGroup Condition="'$(RepoRoot)' != 'null' and '$(TestsRunningOutsideOfRepo)' != 'true' and ($(MSBuildProjectName.EndsWith('AppHost')) or '$(IsAspireHost)' == 'true')"> <Import Project="$(RepoRoot)src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.props" Condition="Exists('$(RepoRoot)src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.props')" /> </ImportGroup>

We are using ($(MSBuildProjectName.EndsWith('AppHost')) or '$(IsAspireHost)' == 'true') to determine whether it is an aspire host or not. And in this case the name does not help. Open to any suggestions of course.

eerhardt · 2024-07-16T22:18:55Z

tests/Shared/RepoTesting/Directory.Packages.Helix.props

+  </ItemGroup>
+
+  <ItemGroup Label="For tests">
+    <PackageVersion Include="xunit" Version="$(XunitVersion)" />


Why are the xunit packages needed to be defined here, but not any other external packages?

When a test project is built in the repo, Arcade adds these references automatically. But when we are building outside the repo, the Arcade targets are not used, thus we need to explicitly add these.

Any other external packages needed by a project would be added by those projects themselves. We add PackageVersion for them in ./tests/Shared/RepoTesting/Directory.Packages.Helix.props. Hm this should be added in a common README.md too.

tests/Shared/RepoTesting/Aspire.Testing.Repo.props

eerhardt · 2024-07-16T22:21:40Z

tests/testproject/TestProject.AppHost/TestProject.AppHost.csproj

-    <PackageReference Include="Aspire.Hosting.SqlServer" />
-    <PackageReference Include="Aspire.Hosting.Valkey" />
+  <ItemGroup>
+    <NonComponentReferenceForTests Include="Aspire.Hosting.Azure" IsAspireProjectResource="false" />


Why is this named NonComponentReferenceForTests? What does "Component" matter?

Lack of a better name! Please suggest one :)

Maybe drop Component and just have ReferenceForTests. Or OutsideOfRepoReference?

AbstractReference

After some offline discussion with @eerhardt I was able to combine the two into a single item AspireProjectOrPackageReference. Feel free to suggest a different name though :)

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

…gets # Conflicts: # tests/testproject/TestProject.AppHost/TestProject.AppHost.csproj

…gets # Conflicts: # tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj

tests/Directory.Build.props

…gets

…rhardt

… into AspireProjectOrPackageReference

eerhardt

LGTM

eerhardt · 2024-07-17T20:49:53Z

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

+    <_BuildForTestsRunningOutsideOfRepo Condition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(ContinuousIntegrationBuild)' == 'true'">true</_BuildForTestsRunningOutsideOfRepo>
+    <DeployOutsideOfRepoSupportFiles>$(_BuildForTestsRunningOutsideOfRepo)</DeployOutsideOfRepoSupportFiles>


Suggested change

<_BuildForTestsRunningOutsideOfRepo Condition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(ContinuousIntegrationBuild)' == 'true'">true</_BuildForTestsRunningOutsideOfRepo>

<DeployOutsideOfRepoSupportFiles>$(_BuildForTestsRunningOutsideOfRepo)</DeployOutsideOfRepoSupportFiles>

<DeployOutsideOfRepoSupportFilesCondition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(ContinuousIntegrationBuild)' == 'true'">true</DeployOutsideOfRepoSupportFiles>

Why do we need 2 separate properties for the same thing?

The condition is needed in multiple places, and it clarifies that this project is being built for running tests out-of-repo vs project itself being prepared to run out of repo.

Merging the PR, but if we decide for a change here then I can do it in the follow up.

eerhardt · 2024-07-17T20:58:33Z

tests/Shared/WorkloadTesting/data/nuget8.config

    <clear />
    <add key="built-local" value="%BUILT_NUGETS_PATH%" />
    <add key="dotnet-public" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json" />
+    <add key="dotnet-eng" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json" />


What comes from here?

I don't recall why I had to add that one :( I will remove this in the follow up PR, and see what fails.
Also, with using a single nuget.org for sources this will coming from the root nuget.config .

eerhardt · 2024-07-17T20:59:57Z

tests/Shared/WorkloadTesting/Aspire.Shared.WorkloadTesting.targets

+  <ItemGroup>
+    <Compile Include="$(MSBuildThisFileDirectory)*.cs" Link="WorkloadTestingCommon" />
+
+    <_DataFile Include="$(MSBuildThisFileDirectory)data\**\*" Link="testassets\%(RecursiveDir)%(FileName)%(Extension)" CopyToOutputDirectory="PreserveNewest" />


Can we call this something other than data? That doesn't seem like a great name.

TestSupportFiles maybe? I see we call it TestAssets elsewhere. Maybe that would be a good name for the folder in the repo as well?

Added as follow up in #4946

eerhardt · 2024-07-17T21:02:54Z

tests/Shared/nuget-with-package-source-mapping.config

@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="utf-8"?>


This is the 3rd time we have a nuget.config copied in the repo. Is there a way to use the root and just tweak it in places we need it to be special?

Yeah, we had a task doing that earlier because we needed to put in the path to the built nugets. But now we use %BUILT_NUGETS_PATH% as the source in the nuget.config for tests, so it always resolves it from the environment variable.

I'll bring the task back to get the sources from the root nuget.config, add the source for built-nugets, and add the package source mapping as needed.
Added this as follow up work - #4946 .

ghost added the area-integrations Issues pertaining to Aspire Integrations packages label Jul 16, 2024

radical mentioned this pull request Jul 16, 2024

[IGNORE] extracting out-of-repo changes to just tests, and making it reusable #4763

Closed

build-analysis bot mentioned this pull request Jul 16, 2024

Playwright tests failing on linux complaining about missing dependencies #4921

Closed

radical added 11 commits July 16, 2024 15:31

Move '_ExtractTestClassNames' target to tests/Directory.Build.targets…

9e519b3

… for more common use

Add new target to generate a Packages.Versions.props with all the rel…

6e07d55

…evant properties, and remove old custom tasks

move out more shared files from workload testing for use in other pro…

edf4b75

…jects

Remove the redundant Directory.Build.props for various projects

e577386

.. which were there essentially to import Hosting.props/targets . Instead, now the `Aspire.Testing.Repo.{props,targets}` are imported by default, which decide how to get the hosting targets.

Track changes in Workload tests

2a92aeb

Track changes in Aspire.Hosting.Tests

5ca6430

update docs

b591ceb

update docs

c532b48

Move Directory.Packages.Helix.props to RepoTesting

1fe6b17

.. as it is only used by that.

radical force-pushed the tests-out-of-repo-targets branch from 8616c15 to 1fe6b17 Compare July 16, 2024 19:34

radical marked this pull request as ready for review July 16, 2024 20:11

radical requested review from eerhardt and mitchdenny as code owners July 16, 2024 20:11

radical requested review from eerhardt, joperezr and sebastienros and removed request for eerhardt, joperezr and mitchdenny July 16, 2024 20:11

radical added the testing ☑️ label Jul 16, 2024

eerhardt reviewed Jul 16, 2024

View reviewed changes

radical and others added 3 commits July 16, 2024 18:53

Update tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj

b41f773

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

address review feedback from @ eerhardt

1326f38

address review feedback from @ eerhardt

065dc6e

radical added 5 commits July 16, 2024 19:16

Merge remote-tracking branch 'origin/main' into tests-out-of-repo-tar…

994695a

…gets # Conflicts: # tests/testproject/TestProject.AppHost/TestProject.AppHost.csproj

Merge remote-tracking branch 'origin/main' into tests-out-of-repo-tar…

a15b208

…gets # Conflicts: # tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj

fix merge

54b2037

Add README.md for Shared/RepoTesting

610bf1f

Add comment, and simple cleanup

5fe9ab1

radical commented Jul 17, 2024

View reviewed changes

tests/Directory.Build.props Outdated Show resolved Hide resolved

comment

2b3cce9

radical requested a review from eerhardt July 17, 2024 18:04

radical added 3 commits July 17, 2024 15:38

Merge remote-tracking branch 'origin/main' into tests-out-of-repo-tar…

45524c3

…gets

Rename Aspire.Testing.Repo to Aspire.RepoTesting - feedback from @ ee…

d805dc9

…rhardt

Combine NonComponentReferenceForTests, and ComponentReferenceForTests…

d5231fd

… into AspireProjectOrPackageReference

eerhardt approved these changes Jul 17, 2024

View reviewed changes

radical mentioned this pull request Jul 17, 2024

Follow up work for out-of-repo tests support #4946

Closed

2 tasks

radical merged commit 8605f40 into microsoft:main Jul 17, 2024

radical deleted the tests-out-of-repo-targets branch July 17, 2024 21:50

github-actions bot locked and limited conversation to collaborators Aug 17, 2024

		<TestsSharedDir>$([MSBuild]::NormalizeDirectory($(MSBuildThisFileDirectory), 'tests', 'Shared'))</TestsSharedDir>
		<TestsSharedRepoTestingDir>$([MSBuild]::NormalizeDirectory($(TestsSharedDir), 'RepoTesting'))</TestsSharedRepoTestingDir>

		<!-- Used for running one helix job per test class -->
		<Target Name="_ExtractTestClassNames"

		<_BuildForTestsRunningOutsideOfRepo Condition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(ContinuousIntegrationBuild)' == 'true'">true</_BuildForTestsRunningOutsideOfRepo>
		<DeployOutsideOfRepoSupportFiles>$(_BuildForTestsRunningOutsideOfRepo)</DeployOutsideOfRepoSupportFiles>

	<_BuildForTestsRunningOutsideOfRepo Condition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(ContinuousIntegrationBuild)' == 'true'">true</_BuildForTestsRunningOutsideOfRepo>
	<DeployOutsideOfRepoSupportFiles>$(_BuildForTestsRunningOutsideOfRepo)</DeployOutsideOfRepoSupportFiles>
	<DeployOutsideOfRepoSupportFilesCondition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(ContinuousIntegrationBuild)' == 'true'">true</DeployOutsideOfRepoSupportFiles>

Conversation

radical commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Details

Microsoft Reviewers: Open in CodeFlow

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radical Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

radical commented Jul 16, 2024 •

edited

Loading

eerhardt Jul 17, 2024 •

edited

Loading

radical Jul 17, 2024 •

edited

Loading