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

Fix/merge modules #211

Merged
merged 3 commits into from
Oct 11, 2018
Merged

Conversation

pape77
Copy link
Contributor

@pape77 pape77 commented Oct 9, 2018

Fixes Merge #204 !

@tonerdo So after playing a bit more around with my last change (which is already merged #204) , I realized I made a fuck-up because despite i was comparing by filename, you were already storing the keys for the Modules dictionary as complete paths in Coverage.cs.

This caused the comparison I modified in #204 to asume the keys for modules and Modules were the same. But i didn't change the values of these keys to be only the file name too, so they still had the complete path on it (and therefore, were different) .
As it tries to access the dictionary with the wrong key (asuming they were the same in both modules and Modules) it crashes.
If you see the different commits maybe it helps to understand what I was aiming to achieve.

The solution was much simpler. Now I just store the module keys with Path.GetFileName in Coverage.cs, so they can be correctly matched in CoverageResult.cs as it was done before my change.

I already tested it locally and worked for me for the example on issue #203

Hope this is clear enough!

Here's the output. Please merge!

Calculating coverage result...
  Generating report '\results\coverage.opencover.xml'

+----------------------+--------+--------+--------+
| Module               | Line   | Branch | Method |
+----------------------+--------+--------+--------+
| Rtl.ProjectName.Api         | 59.4%  | 66.7%  | 66.7%  |
+----------------------+--------+--------+--------+
| Rtl.ProjectName.Application | 100%   | 60%    | 100%   |
+----------------------+--------+--------+--------+
| Rtl.ProjectName.Data        | 24.8%  | 38.5%  | 54.5%  |
+----------------------+--------+--------+--------+

@codecov
Copy link

codecov bot commented Oct 9, 2018

Codecov Report

Merging #211 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #211   +/-   ##
=======================================
  Coverage   95.03%   95.03%           
=======================================
  Files          15       15           
  Lines        1590     1590           
=======================================
  Hits         1511     1511           
  Misses         79       79
Impacted Files Coverage Δ
src/coverlet.core/CoverageResult.cs 16.21% <0%> (ø) ⬆️
src/coverlet.core/Coverage.cs 99.49% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 967c180...00ec6ea. Read the comment docs.

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!!! Gonna do a test before I make a new release

@tonerdo tonerdo merged commit 3ac0868 into coverlet-coverage:master Oct 11, 2018
@pape77
Copy link
Contributor Author

pape77 commented Oct 12, 2018

@tonerdo Great! Please let me know when you do as I really need this!
Feel free to test it of course. I'm pretty confident it will work :)

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.

2 participants