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 merging of resultset #506

Closed
wants to merge 2 commits into from
Closed

Conversation

hanazuki
Copy link
Contributor

In merging two resultsets, if a file only appears in only one of the resultsets, the current merger implementation generates incorrect resultset.

  • Merging {file => [nil, 0, 1]} and {} results in {file => [nil, 0, 1]} (ok)
  • Merging {} and {file => [nil, 0, 1]} results in {file => [nil, nil, 1]} (0 is lost)

This patch fixes the problem.

Proper merging of a file which is only available on one of the two merged resultset
@bf4
Copy link
Collaborator

bf4 commented Jun 17, 2016

possibly related #501 (comment)

@ayufan
Copy link

ayufan commented Jun 30, 2016

Actually there's one more issue with merging.

It's quite common that you have:

{file => [1, nil, nil]} to merge with {file => [0, 0, 0]} results in {file => [1, 0, 0]} which is incorrect.

SimpleCov for file that is only loaded, but not executed prefills coverage with zero's. The nil has a special meaning here: this line were processed, but ignored. This results in badly calculated coverage report when you have specs that touches different files and their result is then merged. The ignored lines are marked as not covered, where they should be ignored.

Given above. Proper result for this is: {file => [1, nil, nil]}.

@hanazuki
Copy link
Contributor Author

hanazuki commented Jul 1, 2016

@ayufan
That case is fixed in #441 (not yet released) and now it seems working correctly.

% git rev-parse @
a80baea9eb97772eeeec339d379887368969c91c
% bundle exec irb -rsimplecov
irb(main):001:0> [1,nil,nil].extend(SimpleCov::ArrayMergeHelper).merge_resultset([0,0,0])
=> [1, nil, nil] (size=3)
irb(main):002:0> [0,0,0].extend(SimpleCov::ArrayMergeHelper).merge_resultset([1,nil,nil])
=> [1, nil, nil] (size=3)

@ayufan
Copy link

ayufan commented Jul 1, 2016

Awesome!

@sferik
Copy link
Collaborator

sferik commented Jul 2, 2016

Working on a new release now!

@thedrow
Copy link

thedrow commented Jul 12, 2016

@hanazuki @sferik We should close this one then right?

@sferik
Copy link
Collaborator

sferik commented Jul 12, 2016

@thedrow Yes, I believe so. Please reopen it if this issue reappears.

@sferik sferik closed this Jul 12, 2016
@hanazuki
Copy link
Contributor Author

@thedrow @sferik
No, this problem is not fixed yet.
Actually this one is a regression of #441, which intoroduces nil-awareness to the merger procedure.

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.

5 participants