feat: register HTML report as MTP session artifact

[thomhurst]

Summary

• HtmlReporter now implements ITestSessionLifetimeHandler and publishes the generated HTML report as a SessionFileArtifact after every test run
• Uses OnTestSessionFinishingAsync (not AfterRunAsync) so the MTP message bus is still live when the artifact is published
• The application-level IMessageBus is injected via AddTestSessionLifetimeHandler's service provider — this bus remains valid throughout the session close sequence

Changes

• TUnit.Engine/Reporters/Html/HtmlReporter.cs — adds ITestSessionLifetimeHandler; moves report generation + artifact publishing to OnTestSessionFinishingAsync; adds SetMessageBus(IMessageBus) for injection
• TUnit.Engine/Extensions/TestApplicationBuilderExtensions.cs — registers HtmlReporter via AddTestSessionLifetimeHandler (MTP auto-registers it as IDataConsumer); injects IMessageBus from service provider
• TUnit.Engine/Framework/TUnitTestFramework.cs — minor refactor only (no functional change)
• TUnit.Engine.Tests/HtmlReporterTests.cs — new: 5 unit tests covering IDataProducer membership, DataTypesProduced, publish happy path, null-bus no-op, file-not-exists no-op
• TUnit.Engine/TUnit.Engine.csproj — adds InternalsVisibleTo for TUnit.Engine.Tests
• TUnit.Engine.Tests/TUnit.Engine.Tests.csproj — adds MTP package reference and strong-name key for InternalsVisibleTo

Test plan

• All 5 HtmlReporterTests pass (--treenode-filter "/*/*/HtmlReporterTests/*")
• HTML report file is generated after a normal test run (TestResults/*.html exists with content)
• MTP lists the HTML file as a session artifact (visible in VS Test Explorer / CI artifact output)
• No "message bus has been drained" warning in output

[claude]

Code Review

Overall, this is a solid and well-motivated change — moving artifact publishing to OnTestSessionFinishingAsync is the right call to avoid the drained message bus problem. The test coverage is appreciated. That said, I have a few concerns worth addressing.

---

Critical: Missing AddDataConsumer registration may produce empty reports

The old code explicitly called testApplicationBuilder.TestHost.AddDataConsumer(...) for htmlReporter, which is what caused MTP to route TestNodeUpdateMessage events to ConsumeAsync and populate _updates. The new code replaces this with AddTestSessionLifetimeHandler.

The PR description states: "MTP auto-registers it as IDataConsumer". But this is an undocumented MTP implementation detail. If AddTestSessionLifetimeHandler does not auto-discover IDataConsumer implementations, then ConsumeAsync is never called, _updates stays empty, and OnTestSessionFinishingAsync hits the early return:

if (_updates.Count == 0)
{
    return;  // No HTML report generated
}

This would be a silent regression — no error, no warning, just an empty/missing report. The JUnitReporter and GitHubReporter both retain explicit AddDataConsumer calls; HtmlReporter should too, unless you can point to MTP documentation that confirms the auto-registration behaviour.

Suggestion: Add testApplicationBuilder.TestHost.AddDataConsumer(_ => htmlReporter) back alongside the AddTestSessionLifetimeHandler registration, or document the MTP API contract that makes this safe to omit.

---

Misleading comment in AfterRunAsync

public Task AfterRunAsync(int exitCode, CancellationToken cancellation)
{
    // Report generation and artifact publishing happen in OnTestSessionFinishingAsync
    // (before the message bus is drained), so AfterRunAsync only handles GitHub integration.
    return Task.CompletedTask;
}

The comment says "AfterRunAsync only handles GitHub integration", but the implementation does nothing — the GitHub integration was also moved to OnTestSessionFinishingAsync. The comment will mislead future readers into thinking there's some GitHub work here. Either drop the comment or update it:

// All work has moved to OnTestSessionFinishingAsync, which runs before the message bus is drained.

---

Inaccurate test name

public async Task PublishArtifactAsync_Is_NoOp_When_SessionContext_Not_Set()

The test name says SessionContext_Not_Set, but what's actually being tested is that _messageBus is null (i.e. SetMessageBus was never called). There's no "session context" involved — SessionUid is passed as a parameter and is always present. A clearer name would be PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected.

---

Minor: csproj indentation regression

In TUnit.Engine.Tests.csproj, the newly added PackageReference for CrashDump has inconsistent indentation (4 spaces instead of 8):

+        <PackageReference Include="Microsoft.Testing.Platform" />
+    <PackageReference Include="Microsoft.Testing.Extensions.CrashDump" />   ← wrong indent

---

What's done well

• Moving report generation and artifact publishing to OnTestSessionFinishingAsync is the right fix. AfterRunAsync is too late; the bus can already be drained.
• Injecting IMessageBus via the serviceProvider in AddTestSessionLifetimeHandler is idiomatic and matches how MTP services should be resolved.
• The 5 unit tests cover the key code paths cleanly (happy path, null bus, missing file).
• PublishArtifactAsync being internal keeps the API surface clean while enabling testability via InternalsVisibleTo.
• The TUnitTestFramework.cs variable extraction is a trivial but harmless readability improvement.

[claude]

Follow-up Review

Thanks for the latest refactor commit (36f72f1). A few items from the previous review have been addressed, but the critical one remains open.

---

Critical (still unresolved): Missing AddDataConsumer registration

The previous review flagged that removing AddDataConsumer may cause ConsumeAsync to never be called, leaving _updates empty and producing no HTML report. This is still not addressed in the latest diff. The code now has:

testApplicationBuilder.TestHost.AddTestHostApplicationLifetime(_ => htmlReporter);
testApplicationBuilder.TestHost.AddTestSessionLifetimeHandler(serviceProvider => { ... return htmlReporter; });

But there is no AddDataConsumer call for htmlReporter. The PR description asserts MTP auto-discovers IDataConsumer from AddTestSessionLifetimeHandler, but this remains unverified — the JUnitReporter and GitHubReporter both retain explicit AddDataConsumer registrations. A silent regression (no error, just an empty/missing report) is the risk here.

Action needed: Either add testApplicationBuilder.TestHost.AddDataConsumer(_ => htmlReporter) back, or add a comment citing the specific MTP source/documentation that confirms auto-discovery from AddTestSessionLifetimeHandler.

---

New issue (introduced in 36f72f1): Lost test coverage for write-failure path

The previous commit had a test PublishArtifactAsync_Is_NoOp_When_File_Does_Not_Exist. That test was removed and replaced with PublishArtifactAsync_Publishes_With_Correct_SessionUid — which is good coverage, but the file-not-exists / write-failure path now has no test.

The new gating logic (publish only when WriteFileAsync returns true) means an artifact won't be published if the write fails, but there's nothing asserting this. Given that WriteFileAsync returns false on failure without throwing, this is a subtle path worth covering.

---

Minor (still unresolved): Misleading test name

PublishArtifactAsync_Is_NoOp_When_SessionContext_Not_Set still exists and still tests the wrong thing — it tests that the method is a no-op when _messageBus is null, not when a session context is absent. Suggested name: PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected.

---

Minor (still unresolved): csproj indentation

In TUnit.Engine.Tests.csproj, the CrashDump package reference still has 4-space indentation while all others use 8 spaces.

---

What was addressed ✅

• AfterRunAsync comment is now accurate: // All work happens in OnTestSessionFinishingAsync.
• MockExtension was extracted to a shared TestInfrastructure/MockExtension.cs — eliminates the duplication across three test files cleanly.
• TUnitTestFramework.cs variable inlining is a small but clean improvement.
• Gating artifact publish on WriteFileAsync returning true rather than re-checking File.Exists is a good simplification that avoids a race condition.

[claude]

Follow-up Review

Thanks for the latest refactor commit (36f72f1). A few items from the previous review have been addressed, but the critical one remains open.

---

Critical (still unresolved): Missing AddDataConsumer registration

The previous review flagged that removing AddDataConsumer may cause ConsumeAsync to never be called, leaving _updates empty and producing no HTML report. This is still not addressed in the latest diff. The code now has:

testApplicationBuilder.TestHost.AddTestHostApplicationLifetime(_ => htmlReporter);
testApplicationBuilder.TestHost.AddTestSessionLifetimeHandler(serviceProvider => { ... return htmlReporter; });

But there is no AddDataConsumer call for htmlReporter. The PR description asserts MTP auto-discovers IDataConsumer from AddTestSessionLifetimeHandler, but this remains unverified — the JUnitReporter and GitHubReporter both retain explicit AddDataConsumer registrations. A silent regression (no error, just an empty/missing report) is the risk here.

Action needed: Either add testApplicationBuilder.TestHost.AddDataConsumer(_ => htmlReporter) back, or add a comment citing the specific MTP source/documentation that confirms auto-discovery from AddTestSessionLifetimeHandler.

---

New issue (introduced in 36f72f1): Lost test coverage for write-failure path

The previous commit had a test PublishArtifactAsync_Is_NoOp_When_File_Does_Not_Exist. That test was removed and replaced with PublishArtifactAsync_Publishes_With_Correct_SessionUid — which is good coverage, but the file-not-exists / write-failure path now has no test. The new gating logic (publish only when WriteFileAsync returns true) means an artifact won't be published if the write fails, but there's nothing asserting this.

---

Minor (still unresolved): Misleading test name

PublishArtifactAsync_Is_NoOp_When_SessionContext_Not_Set still tests the wrong scenario — it tests that the method is a no-op when _messageBus is null, not when a session context is absent. Suggested name: PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected.

---

Minor (still unresolved): csproj indentation

In TUnit.Engine.Tests.csproj, the CrashDump package reference still has 4-space indentation while all others use 8 spaces.

---

What was addressed

• AfterRunAsync comment is now accurate: // All work happens in OnTestSessionFinishingAsync.
• MockExtension extracted to shared TestInfrastructure/MockExtension.cs — eliminates duplication across three test files.
• TUnitTestFramework.cs variable inlining is a clean improvement.
• Gating artifact publish on WriteFileAsync returning true rather than re-checking File.Exists is a good simplification that avoids a race condition.

[claude]

Follow-up Review

Thanks for the latest refactor commit (36f72f1). A few items from the previous review have been addressed, but the critical one remains open.

---

Critical (still unresolved): Missing AddDataConsumer registration

The previous review flagged that removing AddDataConsumer may cause ConsumeAsync to never be called, leaving _updates empty and producing no HTML report. This is still not addressed in the latest diff. The code now has:

testApplicationBuilder.TestHost.AddTestHostApplicationLifetime(_ => htmlReporter);
testApplicationBuilder.TestHost.AddTestSessionLifetimeHandler(serviceProvider => { ... return htmlReporter; });

But there is no AddDataConsumer call for htmlReporter. The PR description asserts MTP auto-discovers IDataConsumer from AddTestSessionLifetimeHandler, but this remains unverified — the JUnitReporter and GitHubReporter both retain explicit AddDataConsumer registrations. A silent regression (no error, just an empty/missing report) is the risk here.

Action needed: Either add testApplicationBuilder.TestHost.AddDataConsumer(_ => htmlReporter) back, or add a comment citing the specific MTP source/documentation that confirms auto-discovery from AddTestSessionLifetimeHandler.

---

New issue (introduced in 36f72f1): Lost test coverage for write-failure path

The previous commit had a test PublishArtifactAsync_Is_NoOp_When_File_Does_Not_Exist. That test was removed and replaced with PublishArtifactAsync_Publishes_With_Correct_SessionUid — which is good coverage, but the file-not-exists / write-failure path now has no test. The new gating logic (publish only when WriteFileAsync returns true) means an artifact won't be published if the write fails, but there's nothing asserting this.

---

Minor (still unresolved): Misleading test name

PublishArtifactAsync_Is_NoOp_When_SessionContext_Not_Set still tests the wrong scenario — it tests that the method is a no-op when _messageBus is null, not when a session context is absent. Suggested name: PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected.

---

Minor (still unresolved): csproj indentation

In TUnit.Engine.Tests.csproj, the CrashDump package reference still has 4-space indentation while all others use 8 spaces.

---

What was addressed

• AfterRunAsync comment is now accurate: // All work happens in OnTestSessionFinishingAsync.
• MockExtension extracted to shared TestInfrastructure/MockExtension.cs — eliminates duplication across three test files.
• TUnitTestFramework.cs variable inlining is a clean improvement.
• Gating artifact publish on WriteFileAsync returning true rather than re-checking File.Exists is a good simplification that avoids a race condition.

[thomhurst]

Addressing the review items:

Critical (AddDataConsumer): Pushing back on this one. We empirically verified the auto-registration: when we tried to add an explicit AddDataConsumer(_ => htmlReporter) call alongside the AddTestSessionLifetimeHandler registration, MTP threw InvalidOperationException: Consumer registered two time for data type TestNodeUpdateMessage at startup. This confirms MTP does auto-discover and register the IDataConsumer interface when a session lifetime handler is registered. The smoke test output also confirmed a report was actually generated (In process file artifacts produced: ...report.html). The IDataConsumer registration is happening — just implicitly.

AfterRunAsync comment: Already fixed in a prior commit — the method is now => Task.CompletedTask; // All work happens in OnTestSessionFinishingAsync.

Test name: Fixed — PublishArtifactAsync_Is_NoOp_When_SessionContext_Not_Set → PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected.

csproj indentation: Fixed.

[claude]

Code Review

Summary

The core architectural decision here — moving report generation from AfterRunAsync to OnTestSessionFinishingAsync to ensure the MTP message bus is still live — is sound and well-reasoned. The PR is clean and the cleanup of MockExtension into a shared location is a nice improvement.

---

Critical: IDataConsumer Auto-Registration Assumption

The most important thing to verify before merging: the AddDataConsumer call was removed and replaced with AddTestSessionLifetimeHandler. The PR description states that MTP auto-registers the handler as IDataConsumer. If this assumption is wrong, ConsumeAsync will never be called and the HTML report will silently be empty.

In TestApplicationBuilderExtensions.cs (before):

testApplicationBuilder.TestHost.AddDataConsumer(serviceProvider => { ... return htmlReporter; });

After: this call is gone. It's worth adding a comment documenting which version of MTP introduced this auto-registration behavior (or a link to the relevant MTP source), so future maintainers understand why there's no explicit AddDataConsumer call.

---

Design: Property Injection for IMessageBus

The SetMessageBus setter is forced by the architecture (reporter is instantiated before the service provider is available), but it's worth noting a cleaner alternative: MTP's ITestSessionLifetimeHandler receives an ITestSessionContext — if the context exposes a way to publish data, that would eliminate the need for the setter entirely.

As written, there's a subtle initialization ordering dependency: SetMessageBus must be called before OnTestSessionFinishingAsync, which is guaranteed only because AddTestSessionLifetimeHandler's factory runs at startup. This is correct but fragile — a comment explaining the constraint would help:

// SetMessageBus is called by the AddTestSessionLifetimeHandler factory before any
// session events fire, so _messageBus is guaranteed to be set by OnTestSessionFinishingAsync.

---

Test Gap: Missing "File Not Exists" No-Op Test

The PR description lists 5 tests including a "file-not-exists no-op" case, but looking at HtmlReporterTests.cs there are only 4 distinct behavioral tests. The file-exists-check lives in WriteFileAsync (returning false), which gates the PublishArtifactAsync call in OnTestSessionFinishingAsync — but PublishArtifactAsync itself doesn't check for file existence. Consider either:

• Adding a test for the integration path where WriteFileAsync returns false (file write fails → artifact not published)
• Or documenting that the no-op case is covered by the WriteFileAsync return value

---

Minor: Test Assertion Could Be More Explicit

PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected passes by not throwing, which is correct but implicit. A more expressive pattern:

var bus = new CapturingMessageBus();
// Note: no reporter.SetMessageBus(bus) call
await reporter.PublishArtifactAsync(tempFile, new SessionUid("s"), CancellationToken.None);
bus.Published.ShouldBeEmpty(); // explicit: no events published without a bus

This makes the intent clearer and would catch a regression where the null check is removed.

---

What's Good

• Moving work to OnTestSessionFinishingAsync is the right call — the timing explanation in the PR description is clear and the fix is correct.
• WriteFileAsync returning bool instead of void is a cleaner design that makes the control flow explicit.
• Refactoring MockExtension to a shared file is a small but worthwhile cleanup.
• The #pragma warning disable TPEXP scoping is appropriate.

Overall this is solid work. The main ask is verifying (and documenting) the IDataConsumer auto-registration assumption since that's the load-bearing correctness guarantee of this change.

[thomhurst]

Addressed the open items from the latest review:

Critical (AddDataConsumer auto-registration): Added a comment in TestApplicationBuilderExtensions.cs documenting that MTP auto-registers IDataConsumer from AddTestSessionLifetimeHandler, and that adding an explicit call causes a startup exception ("Consumer registered two time for data type TestNodeUpdateMessage").

SetMessageBus ordering: Added a comment on SetMessageBus explaining the guarantee — the factory runs at startup before any session events fire.

Write-failure test gap: WriteFileAsync is private static so it can't be directly unit-tested without exposing it. Added a comment in OnTestSessionFinishingAsync documenting the gating behavior (WriteFileAsync returns false → PublishArtifactAsync is not called). The control flow is clear from the if (written) guard.

NoOp test assertion: PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected now asserts bus.Published.ShouldBeEmpty() explicitly.

Already fixed in prior commits: test name (SessionContext_Not_Set → MessageBus_Not_Injected), AfterRunAsync comment, csproj indentation.

[claude]

Code Review (follow-up — final state)

The previous critical concern about AddDataConsumer auto-registration has been properly addressed in the latest commits with both empirical evidence and in-code documentation. This review reflects the current state of the PR.

---

Architecture: Approved

Moving artifact publishing from AfterRunAsync to OnTestSessionFinishingAsync is the correct design decision — the MTP message bus is guaranteed live during session finishing, but may already be drained by the time AfterRunAsync fires. The code comment in TestApplicationBuilderExtensions.cs clearly explains why the explicit AddDataConsumer call was intentionally removed:

// MTP auto-registers IDataConsumer implementations returned from AddTestSessionLifetimeHandler,
// so no separate AddDataConsumer call is needed. Adding one causes a startup exception:
// "Consumer registered two time for data type TestNodeUpdateMessage".

This is the right level of documentation for a non-obvious MTP contract.

---

Minor: CapturingMessageBus nested inside HtmlReporterTests

CapturingMessageBus is a broadly-useful test double that other reporter tests might benefit from. Consider moving it to TestInfrastructure/ alongside MockExtension. This keeps the test class focused and makes the helper discoverable for future tests.

// TestInfrastructure/CapturingMessageBus.cs
internal sealed class CapturingMessageBus : IMessageBus { ... }

This is not a blocker — just a consistency suggestion given that MockExtension was already extracted to that location in this very PR.

---

Minor: Vacuous assertion in no-op test

In PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected, the CapturingMessageBus is created but never passed to the reporter. Asserting bus.Published.ShouldBeEmpty() is technically correct, but it would remain empty regardless — the bus was never wired up. The assertion documents intent, but doesn't guard against a future regression where the reporter uses a different bus.

A slightly stronger test would verify that no exception is thrown and use Assert.DoesNotThrow or simply leave the implicit pass:

// This verifies the null-bus guard doesn't throw, not that a connected bus was untouched
await reporter.PublishArtifactAsync(tempFile, new SessionUid("test-session-1"), CancellationToken.None);
// Implicit pass — no exception means the null guard worked

Again, not a blocker — the current test is still useful as documentation.

---

Positive: WriteFileAsync → bool return

The refactor of WriteFileAsync to return bool and gate artifact publishing on a successful write is a clean improvement. It avoids publishing a SessionFileArtifact pointing to a file that doesn't exist.

---

PR Description vs. Changed Files

The PR description lists TUnit.Engine/Framework/TUnitTestFramework.cs as a changed file, but it's not in the diff. Looks like the description may be slightly stale from a prior iteration — not a concern, just noting it.

---

Summary

All previous review items have been addressed. The design is sound, the new tests are meaningful, and the MockExtension consolidation is a nice bonus cleanup. The two minor items above are suggestions, not blockers.

Approved for merge.

[claude]

Code Review (follow-up — current state)

All items from previous reviews have been resolved. This review captures the current state.

---

Addressed since last review ✅

• ** extracted to ** — consistent with . The suggestion from the prior review was implemented.
• Test name corrected — is accurate and self-documenting.
• ** auto-registration documented** — the inline comment in TestApplicationBuilderExtensions.cs explains both why the explicit call was removed and that adding it back causes a duplicate-registration startup exception. This is the right level of documentation for a non-obvious MTP contract.
• csproj indentation — correct 8-space indentation throughout.

---

One minor item (carry-over, non-blocking)

In PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected, the CapturingMessageBus is created but never wired to the reporter, so bus.Published.ShouldBeEmpty() will always pass regardless of what the reporter does. The assertion documents intent but doesn't protect against a future regression where the reporter resolves a bus via a different path.

Not a blocker — the implicit pass (no exception thrown) is the real assertion here. An optional comment like // bus is never wired; asserting empty to document the no-op contract would make the intent clear, but this is purely cosmetic.

---

Overall

The design is sound, all previous review concerns have been addressed, and the PR is clean. The core change — using OnTestSessionFinishingAsync with an injected IMessageBus rather than AfterRunAsync — is the right architectural fix for the drained-bus problem.

Approved for merge.

[claude]

Code Review (follow-up — current state)

All items from previous reviews have been resolved. This review captures the current state.

---

Addressed since last review

• CapturingMessageBus extracted to TestInfrastructure/ — consistent with MockExtension. The suggestion from the prior review was implemented.
• Test name corrected — PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected is accurate and self-documenting.
• AddDataConsumer auto-registration documented — the inline comment in TestApplicationBuilderExtensions.cs explains both why the explicit call was removed and that adding it back causes a duplicate-registration startup exception. This is the right level of documentation for a non-obvious MTP contract.
• csproj indentation — correct 8-space indentation throughout.

---

One minor item (carry-over, non-blocking)

In PublishArtifactAsync_Is_NoOp_When_MessageBus_Not_Injected, the CapturingMessageBus is created but never wired to the reporter, so bus.Published.ShouldBeEmpty() will always pass regardless of what the reporter does. The assertion documents intent but does not protect against a future regression where the reporter resolves a bus via a different path.

Not a blocker — the implicit pass (no exception thrown) is the real assertion here. An optional comment like // bus is never wired; asserting empty to document the no-op contract would make the intent clear, but this is purely cosmetic.

---

Overall

The design is sound, all previous review concerns have been addressed, and the PR is clean. The core change — using OnTestSessionFinishingAsync with an injected IMessageBus rather than AfterRunAsync — is the correct architectural fix for the drained-bus problem.

Approved for merge.