Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose ResourceNotificationService directly on DistributedApplicationTestingBuilder #6795

Closed
1 task done
afscrome opened this issue Nov 25, 2024 · 2 comments · Fixed by #7517
Closed
1 task done
Assignees
Labels
area-app-testing Issues pertaining to the APIs in Aspire.Hosting.Testing
Milestone

Comments

@afscrome
Copy link
Contributor

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.

I've found that I'm using ResourceNotificationService a lot in tests using DistributedApplicationTestingBuilder, and wonder if it's something worth exposing directly, rather than being hidden behind a GetRequiredService call.

Or perhaps rather than exposing ResourceNotificationService, expose something that's little more suited for the automated tests. E.g. a version of WaitForResourceAsync that fails fast if a resource goes to a terminal state, as opposed to the version in ResourceNotificationService which continues to wait as it's possible as it's possible the service starts again at a later date.

Describe the solution you'd like

        var appHost = await DistributedApplicationTestingBuilder.CreateAsync<Projects.AspireApp2_AppHost>();
        await using var app = await appHost.BuildAsync();
        await app.StartAsync();

        await appHost.ResourceNotificationService.WaitForResourceAsync("whatever");
        // Or possibly something more like:
        await appHost.WaitFor*("whatever");

Additional context

No response

@joperezr joperezr added the untriaged New issue has not been triaged label Nov 27, 2024
@joperezr joperezr added the area-app-testing Issues pertaining to the APIs in Aspire.Hosting.Testing label Jan 7, 2025
@davidfowl davidfowl added this to the 9.1 milestone Jan 10, 2025
@davidfowl davidfowl removed the untriaged New issue has not been triaged label Jan 10, 2025
@davidfowl davidfowl assigned davidfowl and ReubenBond and unassigned davidfowl Jan 10, 2025
@afscrome
Copy link
Contributor Author

I've been experimenting with some wrapper extension methods on DistributedApplication instead of using ResourceNotificationService. In particular I've been using this to handle timeouts and give more specific errors about why the resource failed to go healthy. Extension methods like these could be an alternative to exposing ResourceNotificationService directly on DistributedApplicationTestingBuilder.

For example, the following version of WaitForResourceHealthyAsync will give an error like the following when a resource fails to go healthy

Resource {resourceName} did not become healthy before {reason}
Resource State: {resourceEvent.Snapshot.State?.Text}
Health Status: {resourceEvent.Snapshot.HealthStatus}
-  {report.Name}: {report.Status}
-  {report.Name}: {report.Status} {report.ExceptionText}
- ... For each health report ...
   public static async Task WaitForResourceHealthyAsync(this DistributedApplication app, string resourceName, CancellationToken cancellationToken = default, TimeSpan timeout = default)
   {
      if (timeout == default)
      {
         timeout = TimeSpan.FromSeconds(30);
      }

      var resourceNotificationService = app.Services.GetRequiredService<ResourceNotificationService>();

      ResourceEvent? resourceEvent = null;
      try
      {
         await resourceNotificationService
            .WaitForResourceAsync(resourceName, evt =>
            {
               resourceEvent = evt;

               if (KnownResourceStates.TerminalStates.Contains(evt.Snapshot.State?.Text))
               {
                  throw new DistributedApplicationException(FailureMessage(evt, "terminating"));
               }

               return evt.Snapshot.HealthStatus == HealthStatus.Healthy;
            }, cancellationToken)
            .WaitAsync(timeout, cancellationToken);
      }
      catch (OperationCanceledException ex)
      {
         throw new OperationCanceledException(FailureMessage(resourceEvent, "being cancelled"), ex.CancellationToken);
      }
      catch (TimeoutException)
      {
         throw new TimeoutException(FailureMessage(resourceEvent, "timing out"));
      }

      string FailureMessage(ResourceEvent? resourceEvent, string reason)
      {
         var error = new StringBuilder();
         error.AppendLine($"Resource {resourceName} did not become healthy before {reason}.");
         if (resourceEvent == null)
         {
            error.AppendLine("Resource failed to start.");
         }
         else
         {
            error.AppendLine($"Resource State: {resourceEvent.Snapshot.State?.Text}");
            error.AppendLine($"Health Status: {resourceEvent.Snapshot.HealthStatus}");
            error.AppendLine($"Health Reports:");
            foreach (var report in resourceEvent.Snapshot.HealthReports)
            {
               error.AppendLine($"-  {report.Name}: {report.Status}");
               if (report.ExceptionText is { })
               {
                  error.AppendLine(report.ExceptionText);
               }
            }
         }

         return error.ToString();
      }
   }

I did experiment if we could upstream this into the main ResourceNotificationService, but doing so requires changing signatures as you need to do the WaitAsync within the method, otherwise you can't catch the cancellation and wrap it with a nicer error message.

@ReubenBond
Copy link
Member

ReubenBond commented Feb 10, 2025

Exposing ResourceNotificationService on the builder itself doesn't feel right but exposing it on the DistributedApplication may be useful. It's only replacing a terse expression (i.e, app.Services.GetRequiredService<ResourceNotificationService>()) but it would make things a little bit more discoverable.

I agree that we want different behavior between dev & test scenarios. For that, we have WaitForBehavior now: #7397.

AFAICT, the remaining work to do here is to consider adding a ResourceNotifications property to DistributedApplication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-app-testing Issues pertaining to the APIs in Aspire.Hosting.Testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants