-
-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate from vstest
to Microsoft.Testing.Platform
#43
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Threading.Tasks; | ||
|
||
using Microsoft.Testing.Platform.CommandLine; | ||
using Microsoft.Testing.Platform.Extensions; | ||
using Microsoft.Testing.Platform.Extensions.CommandLine; | ||
|
||
namespace GitHubActionsTestLogger; | ||
|
||
internal sealed class CliOptionsProvider(GitHubTestReporterExtension extension) : ICommandLineOptionsProvider | ||
{ | ||
public const string ReportGitHubOption = "report-github"; | ||
public const string ReportGitHubSummaryOption = "report-github-summary"; | ||
public const string ReportGitHubTitleOption = "report-github-title"; | ||
public const string ReportGitHubMessageOption = "report-github-message"; | ||
|
||
public static class ReportGitHubSummaryArguments | ||
{ | ||
public const string IncludePassedTests = "includePassedTests"; | ||
public const string IncludeSkippedTests = "includeSkippedTests"; | ||
public const string IncludeNotFoundTests = "includeNotFoundTests"; | ||
} | ||
|
||
public string Uid => extension.Uid; | ||
public string Version => extension.Version; | ||
public string DisplayName => extension.DisplayName; | ||
public string Description => extension.Description; | ||
|
||
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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So with the current implementation, would this option be used like so?
In that case, I feel like it'd be more user friendly and idiomatic to have a separate option for each flag:
It's kind of a mouthful though, which is why I opted for the "nested" convention in the current version in the first place. |
||
new(ReportGitHubSummaryOption, "Defines the information to include in the summary", ArgumentArity.OneOrMore, isHidden: false), | ||
new(ReportGitHubTitleOption, "Defines the annotation title format used for reporting test failures", ArgumentArity.ExactlyOne, isHidden: false), | ||
new(ReportGitHubMessageOption, "Defines the annotation message format used for reporting test failures", ArgumentArity.ExactlyOne, isHidden: false), | ||
]; | ||
|
||
public Task<bool> IsEnabledAsync() => extension.IsEnabledAsync(); | ||
|
||
// This method is called once after all options are parsed and is used to validate the combination of options. | ||
public Task<ValidationResult> ValidateCommandLineOptionsAsync(ICommandLineOptions commandLineOptions) | ||
{ | ||
// We just want to validate that the sub options are set only if the main option is set. | ||
if (commandLineOptions.IsOptionSet(ReportGitHubOption)) | ||
{ | ||
return ValidationResult.ValidTask; | ||
} | ||
|
||
if (commandLineOptions.IsOptionSet(ReportGitHubSummaryOption) | ||
|| commandLineOptions.IsOptionSet(ReportGitHubTitleOption) | ||
|| commandLineOptions.IsOptionSet(ReportGitHubMessageOption)) | ||
Comment on lines
+50
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to refer to the list of available options via |
||
{ | ||
return ValidationResult.InvalidTask("The options 'report-github-summary', 'report-github-title', and 'report-github-message' can only be used if 'report-github' is set."); | ||
} | ||
|
||
return ValidationResult.ValidTask; | ||
} | ||
|
||
// This method is called once per option declared and is used to validate the arguments of the given option. | ||
// The arity of the option is checked before this method is called. | ||
public Task<ValidationResult> ValidateOptionArgumentsAsync(CommandLineOption commandOption, string[] arguments) | ||
{ | ||
if (commandOption.Name == ReportGitHubSummaryOption) | ||
{ | ||
if (arguments.Length > 3) | ||
{ | ||
return ValidationResult.InvalidTask($"The option '{ReportGitHubSummaryOption}' can have at most 3 arguments."); | ||
} | ||
|
||
if (arguments.Distinct().Count() != arguments.Length) | ||
{ | ||
return ValidationResult.InvalidTask($"The option '{ReportGitHubSummaryOption}' cannot have duplicate arguments."); | ||
} | ||
|
||
for (int i = 0; i < arguments.Length; i++) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
{ | ||
if (arguments[i] != ReportGitHubSummaryArguments.IncludePassedTests | ||
&& arguments[i] != ReportGitHubSummaryArguments.IncludeSkippedTests | ||
&& arguments[i] != ReportGitHubSummaryArguments.IncludeNotFoundTests) | ||
{ | ||
return ValidationResult.InvalidTask($"The option '{ReportGitHubSummaryOption}' can only have the arguments '{ReportGitHubSummaryArguments.IncludePassedTests}', '{ReportGitHubSummaryArguments.IncludeSkippedTests}', and '{ReportGitHubSummaryArguments.IncludeNotFoundTests}'."); | ||
} | ||
} | ||
} | ||
|
||
return ValidationResult.ValidTask; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||
<Project Sdk="Microsoft.NET.Sdk"> | ||||||
<Project Sdk="Microsoft.NET.Sdk"> | ||||||
|
||||||
<PropertyGroup> | ||||||
<TargetFrameworks>netstandard2.0;net9.0</TargetFrameworks> | ||||||
|
@@ -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" /> | ||||||
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="all" /> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<PackageReference Include="PolyShim" Version="1.15.0" PrivateAssets="all" /> | ||||||
<PackageReference Include="RazorBlade" Version="0.8.0" ExcludeAssets="compile;runtime" PrivateAssets="all" /> | ||||||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,24 @@ | ||||
using Microsoft.Testing.Platform.Builder; | ||||
using Microsoft.Testing.Platform.Extensions; | ||||
using Microsoft.Testing.Platform.Services; | ||||
|
||||
namespace GitHubActionsTestLogger; | ||||
|
||||
public static class GitHubReportExtensions | ||||
{ | ||||
/// <summary> | ||||
/// Adds GitHub report support to the Testing Platform Builder. | ||||
/// </summary> | ||||
/// <param name="testApplicationBuilder">The test application builder.</param> | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I typically don't include docs for parameters if their usage is obvious. |
||||
public static void AddGitHubReportProvider(this ITestApplicationBuilder testApplicationBuilder) | ||||
{ | ||||
var extension = new GitHubTestReporterExtension(); | ||||
|
||||
var compositeExtension = new CompositeExtensionFactory<GitHubTestReporter>(serviceProvider => | ||||
new GitHubTestReporter(extension, serviceProvider.GetCommandLineOptions())); | ||||
testApplicationBuilder.TestHost.AddDataConsumer(compositeExtension); | ||||
testApplicationBuilder.TestHost.AddTestSessionLifetimeHandle(compositeExtension); | ||||
|
||||
testApplicationBuilder.CommandLine.AddProvider(() => new CliOptionsProvider(extension)); | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
using System; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
using Microsoft.Testing.Platform.CommandLine; | ||
using Microsoft.Testing.Platform.Extensions.Messages; | ||
using Microsoft.Testing.Platform.Extensions.TestHost; | ||
using Microsoft.Testing.Platform.TestHost; | ||
|
||
namespace GitHubActionsTestLogger; | ||
|
||
/// <summary> | ||
/// A Microsoft.Testing.Platform extension that reports test run information to GitHub Actions. | ||
/// </summary> | ||
internal sealed class GitHubTestReporter(GitHubTestReporterExtension extension, ICommandLineOptions commandLineOptions) : IDataConsumer, ITestSessionLifetimeHandler | ||
{ | ||
private readonly TestReporterContext _context = new(GitHubWorkflow.Default, TestReporterOptions.Resolve(commandLineOptions)); | ||
|
||
public Type[] DataTypesConsumed { get; } = | ||
[ | ||
typeof(TestNodeUpdateMessage), | ||
]; | ||
|
||
public string Uid => extension.Uid; | ||
public string Version => extension.Version; | ||
public string DisplayName => extension.DisplayName; | ||
public string Description => extension.Description; | ||
|
||
public Task ConsumeAsync(IDataProducer dataProducer, IData value, CancellationToken cancellationToken) | ||
{ | ||
_context.HandleTestResult((TestNodeUpdateMessage)value); | ||
return Task.CompletedTask; | ||
} | ||
|
||
public Task<bool> IsEnabledAsync() => extension.IsEnabledAsync(); | ||
|
||
public Task OnTestSessionFinishingAsync(SessionUid sessionUid, CancellationToken cancellationToken) | ||
{ | ||
_context.HandleTestRunComplete(); | ||
return Task.CompletedTask; | ||
} | ||
|
||
public Task OnTestSessionStartingAsync(SessionUid sessionUid, CancellationToken cancellationToken) | ||
=> Task.CompletedTask; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of
const string
, we could have an initializedstatic readonly CommandLineOption
here, so it's easier to refer to it later, for example inValidateOptionArgumentsAsync
(wouldn't need to compare names then).