-
Notifications
You must be signed in to change notification settings - Fork 289
Use EasyNamedPipes for dotnet test protocol #6814
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
base: main
Are you sure you want to change the base?
Conversation
| namespace Microsoft.Testing.Platform.IPC.Models; | ||
|
|
||
| internal sealed record CommandLineOptionMessage(string? Name, string? Description, bool? IsHidden, bool? IsBuiltIn); | ||
| internal sealed record CommandLineOptionMessage( |
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.
Are we missing the attribute here?
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.
I am not sure what's the right policy. If I look at the old serializer only the outer message was marked, the inner message is only an object of the outer.
| namespace Microsoft.Testing.Platform.IPC.Models; | ||
|
|
||
| internal sealed record DiscoveredTestMessage(string? Uid, string? DisplayName, string? FilePath, int? LineNumber, string? Namespace, string? TypeName, string? MethodName, string[]? ParameterTypeFullNames, TestMetadataProperty[] Traits); | ||
| internal sealed record DiscoveredTestMessage( |
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.
Are we missing the attribute here?
| namespace Microsoft.Testing.Platform.IPC.Models; | ||
|
|
||
| internal sealed record FileArtifactMessage(string? FullPath, string? DisplayName, string? Description, string? TestUid, string? TestDisplayName, string? SessionUid); | ||
| internal sealed record FileArtifactMessage( |
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.
Are we missing the attribute here?
| namespace Microsoft.Testing.Platform.IPC.Models; | ||
|
|
||
| internal sealed record SuccessfulTestResultMessage(string? Uid, string? DisplayName, byte? State, long? Duration, string? Reason, string? StandardOutput, string? ErrorOutput, string? SessionUid); | ||
| internal sealed record SuccessfulTestResultMessage( |
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.
Same here
| [property: PipePropertyId(8)] string? SessionUid); | ||
|
|
||
| internal sealed record FailedTestResultMessage(string? Uid, string? DisplayName, byte? State, long? Duration, string? Reason, ExceptionMessage[]? Exceptions, string? StandardOutput, string? ErrorOutput, string? SessionUid); | ||
| internal sealed record FailedTestResultMessage( |
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.
Same here
src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Models/TestResultMessages.cs
Outdated
Show resolved
Hide resolved
| [property: PipePropertyId(4)] bool? IsBuiltIn); | ||
|
|
||
| internal sealed record CommandLineOptionMessages(string? ModulePath, CommandLineOptionMessage[]? CommandLineOptionMessageList) : IRequest; | ||
| [PipeSerializableMessage("DotNetTestProtocol", 3)] |
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.
Should we extract the message ID constants to a static class?
| </TfmSpecificPackageFile> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <Compile Remove="Q:\dev\testfx\src\Platform\Microsoft.Testing.Platform\IPC\Serializers\BaseSerializer.cs" /> |
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.
Is this correct, should be a relative path?
|
idea: Could the source generator output the source code into place where we check it in? I assume it will be developed further and it would make it easier to review the changes that happen between check-ins. The change in the protocol will have impact on compatibility of older versions of MTP with dotnet test (at least), I don't think we have any explicit lower bound there. |
|
Trick that Roslyn uses for that: |
|
We also have some roundtrip tests in https://github.com/microsoft/testfx/blob/2001ece9cf87c9e5153b30d4b914d31941bdf976/test/UnitTests/Microsoft.Testing.Platform.UnitTests/IPC/ProtocolTests.cs We could consider asserting the actual bytes so that we know when there is a change to them and then we will be able to carefully review its compatibility. |
Fixes #6351