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 coverage rate of the parallel_tests #441

Merged

Conversation

sinsoku
Copy link
Contributor

@sinsoku sinsoku commented Dec 16, 2015

The coverage rate has decreased when used with parallel_tests. So, I fixed it.

merge_resultset

This Pull Request is to change behaviors of 2 and 4.

no element new_array[i] before after
1 nil nil nil nil
2 nil 0 0 nil
3 nil 1 1 1
4 0 nil 0 nil
5 0 0 0 0
6 0 1 1 1
7 1 nil 1 1
8 1 0 1 1
9 1 1 2 2

Environment

  • Ruby 2.2.3
  • Rails 4.2.5
  • simplecov 0.11.1
  • parallel_tests 1.7.0

@xaviershay
Copy link
Collaborator

To be clear: you intend for this to be just adding specs + refactoring, not changing any behaviour?

@sinsoku
Copy link
Contributor Author

sinsoku commented Dec 17, 2015

No, this Pull Request is changing the behavior a little bit.
Please check the description of Pull Request, because I updated it.

The specification of Coverage module are as below.

  • nil => disabled
  • 0 => uncovered
  • 1 => covered

And the coverage of unloaded files are [0] on simplecov v0.11.0.
This is a problem when run with parallel_tests, to merges coverages generated by a process.

This Pull Request is intended to solve it.

See also:
https://github.com/colszowka/simplecov/blob/v0.11.0/lib/simplecov.rb#L61
https://github.com/ruby/ruby/blob/v2_2_3/ext/coverage/coverage.c

@xaviershay
Copy link
Collaborator

ah, so I suspect this is a "dup" of #436 ?

I don't have a preference as to which is a better solution - only reason I haven't merged the other one is that I was waiting for someone to confirm it actually fixed the issue :)

@sinsoku
Copy link
Contributor Author

sinsoku commented Dec 19, 2015

#436 is insufficient to fix this problem. (#436 (comment))
Moreover, #436 is including extensive refactoring, so I could not create a patch for it.

@ujh
Copy link

ujh commented Feb 24, 2016

It would be great if this could get merged. When I run simplecov as part of a normal rspec run I get around 80% test coverage, but for a while I'm down do around 48% when running as part of parallel_tests. With this branch I'm now back to 80%.

@xaviershay
Copy link
Collaborator

That's the confirmation I was looking for :)

@ujh
Copy link

ujh commented Feb 24, 2016

That's why I posted it. 😁

@rdunlop
Copy link

rdunlop commented Mar 11, 2016

I just used this change on my project, and it fixed our issue of multiple runs being merged incorrectly, leading to lower-than-actual results. It would be really nice to get this released in a new version of the gem.

@sbleon
Copy link

sbleon commented Apr 6, 2016

Just added Simplecov to a Rails 4.2 project. I was disturbed to see a much-lower-than-expected coverage percentage, then I figure out that all of my lines of comments were being reported as "missed". Using this branch fixes the problem. Please merge and release it!

new_array[i] = local_value + other_value
end
pair = [element, new_array[i]]
new_array[i] = if pair.any?(&:nil?) && pair.map(&:to_i).all?(&:zero?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

so, the change is

- result is `nil` if the element in both sets is nil
+ result is `nil` if either set contains nil AND both are each one of either nil, zero, or some non-integer?

and then the merge result is (LHS || 0) + (RHS || 0)

as the test shows

result1      = [nil,   0, nil, 0]
result2      = [nil, nil,   0, 0]
merge_result = [nil, nil, nil, 0]

@bf4
Copy link
Collaborator

bf4 commented Apr 8, 2016

@sinsoku can you rebase and force push your commits, or @colszowka wanna change the merge options? :)

@bf4 bf4 added the Bug label Apr 8, 2016
@sinsoku sinsoku force-pushed the fix_coverage_rate_of_the_parallel_tests branch from a45f7d3 to 54e60b7 Compare April 10, 2016 01:40
@sinsoku
Copy link
Contributor Author

sinsoku commented Apr 10, 2016

@bf4 Thank you for the feedback. I rebased and force pushed.

@sbleon
Copy link

sbleon commented Apr 11, 2016

I just wanted to mention that I have the "incorrectly low coverage" problem but I am not using parallel_tests. Again, this branch fixes the problem for me, so I'd love to see it merged and released soon. Thanks!

@bf4 bf4 merged commit c66eaa1 into simplecov-ruby:master Apr 11, 2016
@bf4
Copy link
Collaborator

bf4 commented Apr 11, 2016

@sinsoku @sbleon merged

@sbleon
Copy link

sbleon commented Apr 11, 2016

Thanks!

On Mon, Apr 11, 2016 at 1:56 PM, Benjamin Fleischer <
notifications@github.com> wrote:

@sinsoku https://github.com/sinsoku @sbleon https://github.com/sbleon
merged


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#441 (comment)

@grosser
Copy link
Contributor

grosser commented Apr 12, 2016

release ? :)

ajm188 pushed a commit to hacsoc/the_jolly_advisor that referenced this pull request May 13, 2016
PR is simplecov-ruby/simplecov#441
undo this change when a new version of simplecov is released
@hanazuki hanazuki mentioned this pull request Jul 1, 2016
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 9, 2016
0.12.0 2016-07-02 ([changes](simplecov-ruby/simplecov@v0.11.2...v0.12.0))
=================

## Enhancements

* Add support for JSON versions 2.x

## Bugfixes

* Fix coverage rate of the parallel_tests. See [#441](simplecov-ruby/simplecov#441) (thanks @sinsoku)
* Fix a regression on old rubies that failed to work with the recently introduced frozen VERSION string. See [#461](simplecov-ruby/simplecov#461) (thanks @leafle)
@sinsoku sinsoku deleted the fix_coverage_rate_of_the_parallel_tests branch May 20, 2023 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants