Updates to Aspire.Hosting.Testing by DamianEdwards · Pull Request #4444 · dotnet/aspire

DamianEdwards · 2024-06-11T04:02:53Z

This change enables capturing resource logs and forwarding them to ILogger during testing when using Aspire.Hosting.Testing, along with an update to ResourceNotificationService to enable waiting for a resource to transition to a given state text.

Forward resource logs to ILogger in Aspire.Hosting.Testing
Add ResourceNotificationService.WaitForResourceAsync
Added more states to KnownResourceStates
Updates test templates to use ResourceNotificationService.WaitForResourceAsync
Updates xUnit templates with a comment about packages that map ILogger to ITestOutputHelper

Some open questions:

Should we have an option/method to control whether resource logs are forwarded to ILogger or not in Aspire.Hosting.Testing?
Should we have an option to control the log level the resource logs are logged at (stderr vs. stdout, etc.)?
Should we update ResourceNotificationService to cancel WatchAsync streams when IHostApplicationLifetime.ApplicationStopping is triggered?

Fixes #4445
Fixes #2790

File -> New -> Aspire Starter App with xUnit.net test project with these changes:

File -> New -> Aspire Starter app with MSTest test project:

File -> New -> Aspire Starter app with NUnit test project:

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs

DamianEdwards · 2024-06-12T02:25:21Z

src/Aspire.Hosting.Testing/ResourceLogAggregatorBackgroundService.cs

+        {
+            foreach (var line in logEvent)
+            {
+                var logLevel = line.IsErrorMessage ? LogLevel.Error : LogLevel.Information;


Should we make the log level the resource LogLines are logged at an option?

I was on the fence, but then I thought we might want to tune things so we only get errors/warnings to help with memory consumption in the test case (for longer running tests).

That could be done already by filtering the logs to error or higher in the test setup I think. But it might be desirable to be able to change the level the non-errors are logged at? Not sure, might not be worth the complexity.

src/Aspire.Hosting.Testing/ResourceLogAggregatorBackgroundService.cs

- Add ResourceNotificationService.WaitForResource

…dashboard

…e call

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs

eerhardt · 2024-06-19T21:17:39Z

tests/Aspire.Workload.Tests/PerTestFrameworkTemplatesTests.cs


        Assert.True(res.ExitCode != 0, $"Expected the tests project build to fail");
-        Assert.Matches("System.ArgumentException.*Resource 'webfrontend' not found.", res.Output);
+        Assert.Matches("System.InvalidOperationException.*Sequence contains no matching element", res.Output);


(In the existing code) @radical - Why are we expecting the tests to fail?

For the updated code, isn't the original exception better? Seems like we are making this worse.

(In the existing code) @radical - Why are we expecting the tests to fail?

The test creates an aspire project, then a aspire-xunit project, uncomments the code in the generated test file, and runs that. And that causes the test to fail because the test is expecting a webfrontend to exist. So, this what the user would see.

We could add a webfrontend to the solution, and patch that up so it gets used in this test. Do we want to do that?

We could move the CreateHttpClient call to before the WaitForResourceAsync call to retain the original exception message and not bother with finding the resource in the DistributedApplicationModel.

I went ahead and adjusted the order of lines in the test templates so that the original exception is thrown. @radical I do think it would be better to update these tests to add a resource though so the tests actually pass. Perhaps we could just base it on the starter template instead (without the tests project option) in order to get the web project by default?

I was under the understanding we were validating the scenario @radical describes above. What is the experience when a dev adds the test project, and just runs it. Do they get an understandable error?

If you just add the test project and run it you don't get an error. The test here actually uncomments the sample code and then runs the test, which fails because the sample code assumes there's a resource to test against (which the comments say).

eerhardt

LGTM.

Might want to wait to see if @davidfowl doesn't approve of the ResourceLoggerService change.

mitchdenny · 2024-06-20T02:03:51Z

src/Aspire.Hosting.Dapr/IDistributedApplicationComponentBuilderExtensions.cs

    /// <param name="builder">The resource builder instance.</param>
    /// <param name="options">Options for configuring the Dapr sidecar, if any.</param>
    /// <returns>The resource builder instance.</returns>
-    [System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0027:API with optional parameter(s) should have the most parameters amongst its public overloads", Justification = "<Pending>")]


Seems unrelated?

Hmm not sure. Possibly I got a "suppression not required" diagnostic and acted on it? Will have to check

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs

afscrome · 2024-06-20T09:46:35Z

@DamianEdwards Wonder if the logger race is similar to #4606

aspire/src/Aspire.Hosting/ApplicationModel/ResourceLoggerService.cs

Line 106 in bae6c46

LoggerAdded += OnLoggerAdded;

Although on second thoughts, if it's the same issue I'd expect logs to be lost rather than duplicated.

mitchdenny

Looking forward to this going in. I think I've got an immediate use case for it in some tests I'm writing!

radical · 2024-06-20T20:29:27Z

You can merge main to get #4614 which disables the failing WithDockerfileTests.

DamianEdwards · 2024-06-20T21:19:07Z

@davidfowl @ReubenBond @eerhardt @mitchdenny I've reimplemented the ResourceLoggerService.WatchAsync logic based on a discussion I had with @davidfowl but it changes the behavior in a fundamental way, i.e. new subscribers won't get the backlog until after the next log is sent since they subscribed, as flushing the backlog was moved to the OnLog subscription method itself. I've updated the tests to account for this but I'd appreciate folks' thoughts on whether this is actually what we want. It definitely makes testing a bit harder (need to coordinate waiting for subs to change before publishing logs and need to publish extra logs to get the backlog) and could lead to situations where no historical logs are shown until the next log is sent, or never shown if another log is never sent (e.g. if the resource has stopped).

UPDATE: OK I updated this again so that the backlog is always returned immediately and is synchronized with the OnNewLog method so that dupes are properly handled there. This should return us to the original behavior and handle the de-duping required due to the race, hopefully in a way that everyone is happier with 😄

radical · 2024-06-20T23:59:37Z

Green!!

radical · 2024-06-21T01:16:43Z

Since #4500 has merged, this PR should merge main to check if everything still works.

mitchdenny · 2024-06-21T04:52:52Z

kicking build again because it failed because of a code coverage upload issue.

mitchdenny · 2024-06-21T04:52:58Z

/azp run

azure-pipelines · 2024-06-21T04:53:08Z

Azure Pipelines successfully started running 1 pipeline(s).

eerhardt · 2024-06-21T15:54:51Z

src/Aspire.Hosting/ApplicationModel/ResourceLoggerService.cs

+
+                flushBacklogSync.Wait(cancellationToken);
+                // We need to ensure we don't write this log to the channel if it was already in the backlog
+                if (backlogSnapshot?.Contains(log) == false)


Can backlogSnapshot ever be null here? flushBacklogSync.Set(); is called after backlogSnapshot is set. So if we are waiting for it to be set, backlogSnapshot should always be non-null.

Correct. I had this throwing when null previously but that seemed extreme. Are you suggesting this is a case we should just bang the nullability warning away?

I guess I personally would just do:

Debug.Assert(backlogSnapshot is not null, "backlogSnapshot is always set before flushBacklogSync is set"); if (!backlogSnapshot.Contains(log))

(Note that this is really just a nit on the readability of the method. Not critical to change.)

Yeah I personally like the Debug.Assert approach too but this seems like another example of where we're not super consistent in the repo.

DamianEdwards requested review from ReubenBond, davidfowl, eerhardt and mitchdenny June 11, 2024 04:02

ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 11, 2024

davidfowl reviewed Jun 11, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

DamianEdwards commented Jun 12, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

DamianEdwards commented Jun 12, 2024

View reviewed changes

src/Aspire.Hosting.Testing/ResourceLogAggregatorBackgroundService.cs Show resolved Hide resolved

DamianEdwards added 11 commits June 12, 2024 13:38

Forward resource logs to ILogger in Aspire.Hosting.Test

a54df93

- Add ResourceNotificationService.WaitForResource

Change resource log format to more closely match what's shown in the …

87cfda6

…dashboard

Update WaitForResourceAsync to call through WatchAsync

255b388

Ensure timeout CTS is disposed

ffba5c8

Ensure WaitForResourceAsync cancels when app is stopping

e604324

Add XUnit logger integration in starter app template & WaitForResourc…

e7b5791

…e call

Fix typos.

7f9e8d7

Support WaitForResourceAsync with multiple target states

25cae9f

Add tests for WaitForResourceAsync

8920d6d

Add test to cover resource name/state casing scenario

8b72f8c

More tests

75a6baf

DamianEdwards force-pushed the damianedwards/test-resource-watcher branch from 6caf78c to 75a6baf Compare June 12, 2024 20:38

Add tests for ResourceLogAggregatorBackgroundService

98febdd

davidfowl reviewed Jun 12, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jun 12, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jun 12, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jun 12, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jun 12, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

DamianEdwards added 3 commits June 12, 2024 16:49

Update test templates & PR feedback

06a7fcf

PR feedback

4dc79d3

Add test to ensure log aggregator is registered as a background service

a9e02ec

Make test templates use application model to retrieve resource first.

4de6dde

eerhardt reviewed Jun 19, 2024

View reviewed changes

Simplify test template & revert test change

61784cb

eerhardt approved these changes Jun 19, 2024

View reviewed changes

ReubenBond approved these changes Jun 19, 2024

View reviewed changes

Reimplement the log backlog & channel synchronization

fb6e3c4

mitchdenny reviewed Jun 20, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Show resolved Hide resolved

mitchdenny approved these changes Jun 20, 2024

View reviewed changes

Merge branch 'main' into damianedwards/test-resource-watcher

285470c

DamianEdwards added 2 commits June 20, 2024 14:12

Fix tests after ResourceLoggerService behavior change

111f13a

Merge branch 'main' into damianedwards/test-resource-watcher

b6b8925

DamianEdwards added 2 commits June 20, 2024 18:37

Ensure resource logs backlog is flushed immediately

a920454

Merge branch 'main' into damianedwards/test-resource-watcher

7198eb7

DamianEdwards requested a review from davidfowl June 21, 2024 01:39

build-analysis bot mentioned this pull request Jun 21, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

mitchdenny mentioned this pull request Jun 21, 2024

reenable tests to start debugging in the pipeline (they run fine local… #4618

Merged

davidfowl approved these changes Jun 21, 2024

View reviewed changes

DamianEdwards merged commit 72edbe0 into main Jun 21, 2024

DamianEdwards deleted the damianedwards/test-resource-watcher branch June 21, 2024 14:57

eerhardt reviewed Jun 21, 2024

View reviewed changes

github-actions bot locked and limited conversation to collaborators Jul 22, 2024

Conversation

DamianEdwards commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

afscrome commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitchdenny left a comment

Choose a reason for hiding this comment

Uh oh!

radical commented Jun 20, 2024

Uh oh!

DamianEdwards commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radical commented Jun 20, 2024

Uh oh!

radical commented Jun 21, 2024

Uh oh!

mitchdenny commented Jun 21, 2024

Uh oh!

mitchdenny commented Jun 21, 2024

Uh oh!

azure-pipelines bot commented Jun 21, 2024

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!

8 participants

DamianEdwards commented Jun 11, 2024 •

edited

Loading

afscrome commented Jun 20, 2024 •

edited

Loading

DamianEdwards commented Jun 20, 2024 •

edited

Loading