Restore 100% test coverage #3086
Conversation
nevinera
force-pushed
the
nev--full-test-coverage
branch
from
23e1771
to
a89e6a1
Compare
|
(Moving back to draft while I figure out what else is covered locally but not in CI) |
nevinera
force-pushed
the
nev--full-test-coverage
branch
3 times, most recently
from
d0cdac0
to
7ef21be
Compare
| end | ||
|
|
||
| it '`should_not` does not print a deprecation warning when given a value' do | ||
| expect_no_deprecations |
There was a problem hiding this comment.
Unrelated to the changes in this pr.
Wondering why we test against should_not, but not ‘is_expected.not_to’. To my best memory this is because ‘should’/‘should_not’ are also defined in rspec-core, and we have this spec for is_expected in rspec-expectations.
Why do we expect an Exception here, and not some more specific exception? Also probably to untangle from rspec-expectations that defines them?
Thise are minor either way.
There was a problem hiding this comment.
I have far less clue than you do, but those sound like good hypotheses? I assumed we were handling Exception here (and not StandardError) because the expectation errors aren't descendants of the latter, but I hadn't given it any thought past that. I'm also guessing that the reason for preferring is_expected over should is that the former can be implemented as a matcher, instead of in rspec-core?
(and doesn't run in current versions of ruby)
And conditionalize the specs that weren't yet to only run if the 'taint' feature is supported (it was removed in 2.7).
It is actually fully covered (I checked manually) by spec/integration/bisect_spec.rb, but the code in question is running in a forked process, so the Coverage instance in the main process doesn't track it.
The gem isn't installed on CI, so we need to make the coverage optional (though it is fully covered, when the specs get to run)
This is a pretty silly spec, but the name method here isn't actually used by anything that I can see - I assume it exists only for symmetry with the ShellRunner, to present a more consistent interface). Happy to just drop the method if that's preferred, and if someone else is confident that it's not relevant to anything.
nevinera
force-pushed
the
nev--full-test-coverage
branch
from
d1dc26e
to
aef40b3
Compare
.github/workflows/ci.yml
Outdated
| @@ -123,6 +131,7 @@ jobs: | |||
| env: | |||
| LEGACY_CI: true | |||
| JRUBY_OPTS: ${{ matrix.container.jruby_opts || '--dev' }} | |||
| NO_COVERAGE=true | |||
There was a problem hiding this comment.
Note: this isn't actually impactful, since none of these were running it anyway. But I thought it would be clearer to supply it anyway - happy to drop it again if you disagree!
nevinera
force-pushed
the
nev--full-test-coverage
branch
3 times, most recently
from
0290c7d
to
caceb39
Compare
This allows us to check coverage on lines that only run on modern rubies, rather than needing to :nocov: them so older rubies can pass the test suite. It also makes it easy to keep track of and change which ruby's coverage is being _used_ in CI, since it's just being specified in an environment variable on the matrix.
nevinera
force-pushed
the
nev--full-test-coverage
branch
from
caceb39
to
1198582
Compare
There was a problem hiding this comment.
My apologies for another round of comments. Now when we have a check for >= 3.1, would it work to run coverage on those versions?
That way we would get rid of a necessity having this coverage_only_on. At a (quite low) cost of running coverage on 3.1 and 3.2. What do you think?
Ah! Yes, it shouldn't be necessary anymore. But that does call to mind the reason I had it as an environment variable in the first place, which was that controlling which rubies we want to enforce coverage against should ideally be specified near the matrix, instead of in the script - I should have used that variable for this purpose instead! One moment. There, that simplifies things a bit. The only remaining awkwardness (that I see) is that if someone is locally running say, 2.7 and runs the script, it'll fail their builds because they don't have 100% coverage. But they can |
|
3.1 coverage seems to be 99.9. I’m ok to enforce on 3.2+ |
Oh, it looks like |
It turns out that this condition: SyntaxError.instance_methods.include?(:detailed_message) is false under ruby-3.1, so we don't have 100% coverage there. Ah well!
|
Thank you! |
|
Thank you! |
This is mostly annotational, but there were a few minor tests missing from however long the coverage-check was busted.
The larger items:
RSpec::Support::RubyFeatures.supports_taint?. They were passing anyway, since they were just checking for "doesn't raise an error", but they weren't actually testing what they said they were testing, when run against anything after ruby-2.6ForkRunner#run_specsmethod as :nocov: - it actually is fully covered, but it only executes in a forked process, when run by theintegration/bisect_spec.rb, and the Coverage run in the main process doesn't hear about it. But I manually checked, and both conditional branches are getting exercised.:nocov:, since the gem isn't installed on CI - none of the code inside this file can be exercised there (though it is covered anywhere it can run)Bisect::ShellRunner.namewhich maybe should just be deleted instead? It's not clear that this method is necessary - if anyone can be confident that it's not, I'd be just as happy to remove it. (But probably in another PR - I like keeping these kinds of things comment/test-only)