Register ServerTelemetry in the correct location of TestHostBuilder

[Copilot]

Bug Fix

What was the bug?

ServerTelemetry (which forwards telemetry events to the JSON-RPC client in server mode) was instantiated inside the ServerTestHost constructor as a private field but never registered in the service provider. Any code calling serviceProvider.GetTelemetryCollector() — including LogTestHostCreatedAsync — received the NopTelemetryService instead, silently dropping telemetry rather than forwarding it to the client.

How did you fix it?

• ServiceProvider: Added ReplaceService<T>(T newService) — replaces the first registered instance of type T in-place, preserving lookup order. Falls back to append if no existing instance is found.

• TestHostBuilder.Modes.cs (BuildServerTestHostAsync): After ServerTestHost is constructed, creates ServerTelemetry and registers it via ReplaceService<ITelemetryCollector>, displacing the NopTelemetryService that SetupCommonServicesAsync added earlier. This is the correct registration site — within TestHostBuilder, not inside the host itself.

• ServerTestHost: Removed the private _telemetryService field and its constructor assignment. The single call site now uses ServiceProvider.GetTelemetryCollector(), keeping the host decoupled from the concrete telemetry type.

// TestHostBuilder.Modes.cs — BuildServerTestHostAsync
ServerTestHost serverTestHost = new(context.ServiceProvider, ...);

// Register ServerTelemetry so all telemetry in server mode is forwarded
// to the client via JSON-RPC, replacing the earlier NopTelemetryService.
context.ServiceProvider.ReplaceService<ITelemetryCollector>(new ServerTelemetry(serverTestHost));

[Evangelink]

/review

[github-actions]

❌ Expert Code Review (command) failed. Please review the logs for details.

[Copilot]

Pull request overview

Fixes server-mode telemetry forwarding in Microsoft.Testing.Platform by ensuring ServerTelemetry is registered in the ServiceProvider (so GetTelemetryCollector() resolves to the forwarding implementation instead of NopTelemetryService), and by removing the direct ServerTelemetry coupling from ServerTestHost.

Changes:

• Added ServiceProvider.ReplaceService<T>(T) to replace the first matching service instance while preserving lookup order.
• Updated TestHostBuilder server-mode build path to register ServerTelemetry as the ITelemetryCollector.
• Updated ServerTestHost to obtain the telemetry collector from the ServiceProvider rather than maintaining its own private instance.

Show a summary per file

File	Description
src/Platform/Microsoft.Testing.Platform/Services/ServiceProvider.cs	Adds ReplaceService<T> to support in-place replacement of an existing registration.
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.Modes.cs	Registers ServerTelemetry in server mode by replacing the previously-registered telemetry collector.
src/Platform/Microsoft.Testing.Platform/Hosts/ServerTestHost.cs	Removes the private ServerTelemetry field and uses ServiceProvider.GetTelemetryCollector() for event logging.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 3

[Evangelink]

@copilot address review comments

[Copilot]

@copilot address review comments

Addressed all three review comments in commit Address review comments: null-guard, IsEnabled check, and ReplaceService unit tests:

1. Null-guard: Added _ = newService ?? throw new ArgumentNullException(nameof(newService)) to ReplaceService<T> (using the codebase pattern compatible with netstandard2.0).
2. Conditional registration: The ReplaceService<ITelemetryCollector> call in BuildServerTestHostAsync is now guarded by telemetryInformation.IsEnabled, so the NopTelemetryService is preserved when telemetry is opted out.
3. Unit tests: Added four tests to ServiceProviderTests.cs: ReplaceService_WhenMatchingServiceExists_ReplacesInPlace, ReplaceService_WhenNoMatchingServiceExists_AppendsService, ReplaceService_ReplacesFirstMatch_PreservesOtherServices, and ReplaceService_NullService_ThrowsArgumentNullException.

[Evangelink]

@copilot address review comments

[Copilot]

@copilot address review comments

All three review comments were already addressed in commit aef6566:

1. Null-guard — _ = newService ?? throw new ArgumentNullException(nameof(newService)) added to ReplaceService<T>.
2. Conditional registration — ReplaceService<ITelemetryCollector> is now guarded by if (context.ServiceProvider.GetTelemetryInformation().IsEnabled).
3. Unit tests — Four tests added covering replace-in-place, append-when-missing, ordering preservation, and null rejection. All pass.

[Evangelink]

@copilot fix the broken test

test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/ServerMode/v1.0.0/TestingPlatformClient.cs#L130

test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/ServerMode/v1.0.0/TestingPlatformClient.cs(130,1): error : [DiscoverAndRun ("net10.0")] [net11.0] Test method MSTest.Acceptance.IntegrationTests.ServerModeTests.DiscoverAndRun threw exception:
StreamJsonRpc.RemoteInvocationException: System.InvalidOperationException: Unexpected telemetry call, the telemetry is disabled.
at Microsoft.Testing.Platform.Telemetry.NopTelemetryService.LogEventAsync(String eventName, IDictionary`2 paramsMap, CancellationToken cancellationToken) in //src/Platform/Microsoft.Testing.Platform/Telemetry/NopTelemetryService.cs:line 13
at Microsoft.Testing.Platform.Hosts.ServerTestHost.ExecuteRequestAsync(RequestArgsBase args, String method, ServiceProvider perRequestServiceProvider, CancellationToken cancellationToken) in //src/Platform/Microsoft.Testing.Platform/Hosts/ServerTestHost.cs:line 572
at Microsoft.Testing.Platform.Hosts.ServerTestHost.HandleRequestCoreAsync(RequestMessage message, RpcInvocationState rpcInvocationState, CancellationToken cancellationToken) in //src/Platform/Microsoft.Testing.Platform/Hosts/ServerTestHost.cs:line 433
at Microsoft.Testing.Platform.Hosts.ServerTestHost.HandleRequestAsync(RequestMessage request, CancellationToken serverClosing, CancellationToken cancellationToken) in //src/Platform/Microsoft.Testing.Platform/Hosts/ServerTestHost.cs:line 344

[Copilot]

@copilot fix the broken test

test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/ServerMode/v1.0.0/TestingPlatformClient.cs#L130
...

Fixed in the latest commit. The IsEnabled guard was incorrect — ServerTelemetry in server mode is a protocol concern (JSON-RPC forwarding to the client), not a data-collection concern, so it must always be registered regardless of whether platform telemetry is enabled or disabled. Removed the guard so ReplaceService<ITelemetryCollector> is now unconditional, matching the original behavior where ServerTestHost always used a private ServerTelemetry instance directly.