[tests] Add `Aspire.Playground.Tests` #5208

radical · 2024-08-07T01:40:01Z

Add wrapper tests for apps in playground/.

The tests are based on https://github.com/dotnet/aspire-samples/blob/aee52a7a08ca3433bd1e54e8a90a7febdc852ebd/tests/SamplesIntegrationTests/AppHostTests.cs .
And test infrastructure from https://github.com/dotnet/aspire-samples/tree/aee52a7a08ca3433bd1e54e8a90a7febdc852ebd//tests/SamplesIntegrationTests/Infrastructure has been copied to the project here

This effectively increases coverage for various components like Qdrant, and Seq, and their hosting bits.

Details:

Skip dashboard project reference to playground apps when running building for tests
Use MapDefaultEndpoints to add /alive and /health endpoints needed for testing
Rename playground/TestShop/ServiceDefaults to playground/TestShop/TestShop.ServiceDefaults to disambiguate the project binaries in artifacts
Only a few of the playground apps are being tested here. More will be added in follow up PRs.

Contributes to #4297 .

Microsoft Reviewers: Open in CodeFlow

.. to disambiguate the project binaries in `artifacts`

Playground apps add a reference to the Dashboard project for local debugging. Skip that when building for tests.

…nts needed for testing

.. with test infrastructure (`tests/Aspire.Playground.Tests/Infrastructure/`) taken from aspire-samples repo.

.. under a new category - `buildforhelixtests`. This is for any tests that expect to build the tests project itself on helix, and then run it there.

.. props file. This allows project builds where test packages are added by default to work by providing the package versions for those packages.

mitchdenny · 2024-08-07T02:41:49Z

As part of this it would be good to ensure that the the content of aspire-manifest.json has not changed in each of the playground projects. We want to detect if the apphost is generating a manifest different to what is checked in so that we can identify changes that we didn't intend (it also helps with the review process).

radical · 2024-08-07T05:33:31Z

As part of this it would be good to ensure that the the content of aspire-manifest.json has not changed in each of the playground projects. We want to detect if the apphost is generating a manifest different to what is checked in so that we can identify changes that we didn't intend (it also helps with the review process).

Added to #5210

tests/Aspire.Playground.Tests/AppHostTests.cs

playground/TestShop/TestShop.sln

playground/Directory.Build.targets

tests/Aspire.Playground.Tests/README.md

eerhardt · 2024-08-08T17:13:08Z

tests/Aspire.Playground.Tests/.runsettings

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


Do we need this copy of this file? Can we use the default one in the root?

This is to allow the source being buildable from outside the repo, as we wouldn't have eng/testing/.runsettings available there.

The alternative would be to copy it for the archive, and also have a <RunSettingsFilePath Condition="'$(RepoRoot)' == ''">$(MSBuildThisFileDirectory).runsettings</RunSettingsFilePath>.

Can we just have the root .runsettings file checked into the repo, and then the infrastructure copies it to the correct place in the helix payload?

This will get covered by the follow up task of building on the build machine, and only running on helix.

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

tests/helix/send-to-helix-buildonhelixtests.targets

tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj

eerhardt · 2024-08-08T17:21:41Z

tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj

+    <Compile Include="$(TestsSharedDir)WorkloadTesting\EnvironmentVariables.cs" />
+    <Compile Include="$(TestsSharedDir)Logging\*.cs" />
+
+    <AspireProjectOrPackageReference Include="Aspire.Hosting" />


Do we rebuild the Aspire.Playground.Tests in Helix? If so, why? Why don't we build this project on the build machine, and then run the already built tests on Helix?

hmm I think this can be reworked so this project builds on the build machine. Then we can build the playground projects on helix, and point the tests to the bindir for these.

This will need some reworking. I have added it as a follow up task in #5210 . We can get the tests checked-in before that.

eerhardt · 2024-08-08T17:22:59Z

tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj

+    <ProjectReference Include="$(PlaygroundSourceDir)ProxylessEndToEnd/ProxylessEndToEnd.AppHost/ProxylessEndToEnd.AppHost.csproj" AdditionalProperties="SkipDashboardProjectReference=true" />
+    <ProjectReference Include="$(PlaygroundSourceDir)Qdrant/Qdrant.AppHost/Qdrant.AppHost.csproj" AdditionalProperties="SkipDashboardProjectReference=true" />
+    <ProjectReference Include="$(PlaygroundSourceDir)TestShop/TestShop.AppHost/TestShop.AppHost.csproj" AdditionalProperties="SkipDashboardProjectReference=true" />
+    <ProjectReference Include="$(PlaygroundSourceDir)mongo/Mongo.AppHost/Mongo.AppHost.csproj" AdditionalProperties="SkipDashboardProjectReference=true" />
+    <ProjectReference Include="$(PlaygroundSourceDir)mysql/MySqlDb.AppHost/MySqlDb.AppHost.csproj" AdditionalProperties="SkipDashboardProjectReference=true" />
+    <ProjectReference Include="$(PlaygroundSourceDir)nats/Nats.AppHost/Nats.AppHost.csproj" AdditionalProperties="SkipDashboardProjectReference=true" />
+    <ProjectReference Include="$(PlaygroundSourceDir)seq/Seq.AppHost/Seq.AppHost.csproj" AdditionalProperties="SkipDashboardProjectReference=true" />


Why only some of these? Why not all?

Should we do it like the aspire-samples does it?

Because not all of them work right now. Issues like #5219, #5218, some of the projects requiring azure creds, some failing with other issues that need to be investigated.

tests/Aspire.Playground.Tests/Infrastructure/PasswordGenerator.cs

…Tests.csproj

…Aspire.Hosting instead of duplicating from aspire-samples

playground/AzureStorageEndToEnd/AzureStorageEndToEnd.AppHost/Program.cs

joperezr · 2024-08-08T23:00:21Z

playground/mongo/Mongo.ApiService/Program.cs

@@ -12,6 +12,7 @@

 var app = builder.Build();

+app.MapDefaultEndpoints();


It's actually interesting that we weren't calling this in the past but we were Adding Service Defaults. Do you happen to know why? Why do we need it now?

We need it to get the /alive, and /health endpoints being used by the tests. When running it manually, it wasn't useful I'm guessing.

eerhardt

This is great work, @radical. Nice job.

Let's get this in and get more tests enabled.

eerhardt · 2024-08-09T17:07:05Z

playground/Directory.Build.targets

+  <!-- Import only when in-repo. For the out-of-repo case a parent Directory.Build.targets does the import -->
+  <Import Project="$(TestsSharedRepoTestingDir)Aspire.RepoTesting.targets" Condition="'$(RepoRoot)' != ''" />
+
+  <PropertyGroup Condition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(SkipDashboardProjectReference)' == 'true'">


Suggested change

<PropertyGroup Condition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(SkipDashboardProjectReference)' == 'true'">

<PropertyGroup Condition="'$(SkipDashboardProjectReference)' == 'true'">

When are these tests running outside of the repo and SkipDashboardProjectReference is not set?

eerhardt · 2024-08-09T17:09:41Z

tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Compile Condition="'$(RepoRoot)' != ''" Include="$(RepoRoot)src\Aspire.Hosting\Utils\PasswordGenerator.cs" />


How does this build when RepoRoot isn't set? WithRandomParameterValues needs this code in order to build.

We copy the source to the test directory in the archive for helix, so it gets picked up by msbuild as part of collecting the all .cs files.

eerhardt · 2024-08-09T17:18:49Z

tests/Aspire.Playground.Tests/AppHostTests.cs

+
+    public AppHostTests(ITestOutputHelper testOutput)
+    {
+        this._testOutput = new TestOutputWrapper(testOutput);


Why do we need this TestOutputWrapper? Why can't we use the ITestOutputHelper directly?

This is useful to see the output streaming to testOutputHelper when the test run might be taking long. But in the case of these tests most of the output is being logged to the console anyway, so we can drop this.

eerhardt · 2024-08-09T17:19:00Z

tests/Aspire.Playground.Tests/AppHostTests.cs

+
+    public AppHostTests(ITestOutputHelper testOutput)
+    {
+        this._testOutput = new TestOutputWrapper(testOutput);


(nit)

Suggested change

this._testOutput = new TestOutputWrapper(testOutput);

_testOutput = new TestOutputWrapper(testOutput);

radical · 2024-08-09T18:26:54Z

Merging this, and will open follow up PRs for the suggestions.

.. from @ eerhardt . Follow up for #5208 .

radical added 8 commits August 6, 2024 21:33

Rename TestShop/ServiceDefaults to TestShop/TestShop.ServiceDefaults

3c4f835

.. to disambiguate the project binaries in `artifacts`

Remove unused property

6173a1d

[playground] Conditionally AddProject for dashboard reference

aaa21cb

Playground apps add a reference to the Dashboard project for local debugging. Skip that when building for tests.

[playground] Use MapDefaultEndpoints to add /alive and /health endpoi…

4b55c56

…nts needed for testing

[tests] Add Aspire.Playground.Tests

88bfd32

.. with test infrastructure (`tests/Aspire.Playground.Tests/Infrastructure/`) taken from aspire-samples repo.

[ci] Add helix support for running playground tests

454ee21

.. under a new category - `buildforhelixtests`. This is for any tests that expect to build the tests project itself on helix, and then run it there.

[ci] helix: Clean up how playwright tests are disabled

e76e8ba

Always emit the test nuget package versions for helix, in the generated

3c041b4

.. props file. This allows project builds where test packages are added by default to work by providing the package versions for those packages.

dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Aug 7, 2024

radical added testing ☑️ area-engineering-systems infrastructure helix infra engineering repo stuff and removed area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication labels Aug 7, 2024

radical marked this pull request as ready for review August 7, 2024 02:20

radical requested a review from eerhardt as a code owner August 7, 2024 02:20

radical requested review from sebastienros, joperezr, mitchdenny and DamianEdwards August 7, 2024 02:20

radical mentioned this pull request Aug 7, 2024

Tests for missing playground apps #5210

Open

35 tasks

radical added 4 commits August 7, 2024 17:41

fix handling the case of filter not matching anything

76770ec

add README.md

2422147

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

22f1e12

fix README.md

575b0f8

radical commented Aug 7, 2024

View reviewed changes

tests/Aspire.Playground.Tests/AppHostTests.cs Outdated Show resolved Hide resolved

Update tests/Aspire.Playground.Tests/AppHostTests.cs

405ee16

mitchdenny mentioned this pull request Aug 7, 2024

Add tests that validate that playground app manifests are not broken #4297

Open

eerhardt reviewed Aug 8, 2024

View reviewed changes

playground/TestShop/TestShop.sln Show resolved Hide resolved

eerhardt reviewed Aug 8, 2024

View reviewed changes

playground/Directory.Build.targets Outdated Show resolved Hide resolved

eerhardt reviewed Aug 8, 2024

View reviewed changes

tests/Aspire.Playground.Tests/Infrastructure/PasswordGenerator.cs Outdated Show resolved Hide resolved

radical added 5 commits August 8, 2024 17:26

Address review feedback from @ eerhardt

c542e0f

Address review feedback from @ eerhardt

9f80be4

Address review feedback from @ eerhardt - clean up Aspire.Playground.…

0badf7f

…Tests.csproj

Address review feedback from @ eerhardt - Use PasswordGenerator from …

5dc2303

…Aspire.Hosting instead of duplicating from aspire-samples

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

6481a08

joperezr reviewed Aug 8, 2024

View reviewed changes

playground/AzureStorageEndToEnd/AzureStorageEndToEnd.AppHost/Program.cs Show resolved Hide resolved

joperezr reviewed Aug 8, 2024

View reviewed changes

radical added 5 commits August 8, 2024 19:28

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

ee470ff

fix playground build

b7dc633

address review feedback from @ joperezr

97c3715

fix playground build

ef9ce73

address review feedback - revert playwright specific changes

3c7796c

eerhardt approved these changes Aug 9, 2024

View reviewed changes

radical merged commit a7f23ec into dotnet:main Aug 9, 2024
11 checks passed

radical deleted the playground-tests-helix branch August 9, 2024 18:31

radical mentioned this pull request Aug 9, 2024

[tests] Addressing review feedback for Aspire.Playground.Tests #5243

Merged

radical added a commit that referenced this pull request Aug 9, 2024

[tests] Addressing review feedback for Aspire.Playground.Tests (#5243)

3aa35bc

.. from @ eerhardt . Follow up for #5208 .

github-actions bot locked and limited conversation to collaborators Sep 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tests] Add `Aspire.Playground.Tests` #5208

[tests] Add `Aspire.Playground.Tests` #5208

radical commented Aug 7, 2024 •

edited

Loading

mitchdenny commented Aug 7, 2024

radical commented Aug 7, 2024

eerhardt Aug 8, 2024

radical Aug 8, 2024

eerhardt Aug 8, 2024

radical Aug 8, 2024

eerhardt Aug 8, 2024

radical Aug 8, 2024

radical Aug 8, 2024

eerhardt Aug 8, 2024

radical Aug 8, 2024

joperezr Aug 8, 2024

radical Aug 8, 2024

eerhardt left a comment

eerhardt Aug 9, 2024

eerhardt Aug 9, 2024

radical Aug 9, 2024

eerhardt Aug 9, 2024

radical Aug 9, 2024

eerhardt Aug 9, 2024

radical commented Aug 9, 2024

		@@ -12,6 +12,7 @@

		var app = builder.Build();

		app.MapDefaultEndpoints();

	<PropertyGroup Condition="'$(TestsRunningOutsideOfRepo)' == 'true' or '$(SkipDashboardProjectReference)' == 'true'">
	<PropertyGroup Condition="'$(SkipDashboardProjectReference)' == 'true'">

	this._testOutput = new TestOutputWrapper(testOutput);
	_testOutput = new TestOutputWrapper(testOutput);

[tests] Add Aspire.Playground.Tests #5208

[tests] Add Aspire.Playground.Tests #5208

Conversation

radical commented Aug 7, 2024 • edited Loading

Microsoft Reviewers: Open in CodeFlow

mitchdenny commented Aug 7, 2024

radical commented Aug 7, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a 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

radical commented Aug 9, 2024

[tests] Add `Aspire.Playground.Tests` #5208

[tests] Add `Aspire.Playground.Tests` #5208

radical commented Aug 7, 2024 •

edited

Loading