Migrate from `vstest` to `Microsoft.Testing.Platform` #43

Evangelink · 2025-03-07T21:42:21Z

First draft at changing the codebase to be an MTP GH reporter

Closes #41

martincostello · 2025-03-08T07:37:49Z

GitHubActionsTestLogger/GitHubActionsTestLogger.csproj

@@ -24,7 +24,7 @@

  <ItemGroup>
    <PackageReference Include="CSharpier.MsBuild" Version="0.30.6" PrivateAssets="all" />
-    <PackageReference Include="Microsoft.TestPlatform.ObjectModel" Version="17.12.0" />
+	<PackageReference Include="Microsoft.Testing.Platform" Version="1.6.2" />


Suggested change

<PackageReference Include="Microsoft.Testing.Platform" Version="1.6.2" />

<PackageReference Include="Microsoft.Testing.Platform" Version="1.6.2" />

martincostello · 2025-03-08T07:39:44Z

GitHubActionsTestLogger/GitHubTestReporterExtension.cs

+    // A unique identifier for the extension across all registered extensions.
+    public string Uid => nameof(GitHubTestReporterExtension);
+
+    // TODO: Decide how to get version (e.g. hardcoded, from some generated file, from assembly version...)


AssemblyInformationalVersionAttribute I'd guess?

martincostello · 2025-03-08T07:45:06Z

GitHubActionsTestLogger/TestReporterContext.cs


        // Target framework
+        // TODO: Copy logic from platform: https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/OutputDevice/BrowserOutputDevice.cs#L78


I guess as long as TargetFrameworkParser is public a one line copy-paste is fine?

Assembly.GetEntryAssembly() would that not have any issues when running tests in an IDE? I guess with the MTP there is no special runner, so it shouldn't be a problem? Either way, I'd prefer not to take a logical dependency on that assumption if possible, so it would be great if the platform could expose the framework name.

Assembly.GetEntryAssembly() is problem only for nativeaot and single-file mode, this to me looked like yet another case where we would benefit from telling the hook into which dll (or give it the hook type) they are hooking into.

microsoft/testfx#5116

martincostello · 2025-03-08T07:46:36Z

GitHubActionsTestLogger/TestReporterContext.cs


        // Trait tokens
-        foreach (var trait in testResult.Traits.Union(testResult.TestCase.Traits))
+        // TODO: this is not enough, we would also need to handle properties from VSTest bridge


Is that a big piece of work, or relatively trivial?

martincostello · 2025-03-08T07:50:06Z

GitHubActionsTestLogger/CliOptionsProvider.cs

+    public IReadOnlyCollection<CommandLineOption> GetCommandLineOptions() =>
+        [
+            new(ReportGitHubOption, "Reports test run information to GitHub Actions", ArgumentArity.Zero, isHidden: false),
+            // TODO: Do you prefer multiple zero argument options or a single option with argument?


If it were me, I'd probably prefer individual ones as then when consuming via MSBuild it would be conceptually one-to-one when building up the arguments to pass through with TestingPlatformCommandLineArguments so conditionally deciding what to turn on/off is simpler.

So with the current implementation, would this option be used like so?

... --report-github-summary includePassedTests includeSkippedTests

In that case, I feel like it'd be more user friendly and idiomatic to have a separate option for each flag:

--report-github-summary-include-passed-tests --report-github-summary-include-skipped-tests

It's kind of a mouthful though, which is why I opted for the "nested" convention in the current version in the first place.

martincostello · 2025-03-08T07:53:44Z

GitHubActionsTestLogger/CliOptionsProvider.cs

+                return ValidationResult.InvalidTask($"The option '{ReportGitHubSummaryOption}' cannot have duplicate arguments.");
+            }
+
+            for (int i = 0; i < arguments.Length; i++)


Could this be simplified by just having a HashSet with the known argument names in it and then returning the error if the Except of the two is non-zero?

PR Overview

This PR updates the codebase to support only MTP by renaming logging components (from TestLogger to TestReporter) and updating their corresponding options and contexts.

Renames TestLoggerOptions/Context to TestReporterOptions/Context.
Updates GitHub report provider configuration and integration.
Removes old VS Test Platform dependencies and adapts to MTP.

Reviewed Changes

File	Description
GitHubActionsTestLogger/CliOptionsProvider.cs	Introduces command line options for GitHub reporting.
GitHubActionsTestLogger/TestReporterOptions.cs	Defines reporter options and resolves command line parameters.
GitHubActionsTestLogger/GitHubReportExtensions.cs	Configures integration with the testing platform builder.
GitHubActionsTestLogger/GitHubTestReporterExtension.cs	Provides basic extension properties and enablement.
GitHubActionsTestLogger/TestingPlatformBuilderHook.cs	Hooks into the testing platform to register the GitHub reporter.
GitHubActionsTestLogger/GitHubTestReporter.cs	Implements the GitHub reporter consuming test messages.
GitHubActionsTestLogger/TestReporterContext.cs	Implements the context to handle test results and generate annotations and summary.
GitHubActionsTestLogger/TestRunStatistics.cs	Adjusts test run statistics to support the new reporter context.
GitHubActionsTestLogger.Tests/*	Updates tests to reflect reporter context and options renaming.
GitHubActionsTestLogger/TestLoggerOptions.cs & TestLogger.cs	Removes obsolete logger classes.

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

Comments suppressed due to low confidence (6)

GitHubActionsTestLogger/TestReporterContext.cs:91

The variable 'args' is used but is not defined in the updated HandleTestResult method; consider replacing it with the appropriate property from testNodeUpdateMessage.

args.Result.TryGetSourceFilePath(),

GitHubActionsTestLogger/TestReporterContext.cs:91

The variable 'args' is used where it is undefined; update this to extract source line information from testNodeUpdateMessage or another valid source.

args.Result.TryGetSourceLine()

GitHubActionsTestLogger/TestReporterContext.cs:122

The '_testRunCriteria' variable is referenced but was removed from the context; consider removing this usage or reintroducing the necessary declaration.

var targetFramework = _testRunCriteria?.TryGetTargetFramework() ?? "Unknown Target Framework";

GitHubActionsTestLogger/TestReporterContext.cs:130

The method HandleTestRunComplete no longer receives 'args', yet it is used here to extract test statistics; update this logic to use available data or adjust the method signature accordingly.

(int?)args.TestRunStatistics?[TestOutcome.Passed] ?? _testResults.Count(r => r.Outcome == TestOutcome.Passed),

GitHubActionsTestLogger/TestReporterContext.cs:90

The variable 'github' is used but not defined in the TestReporterContext; please ensure an instance of GitHubWorkflow or equivalent is provided to enable annotation creation.

github.CreateErrorAnnotation(

GitHubActionsTestLogger/TestReporterContext.cs:113

The 'github' variable is referenced for creating the summary, but it is undefined in this context; consider injecting or defining the necessary GitHub workflow instance.

github.CreateSummary(template.Render());

Tyrrrz · 2025-03-09T13:17:20Z

GitHubActionsTestLogger/CliOptionsProvider.cs

+    public IReadOnlyCollection<CommandLineOption> GetCommandLineOptions() =>
+        [
+            new(ReportGitHubOption, "Reports test run information to GitHub Actions", ArgumentArity.Zero, isHidden: false),
+            // TODO: Do you prefer multiple zero argument options or a single option with argument?


So with the current implementation, would this option be used like so?

... --report-github-summary includePassedTests includeSkippedTests

In that case, I feel like it'd be more user friendly and idiomatic to have a separate option for each flag:

--report-github-summary-include-passed-tests --report-github-summary-include-skipped-tests

It's kind of a mouthful though, which is why I opted for the "nested" convention in the current version in the first place.

Tyrrrz · 2025-03-09T13:18:28Z

GitHubActionsTestLogger/CliOptionsProvider.cs

+        if (commandLineOptions.IsOptionSet(ReportGitHubSummaryOption)
+            || commandLineOptions.IsOptionSet(ReportGitHubTitleOption)
+            || commandLineOptions.IsOptionSet(ReportGitHubMessageOption))


Would it be possible to refer to the list of available options via GetCommandLineOptions() here so as to avoid having multiple sources of truths?

Tyrrrz · 2025-03-09T13:19:35Z

GitHubActionsTestLogger/CliOptionsProvider.cs

+
+internal sealed class CliOptionsProvider(GitHubTestReporterExtension extension) : ICommandLineOptionsProvider
+{
+    public const string ReportGitHubOption = "report-github";


Maybe instead of const string, we could have an initialized static readonly CommandLineOption here, so it's easier to refer to it later, for example in ValidateOptionArgumentsAsync (wouldn't need to compare names then).

Tyrrrz · 2025-03-09T13:20:27Z

GitHubActionsTestLogger/GitHubReportExtensions.cs

+    /// <summary>
+    /// Adds GitHub report support to the Testing Platform Builder.
+    /// </summary>
+    /// <param name="testApplicationBuilder">The test application builder.</param>


Suggested change

/// <param name="testApplicationBuilder">The test application builder.</param>

I typically don't include docs for parameters if their usage is obvious.

Tyrrrz · 2025-03-09T13:25:28Z

GitHubActionsTestLogger/TestReporterContext.cs


        // Target framework
+        // TODO: Copy logic from platform: https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/OutputDevice/BrowserOutputDevice.cs#L78


Assembly.GetEntryAssembly() would that not have any issues when running tests in an IDE? I guess with the MTP there is no special runner, so it shouldn't be a problem? Either way, I'd prefer not to take a logical dependency on that assumption if possible, so it would be great if the platform could expose the framework name.

Tyrrrz · 2025-03-09T13:28:02Z

GitHubActionsTestLogger/GitHubActionsTestLogger.csproj

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;net9.0</TargetFrameworks>


By the way, is there any guidance on which frameworks the reporter library should target? I believe with VSTest netstandard2.0 was a requirement/strong recommendation for a reason I struggle to remember.

The reason for VSTest is netstandard2.0 because your project will restore the dll, and then it will be imported by the vstest.console that is in use, either .NET Framework based vstest.console.exe in visual studio, or .NET based vstest.console.dll in dotnet test. The logger is not connected to the TFM of your project, and so netstandard2.0 (and ideally no dependencies) are recommended.

The reporters in MTP run inside of their respective processes, so the TFMs you support are 1:1 to what your userbase uses for their projects. You probably want to target netstandard2.0 to cover .NET Framework, and net8.0 as the oldest supported .NET.

This also brings the question: in dotnet test and vstest.console the loggers behaved differently. In dotnet.test you will get 1 instance of vstest.console per project, and so a 1 report per project.

In vstest.console.exe you will get 1 report per run (which is either solution, or a collection of dlls).

I assume most users use dotnet test to run tests on GH actions, and so the 1 report per project is preferred? This is also how MTP does it, because the reporter runs 1 per test project / dll.

Makes sense, thanks @nohwnd.

Yes, I think the current assumption is that the logger lifecycle is limited to one test project.

GitHubActionsTestLogger/GitHubActionsTestLogger/TestLoggerContext.cs

Lines 102 to 104 in dfd847d

var testSuite =

_testRunCriteria?.Sources?.FirstOrDefault()?.Pipe(Path.GetFileNameWithoutExtension)

?? "Unknown Test Suite";

Tyrrrz · 2025-03-09T13:28:59Z

GitHubActionsTestLogger/TestingPlatformBuilderHook.cs

+    /// <param name="testApplicationBuilder">The test application builder.</param>
+    /// <param name="_">The command line arguments.</param>


Suggested change

/// <param name="testApplicationBuilder">The test application builder.</param>

/// <param name="_">The command line arguments.</param>

Tyrrrz · 2025-03-09T13:30:14Z

Would be nice if we could also update the Demo project to use MTP as well 🙏🏻

nohwnd · 2025-03-10T09:33:11Z

Hehe, I was actually working on the code to support both at the same time 😁

nohwnd · 2025-03-10T09:38:13Z

Because I missed this comment, saying that you are okay to migrate:

#41 (comment)

fwiw code is here (the MTP part is heavily not done) #45 feel free to close.

nohwnd · 2025-03-10T09:44:30Z

GitHubActionsTestLogger/TestReporterContext.cs

+        Exception? exception = testNodeStateProperty switch
+        {
+            FailedTestNodeStateProperty failed => failed.Exception,
+            ErrorTestNodeStateProperty error => error.Exception,


Suggested change

ErrorTestNodeStateProperty error => error.Exception,

ErrorTestNodeStateProperty error => error.Exception,

CancelledTestNodeStateProperty cancelled => cancelled.Exception,

TimeoutTestNodeStateProperty timeout => timeout.Exception,

nohwnd · 2025-03-10T09:47:25Z

GitHubActionsTestLogger/TestReporterContext.cs


        // Target framework
+        // TODO: Copy logic from platform: https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/OutputDevice/BrowserOutputDevice.cs#L78


Assembly.GetEntryAssembly() is problem only for nativeaot and single-file mode, this to me looked like yet another case where we would benefit from telling the hook into which dll (or give it the hook type) they are hooking into.

microsoft/testfx#5116

Evangelink · 2025-03-19T08:06:40Z

Blocked by microsoft/testfx#5265

I'll address the other comments and do some ccleanup this week,

Update codebase to be supporting only MTP

75c6b9c

Evangelink mentioned this pull request Mar 7, 2025

Migrate from vstest to Microsoft.Testing.Platform #41

Open

5 tasks

martincostello reviewed Mar 8, 2025

View reviewed changes

Tyrrrz added the enhancement New feature or request label Mar 9, 2025

Tyrrrz requested a review from Copilot March 9, 2025 13:12

Copilot AI reviewed Mar 9, 2025

View reviewed changes

Tyrrrz reviewed Mar 9, 2025

View reviewed changes

Tyrrrz changed the title ~~Update codebase to be supporting only MTP~~ Migrate from vstest to Microsoft.Testing.Platform Mar 9, 2025

nohwnd reviewed Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from `vstest` to `Microsoft.Testing.Platform` #43

Migrate from `vstest` to `Microsoft.Testing.Platform` #43

Evangelink commented Mar 7, 2025

martincostello Mar 8, 2025

martincostello Mar 8, 2025

martincostello Mar 8, 2025

Tyrrrz Mar 9, 2025

nohwnd Mar 10, 2025

martincostello Mar 8, 2025

martincostello Mar 8, 2025

Tyrrrz Mar 9, 2025

martincostello Mar 8, 2025

Tyrrrz Mar 9, 2025

Tyrrrz Mar 9, 2025

Tyrrrz Mar 9, 2025

Tyrrrz Mar 9, 2025

Tyrrrz Mar 9, 2025

Tyrrrz Mar 9, 2025

nohwnd Mar 10, 2025 •

edited

Loading

Tyrrrz Mar 11, 2025

Tyrrrz Mar 9, 2025

Tyrrrz commented Mar 9, 2025

nohwnd commented Mar 10, 2025

nohwnd commented Mar 10, 2025

nohwnd Mar 10, 2025

nohwnd Mar 10, 2025

Evangelink commented Mar 19, 2025 •

edited

Loading

	<PackageReference Include="Microsoft.Testing.Platform" Version="1.6.2" />
	<PackageReference Include="Microsoft.Testing.Platform" Version="1.6.2" />


		// Target framework
		// TODO: Copy logic from platform: https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/OutputDevice/BrowserOutputDevice.cs#L78

	var testSuite =
	_testRunCriteria?.Sources?.FirstOrDefault()?.Pipe(Path.GetFileNameWithoutExtension)
	?? "Unknown Test Suite";

		/// <param name="testApplicationBuilder">The test application builder.</param>
		/// <param name="_">The command line arguments.</param>

Migrate from vstest to Microsoft.Testing.Platform #43

Are you sure you want to change the base?

Migrate from vstest to Microsoft.Testing.Platform #43

Conversation

Evangelink commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nohwnd Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyrrrz commented Mar 9, 2025

nohwnd commented Mar 10, 2025

nohwnd commented Mar 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evangelink commented Mar 19, 2025 • edited Loading

Migrate from `vstest` to `Microsoft.Testing.Platform` #43

Migrate from `vstest` to `Microsoft.Testing.Platform` #43

nohwnd Mar 10, 2025 •

edited

Loading

Evangelink commented Mar 19, 2025 •

edited

Loading