Add CodeCov badge and minimum coverages #59
Conversation
Codecov Report
@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 97.92% 98.06% +0.13%
==========================================
Files 1 2 +1
Lines 338 413 +75
==========================================
+ Hits 331 405 +74
- Misses 7 8 +1
Continue to review full report at Codecov.
|
jemmaissroff
force-pushed
the
codecov-badge-minimum
branch
3 times, most recently
from
f24e52d
to
ab420e8
Compare
| private :private_memowise_method | ||
| memo_wise :private_memowise_method |
There was a problem hiding this comment.
The ordering here meant we were making the methods private or protected after memoizing them. We need to change their visibility before memoizing them so that our memoization can preserve visibility.
I have added a note to describe this in the README in Issue #37
lib/memo_wise.rb
Outdated
| @@ -190,7 +190,7 @@ def #{method_name}#{args_str} | |||
| end | |||
|
|
|||
| module_eval <<-END_OF_VISIBILITY, __FILE__, __LINE__ + 1 | |||
| "#{method_visibility} :#{method_name}" | |||
| #{method_visibility} :#{method_name} | |||
There was a problem hiding this comment.
We didn't need the "" here because the END_OF_VISIBILITY tags are wrapping this in a string
There was a problem hiding this comment.
Happy to keep this as-is, but would it make sense to just use a regular string instead of a heredoc here?
| @@ -1,5 +1,30 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # Simplecov needs to be loaded before we require `memo_wise` in order to | |||
| # properly track all memo_wise files | |||
| if Gem.loaded_specs.key?("codecov") | |||
There was a problem hiding this comment.
Why remove the ENV[CI] == "true" check here? This will make it run locally when we run specs?
There was a problem hiding this comment.
I actually found it useful to run locally, so moved the ENV["CI"] check below to determine the output. When running locally, it now outputs to the coverage directory so we can open coverage/index.html and clearly view the results. I found this helpful for local dev, but let me know if you're thinking differently!
|
|
||
| # SimpleCov.refuse_coverage_drop is only implemented for line coverage, so for | ||
| # branch coverage we must use `minimum_coverage` | ||
| SimpleCov.minimum_coverage branch: 90 |
There was a problem hiding this comment.
Out of curiosity, why 90? What's our branch coverage at now?
There was a problem hiding this comment.
It's at ~93. I figured we could work to improve it and then move this up! The 90 was just arbitrarily lower than what we're currently at. What are you thinking here?
There was a problem hiding this comment.
I'd love to see this at 100, so whatever you think is the best way to get there works for me!
There was a problem hiding this comment.
Yup let's keep at 90 in this PR, and when it's fixed we'll change it to 100. Added this to Issue #56.
lib/memo_wise.rb
Outdated
| module_eval <<-END_OF_VISIBILITY, __FILE__, __LINE__ + 1 | ||
| "#{method_visibility} :#{method_name}" | ||
| END_OF_VISIBILITY | ||
| module_eval "#{method_visibility} :#{method_name}" |
There was a problem hiding this comment.
Does RuboCop not still want , __FILE__, __LINE__ + 1?
jemmaissroff
force-pushed
the
codecov-badge-minimum
branch
from
4f3a18a
to
bf6a254
Compare
jemmaissroff
force-pushed
the
codecov-badge-minimum
branch
from
bf6a254
to
7eea312
Compare
This PR:
lib/memo_wise.rbto the files evaluated by CodeCovResolves Issue #59