Only push output device messages to Test Explorer, don't push logs #4178
Conversation
Logs are intended to be used for diagnosing. They shouldn't be localized and they shouldn't be pushed to Test Explorer.
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/ServerModeTests.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/ServerModePerCallOutputDevice.cs
Outdated
Show resolved
Hide resolved
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public Task DisplayBannerAsync(string? bannerMessage) => Task.CompletedTask; |
There was a problem hiding this comment.
Let's handle this case and print as debug.
@MarcoRossignoli @drognanar it feels like it would be too verbose to print as Info for TE but maybe we still prefer doing it. WDYT?
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public Task DisplayBannerAsync(string? bannerMessage) => Task.CompletedTask; | ||
|
|
||
| public Task DisplayBeforeSessionStartAsync() => Task.CompletedTask; |
There was a problem hiding this comment.
Let's emit a debug message here and when in trace we should emit a localized message with the path to the log file to help users looking up the log.
| @@ -450,13 +444,20 @@ private async Task<ResponseArgsBase> ExecuteRequestAsync(RequestArgsBase args, s | |||
|
|
|||
| DateTimeOffset adapterLoadStart = _clock.UtcNow; | |||
|
|
|||
| IPlatformOutputDevice outputDevice = ServiceProvider.GetRequiredService<IPlatformOutputDevice>(); | |||
| // TODO: What if some one wants to have their own output device that's also compatible with server mode in IDE? | |||
There was a problem hiding this comment.
I think here we need to do something better.
Despite IPlatformOutputDevice is a bit special and the implementation needs to take into account more scenario(like app restart and correct handling for the "output") I'd like to abstract it from the "modes".
IPlatformOutputDevice implements IOutputDevice and today we pass to the IOutputDevice requestors the IPlatformOutputDevice implementation as-is.
Clearly this is not more good if we need to route display info due to modes.
So we should when IOutputDevice is requested from an extension to proxy it to be able to forward to the IPlatformOutputDevice but also to other internal part of the platform that needs it, for instance the server mode.
High level idea
GetService<IOutputDevice>() -> OutputDeviceRouter -> ArbitraryPlatformOutputDeviceImplementation & ServerModeForwarder
In this way we don't need special or custom output device implementation for old and possible new modes and we can keep the "current" output device choice.
We cannot forward the ArbitraryPlatformOutputDeviceImplementation display..but I would expect that the info printed by the "device" are not important...the device output goal is to "show information" in "some way" it doesn't affect and should not affect in any way the execution, so information produced by this extension shouldn't be so useful.
cc: @Evangelink
microsoft-github-policy-service
bot
added
Area: Server Mode
Area: Testing Platform
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/ProxyPlatformOutputDevice.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // Looks like nothing in this message to really be localized? | ||
| // All are class names, method names, property names, and placeholders. So none is localizable? | ||
| await ServiceProvider.GetOutputDevice().DisplayAsync( |
There was a problem hiding this comment.
I think that use the GetAwaiter().GetResult() is better here. I don't expect a log of call here and awaiting a void we could lose the message.
| @@ -15,15 +16,15 @@ public void SetPlatformOutputDevice(Func<IServiceProvider, IPlatformOutputDevice | |||
| _platformOutputDeviceFactory = platformOutputDeviceFactory; | |||
| } | |||
|
|
|||
| public IPlatformOutputDevice Build(ServiceProvider serviceProvider) | |||
| internal ProxyPlatformOutputDevice Build(ServiceProvider serviceProvider, bool useServerModeOutputDevice) | |||
Logs are intended to be used for diagnosing. They shouldn't be localized and they shouldn't be pushed to Test Explorer.
Fixes #4213
Linked to AB#2275339