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 for: Merging two resultsets marks uncovered line as irrelevant #518

Closed
wants to merge 3 commits into from

Conversation

michelgrootjans
Copy link

Fixes issue #517

@GlennVB
Copy link

GlennVB commented Aug 19, 2016

This might be a duplicate of #513

@hanazuki
Copy link
Contributor

hanazuki commented Sep 1, 2016

Only this example behaves differently against master:

  1) merge helpers SimpleCov::ArrayMergeHelper numbers and empty should eq [0]
     Failure/Error: it { expect(merge([], [0])).to eq [0] }

       expected: [0]
            got: [nil]

When I wrote the patch #513, I thought there might be three kinds of coverage values for a file.
Give a file having 4 physical lines, in a resultset there may be a entry for the file:

  1. [0, 1, 2, nil] -- loaded; Integers for executable lines and nils for non-executable lines
  2. [0, 0, 0, 0] -- not loaded but tracked by simplecov; every line is marked as 0
  3. nil -- not loaded nor tracked

So I have assumed that if a file appears in two resultsets, there two coverage values are both Arrays and have the same length. In the test case above, this assumption does not hold ([] and [0] have different lengths).

If the assumption is not correct, patching as in this p-r is necessary.

@michelgrootjans
Copy link
Author

Your assumption is correct. Both Arrays should have the same length in this version of master. I my fix one of both could be empty. In short, my PR has just become a refactor of the previous fix.
See comparison here: https://github.com/colszowka/simplecov/compare/master...michelgrootjans:master?diff=split&name=master#diff-75e6f92edee607d158835c2598d6a2c0.

Historical explanation:
When I wrote this PR, I wasn't aware of issue #513 and its fix, as we were using simplecov 0.11 with this implementation. I wrote a few specs to zoom in on the problem, and fixed it at the level that seemed appropriate.

@PragTob
Copy link
Collaborator

PragTob commented Jan 26, 2017

Hey there thanks for the PR!

@michelgrootjans do I understand your comment correctly that this PR isn't needed anymore?

Otherwise if I can dig up some time I'll investigate the initial case again.

@michelgrootjans
Copy link
Author

Indeed, I'll close this PR

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