feat: use results directory provided by Microsoft Testing Platform in HtmlReporter

[DavidZidar]

Description

Store HTML reports in the results directory provided by Microsoft Testing Platform instead of using the hard coded path TestResults.

Related Issue

Fixes #5293

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Performance improvement
• Refactoring (no functional changes)

Checklist

Required

• I have read the Contributing Guidelines
• If this is a new feature, I started a discussion first and received agreement
• My code follows the project's code style (modern C# syntax, proper naming conventions)
• I have written tests that prove my fix is effective or my feature works (no, but I have tested the change manually)

TUnit-Specific Requirements

• Dual-Mode Implementation: If this change affects test discovery/execution, I have implemented it in BOTH:
  • Source Generator path (TUnit.Core.SourceGenerator)
  • Reflection path (TUnit.Engine)
• Snapshot Tests: If I changed source generator output or public APIs:
  • I ran TUnit.Core.SourceGenerator.Tests and/or TUnit.PublicAPI tests
  • I reviewed the .received.txt files and accepted them as .verified.txt
  • I committed the updated .verified.txt files
• Performance: If this change affects hot paths (test discovery, execution, assertions):
  • I minimized allocations and avoided LINQ in hot paths
  • I cached reflection results where appropriate
• AOT Compatibility: If this change uses reflection:
  • I added appropriate [DynamicallyAccessedMembers] annotations
  • I verified the change works with dotnet publish -p:PublishAot=true

Testing

• All existing tests pass (dotnet test) - The tests do not run cleanly in main branch so I can't verify
• I have added tests that cover my changes
• I have tested both source-generated and reflection modes (if applicable)

Additional Notes

This is just a proposed solution. I have tested this manually by referencing the projects TUnit.Core, TUnit.Engine in one of my projects and did tests with and without the --results-directory CLI parameter. With this fix the HTML report is saved in the proper location.

I have been unable to run the tests as there are too many failures in the main branch.

[claude]

Code Review

Thanks for the contribution! This is a good fix for an important issue — the hardcoded TestResults path is indeed a problem when --results-directory is passed to the test runner. Here is a detailed review covering the approach, correctness, and a few concerns.

---

What the PR does

The PR introduces SetResultsDirectory(string path) on HtmlReporter, calls it from the service factory in TestApplicationBuilderExtensions via IConfiguration.GetTestResultDirectory(), and moves the _outputPath initialization from BeforeRunAsync to OnTestSessionFinishingAsync. This ensures the HTML report lands in the same directory that MTP has already created for other output files.

---

Correctness of the API usage

IConfiguration.GetTestResultDirectory() is a valid MTP extension method (Microsoft.Testing.Platform.Configurations.ConfigurationExtensions). However, its implementation is:

var result = configuration["platformOptions:resultDirectory"];
return Ensure.NotNull(result, "resultDirectory");

Ensure.NotNull throws if the value is null. This is fine in practice because MTP always populates platformOptions:resultDirectory in AggregatedConfiguration via CheckTestResultsDirectoryOverrideAndCreateItAsync before service factories run. However, the PR's SetResultsDirectory guard:

if (string.IsNullOrWhiteSpace(path))
{
    throw new ArgumentException("Results directory cannot be null or empty", nameof(path));
}

...is redundant: GetTestResultDirectory has already thrown before this is reached. It is not harmful, but it creates a false impression that empty paths need to be guarded against here. The real guard is in MTP itself.

---

The BeforeRunAsync → OnTestSessionFinishingAsync move

The PR moves the _outputPath initialisation out of BeforeRunAsync and into OnTestSessionFinishingAsync. Looking at the current (main branch) code, BeforeRunAsync already does this guard:

public Task BeforeRunAsync(CancellationToken cancellationToken)
{
    if (string.IsNullOrEmpty(_outputPath))
    {
        _outputPath = GetDefaultOutputPath();
    }
    // ...
}

The PR removes this and puts the same check at the top of OnTestSessionFinishingAsync. This is functionally equivalent for the default path, but it changes the observable semantics: previously a consumer could call SetOutputPath(...) any time before BeforeRunAsync ran; now there is a broader window (until OnTestSessionFinishingAsync). That is not a problem in the current codebase, but it is a slight behavioural drift worth noting.

The real issue is that _resultsDirectory needs to be set before GetDefaultOutputPath() is called — and that's guaranteed now only because the service factory runs before any session events fire, which is the same guarantee that already exists for SetMessageBus. This is fine, but worth a comment explaining the ordering invariant (similar to the existing comment for SetMessageBus).

---

The JUnit reporter has the same bug

JUnitReporter.GetDefaultOutputPath() still has the hardcoded "TestResults":

return Path.GetFullPath(Path.Combine("TestResults", $"{sanitizedName}-junit.xml"));

For consistency, the same fix should be applied there. Leaving it out creates a confusing split where the HTML report respects --results-directory but JUnit does not.

---

Architectural concern: two-step initialisation

The current design requires the caller to know it must call both SetMessageBus() and now also SetResultsDirectory(), or the reporter silently falls back to a hardcoded path. This is a form of temporal coupling: the object is not usable until multiple setters have been called.

A cleaner approach would be to inject IConfiguration (or just the resolved path string) via the constructor, since HtmlReporter is instantiated inside a factory lambda that already has access to serviceProvider:

testApplicationBuilder.TestHost.AddTestSessionLifetimeHandler(serviceProvider =>
{
    var commandLineOptions = serviceProvider.GetRequiredService<ICommandLineOptions>();
    var resultsDirectory = serviceProvider.GetRequiredService<IConfiguration>().GetTestResultDirectory();

    var htmlReporter = new HtmlReporter(extension, resultsDirectory);
    // ...
});

This would eliminate SetResultsDirectory entirely and make the dependency explicit in the constructor signature. It would also make HtmlReporter easier to unit-test in isolation.

That said, SetMessageBus has the same pattern and it predates this PR, so the current two-step approach is at least consistent with the existing code. If the constructor refactor is out of scope for this PR, it is worth filing a follow-up.

---

Minor nib: field initialiser vs setter

private string _resultsDirectory = "TestResults";

This fallback default is now vestigial: SetResultsDirectory is always called from the factory, and GetTestResultDirectory always returns a non-null value. If the setter is removed (e.g., via the constructor refactor above) this dead default disappears naturally. As-is, it gives a false sense of safety — if SetResultsDirectory is somehow not called, the reporter silently uses "TestResults" with no warning.

---

Summary

	Assessment
Bug being fixed	Correct and important
API used (GetTestResultDirectory)	Valid and correct
BeforeRunAsync → OnTestSessionFinishingAsync move	Safe, but deserves a comment
SetResultsDirectory guard	Redundant but harmless
JUnit reporter not updated	Gap — same fix needed for consistency
Two-step initialisation	Acceptable given existing patterns, but worth a follow-up issue for constructor injection

The fix is directionally correct and addresses a real user pain point. The main things I would ask for before merging:

1. Apply the same fix to JUnitReporter.GetDefaultOutputPath() so both reporters behave consistently with --results-directory.
2. Add a comment to the SetResultsDirectory call site (same pattern as the existing SetMessageBus comment) explaining that it must be called before OnTestSessionFinishingAsync fires.
3. (Optional but recommended) File a follow-up issue to refactor towards constructor injection so the temporal coupling is eliminated.

[DavidZidar]

I have made the adjustments proposed by Claude with some caveats:

• I'm keeping the default value on _resultsDirectory even if it's vestigial to avoid having to make the field nullable
• I made the same style change to JUnitReporter as requested but I haven't tested this because I'm not sure how to use the JUnit reporter, is this documented? I can't find it.
• Constructor injection requires a bigger refactor than I want like to attempt at this time.

[claude]

Follow-up Review

Thanks for addressing the points from the previous review — comments are in, has the field and method, and the output-path initialisation has been deferred. However there is a copy-paste bug in TestApplicationBuilderExtensions.cs that means the JUnit fix is completely non-functional.

---

Bug: wrong reporter called inside the JUnit factory

In the JUnit AddDataConsumer factory, the code sets the results directory on htmlReporter instead of junitReporter:

// ❌ Current – sets htmlReporter inside the JUnit factory
testApplicationBuilder.TestHost.AddDataConsumer(serviceProvider =>
{
    ...
    htmlReporter.SetResultsDirectory(serviceProvider.GetRequiredService<IConfiguration>().GetTestResultDirectory());
    return junitReporter;  // but returns junitReporter!
});

This means:

1. junitReporter.SetResultsDirectory is never called — _resultsDirectory stays at the fallback "TestResults" despite having the method.
2. htmlReporter.SetResultsDirectory is called twice — once here and again in the AddTestSessionLifetimeHandler factory — which is harmless but wasteful and confusing.

The fix is a one-word change:

// ✅ Correct
junitReporter.SetResultsDirectory(serviceProvider.GetRequiredService<IConfiguration>().GetTestResultDirectory());

---

Everything else looks good

• The HtmlReporter changes are correct: SetResultsDirectory is wired up in the right factory and GetDefaultOutputPath now uses _resultsDirectory.
• The ordering invariant comments on both SetResultsDirectory methods are clear.
• Deferring _outputPath initialisation to OnTestSessionFinishingAsync / AfterRunAsync is the right call.
• The _resultsDirectory = "TestResults" default is a reasonable safety net even if vestigial.

Once the copy-paste bug above is fixed, this PR is in good shape.

[claude]

Follow-up Review

Thanks for addressing the points from the previous review — comments are in, JUnitReporter has the _resultsDirectory field and SetResultsDirectory method, and the output-path initialisation has been deferred. However there is a copy-paste bug in TestApplicationBuilderExtensions.cs that means the JUnit fix is completely non-functional.

---

Bug: wrong reporter called inside the JUnit factory

In the JUnit AddDataConsumer factory, the code sets the results directory on htmlReporter instead of junitReporter:

// ❌ Current – sets htmlReporter inside the JUnit factory
testApplicationBuilder.TestHost.AddDataConsumer(serviceProvider =>
{
    ...
    htmlReporter.SetResultsDirectory(serviceProvider.GetRequiredService<IConfiguration>().GetTestResultDirectory());
    return junitReporter;  // but returns junitReporter!
});

This means:

1. junitReporter.SetResultsDirectory is never called — _resultsDirectory stays at the fallback "TestResults" despite having the method.
2. htmlReporter.SetResultsDirectory is called twice — once here and again in the AddTestSessionLifetimeHandler factory — which is harmless but confusing.

The fix is a one-word change:

// ✅ Correct
junitReporter.SetResultsDirectory(serviceProvider.GetRequiredService<IConfiguration>().GetTestResultDirectory());

---

Everything else looks good

• The HtmlReporter changes are correct: SetResultsDirectory is wired up in the right factory and GetDefaultOutputPath now uses _resultsDirectory.
• The ordering invariant comments on both SetResultsDirectory methods are clear.
• Deferring _outputPath initialisation to OnTestSessionFinishingAsync / AfterRunAsync is the right call.
• The _resultsDirectory = "TestResults" default is a reasonable safety net even if vestigial.

Once the copy-paste bug above is fixed, this PR is in good shape.

[DavidZidar]

Good catch, I fixed the copy & paste bug and rebased the branch.

[thomhurst]

Thanks!