Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 20, 2025

Description

The DistributedApplicationPipelineTests class was not using the test logging infrastructure, making it difficult to diagnose test failures. Tests were using NullLogger.Instance instead of capturing output through xUnit's ITestOutputHelper.

This PR enables comprehensive test logging for all 51 pipeline tests by:

  • Adding ITestOutputHelper to the test class constructor
  • Configuring all tests to use WithTestAndResourceLogging(testOutputHelper)
  • Replacing NullLogger.Instance with a real logger from the DI container
  • Creating a new TestPipelineActivityReporter that writes pipeline activity (steps, tasks, log messages) directly to ITestOutputHelper
  • Adding timeout protection to prevent tests from hanging

Changes Made

1. Test Class Constructor

Added ITestOutputHelper testOutputHelper parameter to enable test output capture.

2. Builder Initialization (51 tests)

Updated all TestDistributedApplicationBuilder.Create() calls to chain .WithTestAndResourceLogging(testOutputHelper):

TestDistributedApplicationBuilder.Create(DistributedApplicationOperation.Publish, step: null)
    .WithTestAndResourceLogging(testOutputHelper)

3. CreateDeployingContext Method

Replaced NullLogger.Instance with a real logger from DI:

app.Services.GetRequiredService<ILogger<DistributedApplicationPipelineTests>>()

4. TestPipelineActivityReporter (New)

Created a reusable test implementation of IPipelineActivityReporter in tests/Shared/ that:

  • Injects ITestOutputHelper directly via constructor
  • Writes formatted output to test output for all pipeline activity:
    • CompletePublishAsync: [CompletePublish] message (State: state)
    • CreateStepAsync: [CreateStep] title
    • CompleteAsync (step): [CompleteStep:title] text (State: state) (indented)
    • CreateTaskAsync: [CreateTask:step] status (indented)
    • Log: [LogLevel:step] message (indented)
    • CompleteAsync (task): [CompleteTask:task] message (State: state) (indented)
    • UpdateAsync: [UpdateTask:task] status (indented)
  • Collects data in lists for test assertions
  • Uses hierarchical indentation to show step/task/log structure clearly

5. Test Registration (51 tests)

All tests now register both the test output helper and the activity reporter:

builder.Services.AddSingleton(testOutputHelper);
builder.Services.AddSingleton<IPipelineActivityReporter, TestPipelineActivityReporter>();

6. Timeout Protection (50 tests)

Added .DefaultTimeout() to all pipeline.ExecuteAsync() calls to prevent tests from hanging (5-second timeout locally, 30 seconds on CI):

await pipeline.ExecuteAsync(context).DefaultTimeout();

Bug Fixes During Development

During implementation, an infinite recursion bug was discovered and fixed. An initial attempt to use ILogger in the reporter caused the reporter's own log calls to be captured by itself recursively. The solution was to have the reporter write directly to ITestOutputHelper.WriteLine() instead, avoiding the logging infrastructure entirely and preventing any possibility of recursion.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
    • No
Original prompt

Use WithTestAndResourceLogging and in DistributedApplicationPipelineTests and make sure we create and ass real logger into CreateDeployingContext


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…ionPipelineTests

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copilot AI changed the title [WIP] Add real logger to CreateDeployingContext Enable test logging in DistributedApplicationPipelineTests Nov 20, 2025
Copilot AI requested a review from davidfowl November 20, 2025 15:59
Copilot finished work on behalf of davidfowl November 20, 2025 15:59
@davidfowl
Copy link
Member

@copilot Keep the original TestDistributedApplicationBuilder.Create(DistributedApplicationOperation.Publish, step: null), but use the extension method WithTestAndResourceLogging as well

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 20, 2025

@copilot Keep the original TestDistributedApplicationBuilder.Create(DistributedApplicationOperation.Publish, step: null), but use the extension method WithTestAndResourceLogging as well

Updated to keep the original Create(DistributedApplicationOperation.Publish, step: null) pattern and chain .WithTestAndResourceLogging(testOutputHelper) as an extension method. Commit: e6ed7c5

Copilot finished work on behalf of davidfowl November 20, 2025 16:55
@github-actions
Copy link
Contributor

github-actions bot commented Nov 20, 2025

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13072

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13072"

@davidfowl davidfowl marked this pull request as ready for review November 20, 2025 19:07
Copilot AI review requested due to automatic review settings November 20, 2025 19:07
Copilot finished reviewing on behalf of davidfowl November 20, 2025 19:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enables comprehensive test logging infrastructure for the DistributedApplicationPipelineTests class, making test failures easier to diagnose by capturing detailed execution logs through xUnit's test output. The changes systematically update all test methods to use proper logging instead of silent null loggers.

Key Changes

  • Added ITestOutputHelper parameter to test class constructor for capturing xUnit output
  • Updated all 51 TestDistributedApplicationBuilder.Create() calls to chain .WithTestAndResourceLogging(testOutputHelper)
  • Modified CreateDeployingContext() to retrieve logger from DI container instead of using NullLogger.Instance

@davidfowl
Copy link
Member

@copilot we also want to add an IPipelineActivityReporter implementation in tests that log to the ITestOutputHelper or ILogger (to capture the steps and logs).

…r/ILogger

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 20, 2025

@copilot we also want to add an IPipelineActivityReporter implementation in tests that log to the ITestOutputHelper or ILogger (to capture the steps and logs).

Added TestPipelineActivityReporter implementation that logs all pipeline activity (steps, tasks, and log messages) to ITestOutputHelper or ILogger. Updated 6 tests in DistributedApplicationPipelineTests to use this reporter instead of the production PipelineActivityReporter. Commit: 3e383f0

Copilot finished work on behalf of davidfowl November 20, 2025 22:51
@davidfowl davidfowl enabled auto-merge (squash) November 21, 2025 04:07
private readonly ITestOutputHelper? _testOutputHelper;
private readonly ILogger? _logger;

public TestReportingStep(TestPipelineActivityReporter reporter, string title, ITestOutputHelper? testOutputHelper, ILogger? logger)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just write to ILogger. And tests configure the logger to write to test output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated TestPipelineActivityReporter to only write to ILogger. Tests now configure the logger to write to test output via WithTestAndResourceLogging(testOutputHelper). Commit: 570ebae

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused a bug 😄

private readonly ITestOutputHelper? _testOutputHelper;
private readonly ILogger? _logger;

public TestReportingTask(TestPipelineActivityReporter reporter, string initialStatusText, ITestOutputHelper? testOutputHelper, ILogger? logger)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous. Just write to logger

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to only use ILogger throughout. All logging now goes through the structured logging API. Commit: 570ebae

@davidfowl
Copy link
Member

@copilot implement the feedback

@davidfowl
Copy link
Member

@copilot Make TestPipelineActivityReporter inject ILogger<TestPipelineActivityReporter> so that tests can do:

builder.Services.AddSingleton<IPipelineActivityReporter, TestPipelineActivityReporter>();

…ogger

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 21, 2025

@copilot Make TestPipelineActivityReporter inject ILogger<TestPipelineActivityReporter> so that tests can do:

builder.Services.AddSingleton<IPipelineActivityReporter, TestPipelineActivityReporter>(...

Changed constructor to accept ILogger<TestPipelineActivityReporter> instead of ILogger. Tests now use simple registration: builder.Services.AddSingleton<IPipelineActivityReporter, TestPipelineActivityReporter>(). DI automatically resolves the generic logger. Commit: 3f38ba9

Copilot finished work on behalf of davidfowl November 21, 2025 05:36
@davidfowl
Copy link
Member

@copilot Add the TestPipelineActivityReporter to all DistributedApplicationPipelineTests

…eTests

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 21, 2025

@copilot Add the TestPipelineActivityReporter to all DistributedApplicationPipelineTests

Added TestPipelineActivityReporter to all 51 tests in DistributedApplicationPipelineTests that execute pipelines. Each test now includes builder.Services.AddSingleton<IPipelineActivityReporter, TestPipelineActivityReporter>(); for comprehensive pipeline activity logging. Commit: 71c8a61

Copilot finished work on behalf of davidfowl November 21, 2025 05:52
@davidfowl
Copy link
Member

@copilot the DistributedApplicationPipelineTests seem to be hanging now. Can you figure out which ones are hanging? Make sure we have timeouts on the calls to ExecuteAsync

…nging tests

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 21, 2025

@copilot the DistributedApplicationPipelineTests seem to be hanging now. Can you figure out which ones are hanging? Make sure we have timeouts on the calls to ExecuteAsync

Added .DefaultTimeout() to all 50 pipeline.ExecuteAsync() calls in DistributedApplicationPipelineTests. This provides a 5-second timeout (30 seconds on CI) to prevent tests from hanging. Also added using Microsoft.AspNetCore.InternalTesting; to access the DefaultTimeout extension method. Commit: 90de509

Copilot finished work on behalf of davidfowl November 21, 2025 17:16
@davidfowl
Copy link
Member

@copilot It's still hanging, run each test in tests/Aspire.Hosting.Tests/Pipelines/DistributedApplicationPipelineTests.cs, individually to figure out where the hang might be.

Today the CI is running this:

/dotnet.sh test /home/runner/work/aspire/aspire/tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj /p:ContinuousIntegrationBuild=true /p:TrxFileNamePrefix="Hosting" -bl:/home/runner/work/aspire/aspire/testresults/test.binlog --no-restore --no-build -- --ignore-exit-code 8 --report-trx --hangdump --hangdump-timeout 7m --crashdump --results-directory /home/runner/work/aspire/aspire/testresults --timeout 25m --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true" 

And it is hanging for 60 minutes on this PR still.

…gger calls

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot It's still hanging, run each test in tests/Aspire.Hosting.Tests/Pipelines/DistributedApplicationPipelineTests.cs, individually to figure out where the hang might be.

Today the CI is running...

Found the issue! The TestPipelineActivityReporter was creating an infinite recursion loop. The reporter's Log() method was calling _logger.Log() which got captured by the reporter again, creating the nested pattern you saw in the CI output.

Fixed by removing all _logger.Log() calls from the reporter. The reporter now only collects data for test assertions - the actual pipeline logging comes from the real logger in CreateDeployingContext. Commit: 813e48c

Copilot finished work on behalf of davidfowl November 22, 2025 08:08
@davidfowl
Copy link
Member

@copilot Sicne the logger is no longer used, we should instead directly inject and use the ITestOutputHelper directly in TestPipelineActivityReporter. Add it to the DI container and inject into the TestPipelineActivityReporter ctor

…of ILogger

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot Sicne the logger is no longer used, we should instead directly inject and use the ITestOutputHelper directly in TestPipelineActivityReporter. Add it to the DI container and inject into the Te...

Updated TestPipelineActivityReporter to inject and use ITestOutputHelper directly instead of ILogger<TestPipelineActivityReporter>. Also added builder.Services.AddSingleton(testOutputHelper); to all 51 tests so the test output helper is available in the DI container for injection. Commit: b099bd3

Copilot finished work on behalf of davidfowl November 22, 2025 08:32
@davidfowl
Copy link
Member

@copilot We should be writing to the _testOutputHelper in the TestPipelineActivityReporter, in each method

…output

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot We should be writing to the _testOutputHelper in the TestPipelineActivityReporter, in each method

Added _testOutputHelper.WriteLine() calls to all methods in TestPipelineActivityReporter:

  • CompletePublishAsync: Logs completion with state
  • CreateStepAsync: Logs step creation
  • CompleteAsync (step): Logs step completion with 2-space indent
  • CreateTaskAsync: Logs task creation with 4-space indent
  • Log: Logs messages with log level and 4-space indent
  • CompleteAsync (task): Logs task completion with 6-space indent
  • UpdateAsync: Logs task updates with 6-space indent

The hierarchical indentation makes it easy to see the structure of pipeline execution in test output. Commit: d97f1c3

Copilot finished work on behalf of davidfowl November 22, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants