9.1 Test Templates don't use new 9.1 Test Features

[afscrome]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

A lot of new testing goodness has been added in Aspire 9.1, but the test templates have hardly changed so don't expose much of this.

(This issue assumes that https://github.com/dotnet/aspire/tree/main/src/Aspire.ProjectTemplates/templates/aspire-starter/9.1/Aspire-StarterApplication.1.Tests is the intended template due to #7603)

Describe the solution you'd like

1. app.Services.GetRequiredService<ResourceNotificationService>() should be replaced with app.ResourceNotifications

2. In order to get details about health check failures and resources not ending up in the expected state, we should consider adding the following to app host configuration in the default test. Users can opt out of these if they don't need them, but by having them in the default template, it's a strong indicator to users that these logs may be useful.

appHost.Services.AddLogging(l => l
    .AddFilter("Aspire.Hosting.ApplicationModel.ResourceNotificationService", LogLevel.Debug)
    .AddFilter("Aspire.Hosting.Health.ResourceHealthCheckService", LogLevel.Debug));

3. Cancellation/Timeouts should be added to the app.StartAsync() call, as this could hang indefinitely in certain chains of WaitFor dependencies, so that users can see the health check & resource notification logs to see why their test hung. (For completeness, cancellation also ought to be added to the appHost.BuildAsync and httpClientGetAsync())

4. Should WaitForResourceAsync be replaced with WaitForResourceHealthyAsync

Additional context

No response

[danmoseley]

@ReubenBond

[davidfowl]

@DamianEdwards

[afscrome]

Cancellation/Timeouts should be added to the app.StartAsync() call, as this could hang indefinitely in certain chains of WaitFor dependencies, so that users can see the health check & resource notification logs to see why their test hung. (For completeness, cancellation also ought to be added to the appHost.BuildAsync and httpClientGetAsync())

Ideally for this, we would plug in to the test framework's cancellation mechanisms:

MSTest:

    [TestClass]
    public class IntegrationTest1(TestContext context)
    {
        [TestMethod]
        [Timeout(30_000)]
        public async Task GetWebResourceRootReturnsOkStatusCode()
        {
            var cancellationToken = context.CancellationTokenSource.Token;

NUnit

        [Test]
        [CancelAfter(30_000)]
        public async Task GetWebResourceRootReturnsOkStatusCode(CancellationToken cancellationToken)
        {

XUnit (Does require upgrading from xunit to xunit.v3 package - see https://xunit.net/docs/getting-started/v3/whats-new#test-context)

        [Fact(Timeout=30_000)]
        public async Task GetWebResourceRootReturnsOkStatusCode()
        {
            var cancellationToken = TestContext.Current.CancellationToken;

(If upgrading to xunit.v3, also consider setting [assembly: CaptureConsole] in place of the comment around ITestOutputHelper https://xunit.net/docs/getting-started/v3/whats-new#capturing-console-debug-and-trace-output )

[DamianEdwards]

Thanks for this @afscrome!

1. app.Services.GetRequiredService<ResourceNotificationService>() should be replaced with app.ResourceNotifications

Agreed.

2. In order to get details about health check failures and resources not ending up in the expected state, we should consider adding the following to app host configuration in the default test. Users can opt out of these if they don't need them, but by having them in the default template, it's a strong indicator to users that these logs may be useful.

appHost.Services.AddLogging(l => l
    .AddFilter("Aspire.Hosting.ApplicationModel.ResourceNotificationService", LogLevel.Debug)
    .AddFilter("Aspire.Hosting.Health.ResourceHealthCheckService", LogLevel.Debug));

I agree this would be helpful as it sets up the test for much easier failure analysis. It might be easier to just enable all "Aspire" logs though (that's what we do in the samples repo tests). Is there concern that would be too noisy? We should likely add logging.SetMinimumLevel(LogLevel.Debug); too.

3. Cancellation/Timeouts should be added to the app.StartAsync() call, as this could hang indefinitely in certain chains of WaitFor dependencies, so that users can see the health check & resource notification logs to see why their test hung. (For completeness, cancellation also ought to be added to the appHost.BuildAsync and httpClientGetAsync())

Agreed, but for the httpClient.GetAsync("/") call I don't think an extra wait should be configured here as the test already adds the resiliency handler which will timeout.

4. Should WaitForResourceAsync be replaced with WaitForResourceHealthyAsync

Yes.

I'll put a PR up with these changes and we can continue any further conversation there.

[DamianEdwards]

Oh and RE updating to xUnit.net v3, we're tracking that separately in #7225