Skip to content
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

Make compiler server logging explicit #48557

Merged
merged 4 commits into from
Nov 20, 2020
Merged

Make compiler server logging explicit #48557

merged 4 commits into from
Nov 20, 2020

Conversation

jaredpar
Copy link
Member

This changes the compiler server logging infrastructure to be an
explicit resource vs. an ambient authority. This both cleans up our
implementation a bit but also allows us to take advantage of xUnit
output logging in our unit tests.

The ITestOutputHelper can be the implementation of an
ICompilerServerLogger. This means that when compiler server tests fail
we get the full benefit of our logging infrastructure in the test
output vs. having to just guess at what failed here.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jaredpar jaredpar marked this pull request as ready for review October 15, 2020 21:18
@jaredpar jaredpar requested a review from a team as a code owner October 15, 2020 21:18
@jaredpar
Copy link
Member Author

@roslyn-compiler PTAL

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of small comments.

@jaredpar
Copy link
Member Author

@roslyn-compiler PTAL. Sorry, lost track of this PR for a bit but it's updated now to latest.

@RikkiGibson RikkiGibson self-assigned this Nov 19, 2020
@jaredpar
Copy link
Member Author

Sigh .... @dotnet/roslyn-compiler PTAL

Apparently I keep @ the wrong alias.

@@ -31,14 +32,16 @@ public sealed class ServerTests : BuildClientTests
private readonly string _pipeName = Guid.NewGuid().ToString("N");
private readonly BuildPaths _buildPaths;
private readonly List<ServerData> _serverDataList = new List<ServerData>();
private readonly XunitCompilerServerLogger _logger;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should this be an ICompilerServerLogger?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the field is unconditionally initialized as XunitCompilerServerLogger hence I decided to keep the field type reflective of that.

Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from a few nits

This changes the compiler server logging infrastructure to be an
explicit resource vs. an ambient authority. This both cleans up our
implementation a bit but also allows us to take advantage of xUnit
output logging in our unit tests.

The `ITestOutputHelper` can be the implementation of an
`ICompilerServerLogger`. This means that when compiler server tests fail
we get the full benefit of our logging infrastructure in the test
output vs. having to just guess at what failed here.
@jaredpar jaredpar merged commit 2693957 into dotnet:master Nov 20, 2020
@ghost ghost added this to the Next milestone Nov 20, 2020
@jaredpar jaredpar deleted the log branch November 20, 2020 21:57
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 20, 2020
* upstream/master: (47 commits)
  Make compiler server logging explicit (dotnet#48557)
  [master] Update dependencies from dotnet/arcade (dotnet#48274)
  Handle removed project in ExternalErrorDiagnosticUpdateSource
  Report an error for ref-returning auto-properties declared in an interface. (dotnet#49510)
  Add usings on paste (dotnet#48501)
  Determinism test needs to use bootstrap compiler (dotnet#49483)
  Simplify and reduce amount of work performed by SourcePropertySymbolBase constructor. (dotnet#49360)
  Updates test
  Simplify
  Split Document/Workspace handler into only handling open/closed documents respectively.
  only report watson once.
  Additional usage of a PooledHashset. (dotnet#49459)
  Loc checkin
  Update src/Features/CSharp/Portable/CodeRefactorings/ConvertLocalFunctionToMethod/CSharpConvertLocalFunctionToMethodCodeRefactoringProvider.cs
  Preserve annotation on comment trivia when performing formatting.
  Validate arguments to IAsyncCompletionSource methods
  Determinism fixes for AnonymousTypes in VB (dotnet#49467)
  Collect nfw information for a crash we're seeing.
  Make sure to not discard text changes when no reference changes are present
  Create an unsafe method from a local function when necessary (dotnet#49389)
  ...
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants