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

Added logging to Coverage to help with debugging #341

Merged
merged 18 commits into from
Feb 27, 2019
Merged

Added logging to Coverage to help with debugging #341

merged 18 commits into from
Feb 27, 2019

Conversation

derhally
Copy link
Contributor

There was no indication why coverage didn't occur because the exception occurring inside Coverage class was being swallowed.

I was missing an dependency and had no way of knowing that. This should help.

Exceptions were being swallowed and there was no indication why coverage didn't occur
@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #341 into master will decrease coverage by 0.09%.
The diff coverage is 75%.

@@            Coverage Diff            @@
##           master     #341     +/-   ##
=========================================
- Coverage    87.1%   87.01%   -0.1%     
=========================================
  Files          31       32      +1     
  Lines        3033     3050     +17     
=========================================
+ Hits         2642     2654     +12     
- Misses        391      396      +5

@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #341 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
- Coverage    87.1%   86.99%   -0.11%     
==========================================
  Files          31       31              
  Lines        3033     3092      +59     
==========================================
+ Hits         2642     2690      +48     
- Misses        391      402      +11

@tonerdo
Copy link
Collaborator

tonerdo commented Feb 15, 2019

@derhally thanks a lot for this! Will take a look later today

src/coverlet.core/Logging/ILogger.cs Outdated Show resolved Hide resolved
src/coverlet.core/Logging/NullLogger.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

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

Added some thoughts

Thank's a lot for help!

src/coverlet.core/Logging/NullLogger.cs Outdated Show resolved Hide resolved
src/coverlet.core/Logging/ILogger.cs Show resolved Hide resolved
src/coverlet.core/Coverage.cs Show resolved Hide resolved
src/coverlet.console/Logging/ConsoleLogger.cs Outdated Show resolved Hide resolved
src/coverlet.core/Coverage.cs Outdated Show resolved Hide resolved
src/coverlet.console/Program.cs Show resolved Hide resolved
@MarcoRossignoli
Copy link
Collaborator

@derhally CI fail on MSBuildLogger.cs for wrong namespace Coverlet.Core.ILogger

Need to call BeginErrorReadLine and BeginOutputReadLine to start reading from those streams.
Remove message parameter from LogError with exception overload.
Use the current foreground color when writing Info to console.
@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Feb 16, 2019

@tonerdo @derhally I did some tests I cannot show LogInformation/LogVerbose(https://docs.microsoft.com/en-us/visualstudio/msbuild/obtaining-build-logs-with-msbuild?view=vs-2017#verbosity-settings) message, Warning and Exception are ok, do you have same issue?

@derhally
Copy link
Contributor Author

@tonerdo @derhally I did some tests I cannot show LogInformation/LogVerbose(https://docs.microsoft.com/en-us/visualstudio/msbuild/obtaining-build-logs-with-msbuild?view=vs-2017#verbosity-settings) message, Warning and Exception are ok, do you have same issue?

So, I completely glossed over that I didn't switch CoverageResultTask to use the logger instead of console. I fixed that.

It seems if you use "dotnet test" you need to specify -v n to see the coverage report and that's even with the MessageImportance set to High. I'm not sure if that is the expected behavior. I would think that people would want to see output by default. This is very odd.

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Feb 16, 2019

It seems if you use "dotnet test" you need to specify -v n to see the coverage report and that's even with the MessageImportance set to High. I'm not sure if that is the expected behavior. I would think that people would want to see output by default. This is very odd.

For now I think we should use Console.WriteLine and use logger only for "debugging" purpouse.
The odd thing is that if I try to add for instance

  _logger.LogInformation("TEST");
  _logger.LogWarning("WARNING");

and run build

 msbuild build.proj /verbosity:diagnostic

I get only warning notification

...
  Build completed. (TaskId:7)
   (TaskId:7)
D:\git\coverlet\build\Debug\coverlet.msbuild.targets(21,5): warning : WARNING [D:\git\coverlet\test\coverlet.core.tests\coverlet.core.tests.csproj] [D:\git\coverlet\build.proj]
  Test run for D:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\coverlet.core.tests.dll(.NETCoreApp,Version=v2.0) (TaskId:7)
  Microsoft (R) Test Execution Command Line Tool Version 16.0.0-preview-20181205-02 (TaskId:7)
  Copyright (c) Microsoft Corporation.  All rights reserved. (TaskId:7)
   (TaskId:7)
...

I dunno why information is not shown @tonerdo do you have some idea? Maybe you've more experience with msbuild.

@derhally
Copy link
Contributor Author

An option is to use Console for the Logger.Information and still use the MSBuild logger for Error and Warning so that those go through the build system

@MarcoRossignoli
Copy link
Collaborator

An option is to use Console for the Logger.Information and still use the MSBuild logger for Error and Warning so that those go through the build system

Could be a breaking change, because until today coverlet users expect console output with percentace, if we use build system log a "quiet" verbosity will suppress all output.

@MarcoRossignoli
Copy link
Collaborator

@derhally are you able to complete this PR if not I could do last minor open changes with no problem(if you're busy, you already did a great work on this)!
@tonerdo I think that logging could help in some of open issues what do you think?I mean we should add verbose logging in some parts(inclusion exclusion filter, file instrumenting mainly).

@derhally
Copy link
Contributor Author

derhally commented Feb 26, 2019

@MarcoRossignoli which track would you prefer, should the CovverageResultTask use

  1. Console.WriteLine as before
  2. Use ILogger api and change MSBuildLogger implementation to log to console?
  3. Use ILogger and leave the current implementation of MSBuildLogger

Either way, the behavior would be same as before.

@MarcoRossignoli
Copy link
Collaborator

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

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

@tonerdo LGTM!
Thank's again @derhally!

@tonerdo
Copy link
Collaborator

tonerdo commented Feb 27, 2019

@tonerdo I think that logging could help in some of open issues what do you think?I mean we should add verbose logging in some parts(inclusion exclusion filter, file instrumenting mainly).

@MarcoRossignoli totally agree. I'm thinking we could add a diag (or similar) flag to turn on verbose logging for debugging purposes

Copy link
Collaborator

@tonerdo tonerdo left a comment

Choose a reason for hiding this comment

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

Thank you @derhally!

@tonerdo tonerdo merged commit f258333 into coverlet-coverage:master Feb 27, 2019
@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Feb 27, 2019

@MarcoRossignoli totally agree. I'm thinking we could add a diag (or similar) flag to turn on verbose logging for debugging purposes

Seems that for now we can use dotnet test /p:CollectCoverage=true -v:n #341 (comment) I did some test with:
dotnet test "C:\git\coverlet\test\coverlet.core.tests\coverlet.core.tests.csproj" -c Debug /p:CollectCoverage=true /p:CoverletOutputFormat=opencover /p:Include=[coverlet.*]* -v:n

image
image

The worst part is that we get output also for others, but better than nothing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants