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

Enabled coverage with deterministic build #3478

Closed

Conversation

MarcoRossignoli
Copy link
Member

@MarcoRossignoli MarcoRossignoli commented Apr 5, 2020

Test coverage with deterministic build.

@sharwell let me know if update are enough to enable deterministic build or feel free to PR on my branch.
Also I'd like to know if coverage looks correct from your perspective.

On my local it seems to work

coverageFolder

ILSpySample

Repro commands coverlet-coverage/coverlet#363 (comment)

cc: @clairernovotny @tmat @tonerdo

@MarcoRossignoli MarcoRossignoli requested a review from a team as a code owner April 5, 2020 17:37
@codecov
Copy link

codecov bot commented Apr 5, 2020

Codecov Report

Merging #3478 into master will decrease coverage by 0.31%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3478      +/-   ##
==========================================
- Coverage   95.30%   94.99%   -0.32%     
==========================================
  Files        1025     1025              
  Lines      232931   232509     -422     
  Branches    15052    13833    -1219     
==========================================
- Hits       221990   220863    -1127     
- Misses       9278     9802     +524     
- Partials     1663     1844     +181     

@sharwell
Copy link
Member

sharwell commented Apr 5, 2020

I'm seeing some changes in reported coverage that I can't explain yet. Can you send a second pull request that updates coverlet without enabling deterministic build to see if that's the source of the difference?

@clairernovotny
Copy link
Member

@MarcoRossignoli One thought here: coverlet-coverage/coverlet@master...MarcoRossignoli:deterministicbuild#diff-d0e735819690b729526b502b93107f5eR6

It's possible that the test project may not be deterministic, for other reasons or incompatibilities, but the projects under test are. I think it'll "just work" without the condition in the test project.

Also, may want to rename the target wokround from GetPathMap to CoverletGetPathMap just to be explicit.

@MarcoRossignoli
Copy link
Member Author

MarcoRossignoli commented Apr 6, 2020

I'm seeing some changes in reported coverage that I can't explain yet.

If you're talking about that 0.31 is possible because every release we update branches alg(to trim roslyn generated ones that our users doesn't like). I'll revert DeterministicSourcePaths.

@MarcoRossignoli
Copy link
Member Author

@sharwell I updated package/switch let me know.

@sharwell
Copy link
Member

sharwell commented Apr 6, 2020

@MarcoRossignoli it looks like the new package is more accurate, though still lacking compared to e.g. OpenCover. For example, if we consider just NormalizeStringsToUppercase.cs:

Master branch:

https://codecov.io/gh/dotnet/roslyn-analyzers/src/51bc5612a8f462c655cc649bf009557ff4e2bb76/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/NormalizeStringsToUppercase.cs

This pull request:

https://codecov.io/gh/dotnet/roslyn-analyzers/src/9c530a286f2462d36fe2bdb0eee98d62bdfedd66/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/NormalizeStringsToUppercase.cs

The main problems still occurring are:

  1. It shows coverage is occurring for blank lines.

  2. It doesn't accurately show partial coverage for expressions spanning multiple lines. For example, consider the following lines:

    var toLowerWithCultureInfo = cultureInfo != null ?
    stringType.GetMembers("ToLower").OfType<IMethodSymbol>().FirstOrDefault(m => m.Parameters.Length == 1 && Equals(m.Parameters[0].Type, cultureInfo)) :
    null;

    If line 60 is showing as partially-covered, then at least one of line 61 or 62 should show as partially- or not-covered. The easiest way would be showing all of 60-62 as partially-covered.

@MarcoRossignoli
Copy link
Member Author

MarcoRossignoli commented Apr 6, 2020

@sharwell for "coverage" problem can you fill an issue on coverlet repo pls, better if with repro code so we can improve code coverage?
Here there are all the improvement to coverage in last months https://github.com/tonerdo/coverlet/blob/master/Documentation/Changelog.md, coverlet is not perfect yet but we're on a path, now we've a good real testing system so it's more simple fix this kind of bug.
At the moment in this PR my concern is to understand if coverage works with deterministic build.

EDIT: I don't have access to code coverage so I cannot see the diff.

image

@sharwell
Copy link
Member

sharwell commented Apr 6, 2020

The new build seems fine. My only reluctance now is the use of an unofficial build.

Once you log in to codecov.io with GitHub, it will work.

@MarcoRossignoli
Copy link
Member Author

MarcoRossignoli commented Apr 6, 2020

Once you log in to codecov.io with GitHub, it will work.

🤦‍♂

The new build seems fine. My only reluctance now is the use of an unofficial build.

This build is only for testing, we'll release official package as soon as it is ready.

Feel free to close this PR for now.

@sharwell sharwell closed this Apr 6, 2020
@sharwell
Copy link
Member

sharwell commented Apr 6, 2020

@MarcoRossignoli I'm looking forward to the update. I'm also excited by the prospect that the new test support will allow this project to start to cover these edge cases.

@MarcoRossignoli
Copy link
Member Author

I'm also excited by the prospect that the new test support will allow this project to start to cover these edge cases.

Feel free to report issue with some repo(if possible)!

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.

3 participants