chore: Rework MTP coverage analysis

[richardwerkman]

related to #3408

todo:

• Remove excessive console logging
• Fix timeout calculation
• Fix timeout trigger

Console logging was excessive. All the RPC requests were logged directly to the console. This pr fixes that by logging to a file instead of console.

The pr also fixes a bug in timeout calculation. The timeout value kept increasing for every mutant being tested. This made the timeout value grow too large on a large project. At one point stryker waited for 23 hours per timeout...

[Copilot]

Pull request overview

This PR addresses a significant performance issue where excessive console logging from JSON-RPC communication was causing Stryker.NET to take hours instead of minutes. The solution redirects JSON-RPC trace output from the console to dedicated log files when file logging is enabled.

Changes:

• Refactored RPC logging from console-based to file-based output via a new FileRpcListener class
• Changed the enableDiagnostic boolean parameter to rpcLogFilePath nullable string parameter across the test server connection infrastructure
• Added logic to generate per-instance RPC log file paths when LogToFile option is enabled
• Removed unused Serilog.Events import

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
TestingPlatformClient.cs	Updated constructor to accept optional log file path instead of boolean flag; creates FileRpcListener when path is provided
RPC/ConsoleRpcListener.cs	Renamed class to FileRpcListener and reimplemented to write to file instead of console; added StreamWriter disposal
ITestServerConnectionFactory.cs	Updated CreateClient interface signature to accept string? rpcLogFilePath parameter
DefaultTestServerConnectionFactory.cs	Updated CreateClient implementation to pass rpcLogFilePath to TestingPlatformClient constructor
AssemblyTestServer.cs	Added BuildRpcLogFilePath method to generate log file paths; removed unused Serilog import; updated to pass log path to CreateClient

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

integrationtest/TargetProjects/MicrosoftTestPlatform/UnitTests.MSTest/SampleTests.cs:10

• SampleTests.cs uses MSTest types ([TestClass], [TestMethod], [DataRow], Assert) but the using Microsoft.VisualStudio.TestTools.UnitTesting; directive was removed. This will not compile unless these are referenced via global using or fully-qualified names. Add the MSTest using back (or introduce a global using for the project) so the integration test target project builds.

using TargetProject.StrykerFeatures;

namespace NetCoreTestProject.MSTest.MTP;

[TestClass]
public class SampleTests
{
    [TestMethod]
    [DataRow(29, false)]
    [DataRow(31, true)]

[Copilot]

Pull request overview

Copilot reviewed 38 out of 39 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (3)

src/Stryker.TestRunner.MicrosoftTestPlatform/RPC/FileRpcListener.cs:1

• Path.GetDirectoryName(filePath) can be null (e.g., when filePath is just a filename). The null-forgiving operator will route this into the catch block and degrade logging unnecessarily. Handle the null directory case explicitly (only call CreateDirectory when the directory part is non-null/non-empty)."
  src/Stryker.TestRunner.MicrosoftTestPlatform/SingleMicrosoftTestPlatformRunner.cs:1
• RunAllTestsInternalAsync processes a single assembly (and returns (Result, TimedOut, DiscoveredTests) for that assembly). Consider renaming to reflect the actual responsibility (e.g., ProcessSingleAssemblyAsync / RunAssemblyTestsInternalAsync) to reduce confusion and keep call sites readable.
  integrationtest/TargetProjects/MicrosoftTestPlatform/UnitTests.MSTest/SampleTests.cs:1
• This file uses MSTest attributes ([DataTestMethod], [DataRow], [TestMethod]) and Assert, but the explicit using Microsoft.VisualStudio.TestTools.UnitTesting; was removed. If this project doesn’t define an equivalent global using, this will fail to compile. Either re-add the using here or ensure the project has a global using Microsoft.VisualStudio.TestTools.UnitTesting;.

using TargetProject.StrykerFeatures;

[Copilot]

Pull request overview

Copilot reviewed 39 out of 40 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/Stryker.TestRunner.MicrosoftTestPlatform/RPC/FileRpcListener.cs:1

• Path.GetDirectoryName(filePath) can return null when filePath is just a filename (no directory component). In that case this throws and you silently degrade to StreamWriter.Null, meaning --log-to-file may appear enabled but produces no file. Consider handling the “no directory” case explicitly (e.g., only calling CreateDirectory when the directory is non-empty, or defaulting to the current directory) so relative filenames work reliably.
  src/Stryker.TestRunner.MicrosoftTestPlatform/SingleMicrosoftTestPlatformRunner.cs:1
• RunAllTestsInternalAsync now operates on a single assembly (string assembly) and returns per-assembly results, which makes the name misleading (it reads like “all assemblies”). Renaming to something like RunAssemblyTestsInternalAsync / ProcessAssemblyAsync would reduce confusion, especially since this method is virtual and used for test overrides.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 40 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

src/Stryker.TestRunner.MicrosoftTestPlatform/RPC/FileRpcListener.cs:1

• Path.GetDirectoryName(filePath) can return null (e.g., when filePath is just a filename without a directory). The null-forgiving operator forces an exception path and silently degrades to StreamWriter.Null, which can unintentionally disable RPC logs. Prefer explicitly handling a null/empty directory (skip CreateDirectory in that case) so valid relative paths still work.
  src/Stryker.TestRunner.MicrosoftTestPlatform/ProcessHandle.cs:1
• Calling Task.Wait(...) during disposal introduces a blocking wait that can slow teardown (potentially 5s per process) and can be problematic in environments sensitive to blocking (e.g., limited-thread test runners). Consider using a non-blocking approach where possible (e.g., WaitAsync with a timeout in async disposal paths) or minimizing the wait to only cases where a clean shutdown is required, while still ensuring commandTask is safely disposed.
  src/Stryker.TestRunner.MicrosoftTestPlatform/SingleMicrosoftTestPlatformRunner.cs:1
• accumulator.HasTimeout is only set inside the if (discoveredTests is not null) block. If RunAssemblyTestsAsync can return timedOut == true with discoveredTests == null (e.g., discovery failure but test execution/connection still hangs), the run may incorrectly return a non-timeout TestRunResult even though an assembly timed out. Set HasTimeout based on timedOut independently of discoveredTests (and keep the HandleAssemblyTimeoutAsync call conditional on having discovery data).
  src/Stryker.TestRunner.MicrosoftTestPlatform/SingleMicrosoftTestPlatformRunner.cs:1
• accumulator.HasTimeout is only set inside the if (discoveredTests is not null) block. If RunAssemblyTestsAsync can return timedOut == true with discoveredTests == null (e.g., discovery failure but test execution/connection still hangs), the run may incorrectly return a non-timeout TestRunResult even though an assembly timed out. Set HasTimeout based on timedOut independently of discoveredTests (and keep the HandleAssemblyTimeoutAsync call conditional on having discovery data).

[Copilot]

Pull request overview

Copilot reviewed 39 out of 40 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

src/Stryker.TestRunner.MicrosoftTestPlatform/RPC/FileRpcListener.cs:1

• Path.GetDirectoryName(filePath) can return null (e.g., when filePath is just "rpc.log" or a root path). The null-forgiving operator will turn that into a runtime exception before you hit the catch logic. Consider guarding the directory creation (only call CreateDirectory when the directory part is non-null/non-empty) so the listener can safely handle relative filenames too.
  src/Stryker.TestRunner.MicrosoftTestPlatform/RPC/FileRpcListener.cs:1
• TraceListener.Write/WriteLine may be called concurrently from multiple threads; StreamWriter is not thread-safe, which can interleave or corrupt log output. Consider synchronizing writes (e.g., a private lock around _writer.Write*) or wrapping the writer with a thread-safe TextWriter.Synchronized(...).
  stryker-on-stryker.ps1:1
• Adding --open-report:dashboard in the CI-driven script is risky: --open-report-style flags commonly try to open a browser/URL, which may fail or behave poorly on headless CI agents. Also, the --open-report:dashboard syntax (colon-delimited) is unusual for CLI args; it may need to be --open-report dashboard / --open-report=dashboard depending on the option parser. Consider gating “open report” behind an explicit switch/env var for local runs and using the documented argument syntax to avoid CI failures.

[Copilot]

CliTestServerProcess (as shown in the hunk) is constructed with (CommandTask<CommandResult> commandTask, Stream outputStream), but Dispose() now references _processHandle, which is not in scope for a primary-constructor parameter list like this unless it’s declared elsewhere in the class body. This will fail to compile unless _processHandle was also added as a field/parameter. Either update the CliTestServerProcess constructor to accept/store the IProcessHandle (recommended, given the new ProcessHandle disposal behavior), or revert to disposing commandTask here and ensure the process handle lifecycle is owned consistently by one component.

[Copilot]

The PR description focuses on MTP coverage analysis, RPC logging, and timeout fixes, but this PR also removes coverlet.collector from multiple integration/example projects (and updates lockfiles accordingly). If this is intentional and required for the MTP changes, it would be helpful to mention it explicitly in the PR description (or split it into a separate PR) to keep the scope clear and to simplify review/rollback.

[sonarqubecloud]

Quality Gate failed

Failed conditions
54.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud