100% test coverage #1458

nevinera · 2024-05-08T20:43:09Z

Well, aside from a few lines in two other PRs (#1457 and #1456) that needed non-spec changes.

First, I found and marked all of the segments that seem to be ruby-version specific, and set them off with :nocov: tags. That's in the first commit. Then I started tackling the remaining uncovered lines one at a time, adding tests I believe there to be no non-spec, non-comment changes in this PR, but I'm confident that some of the specs can be done more elegantly. Please, enhance my approaches, rspec wizards :-D

nevinera · 2024-05-08T20:47:39Z

spec/rspec/matchers/built_in/compound_spec.rb

@@ -973,5 +993,30 @@ def actual
        }.to raise_error(NotImplementedError, /matcher.or matcher` is not supported/)
      end
    end
+
+    describe "#expects_call_stack_jump?" do


This one I was least satisfied with - you've fairly consistently avoided thin unit testing in favor of behavioral/usage-flavored tests. But the tests I rigged up for that here were enormous, and I decided that they were complex enough to get in the way (I needed to make custom ruby-class block-accepting matchers that expected and didn't expect call-stack jumps). But I'll take another swing if I'm not imagining that preference, and you think it's worth the ugly.

nevinera · 2024-05-08T20:58:56Z

spec/rspec/matchers/composable_spec.rb

+        expect {
+          expect(3).to matcher_using_surface_descriptions_in(STDOUT)
+        }.to fail_with(STDOUT.to_s)
+      end


This one was also ugly, because we can't get to the line in question without an instance of IO, and I didn't figure we'd want to open a fixture File for this spec (but if we do, that'd make it nicer).

Would StringIO work?

My first thought as well, but it's actually not a subclass of IO, just duck-typed like it! I assumed so hard I wrote the test and debugged it to find out I was wrong -.-

nevinera · 2024-05-09T17:12:14Z

The oldest rubies are failing two of the new tests:

  1) operator matchers RSpec::Matchers::BuiltIn::NegativeOperatorMatcher complains when negated
     Failure/Error: expect {
       expected /does not support `should_not != .*Use `should ==/ but nothing was raised
     # ./spec/rspec/matchers/built_in/operators_spec.rb:264

  2) operator matchers RSpec::Matchers::BuiltIn::PositiveOperatorMatcher complains when negated
     Failure/Error: expect {
       expected /does not support `should != .*Use `should_not ==/ but nothing was raised
     # ./spec/rspec/matchers/built_in/operators_spec.rb:241

I don't see anything obviously version dependent here, the only thing that seems plausible is this has_non_generic_implementation_of? method, but I don't see how variation there would cause these expectations to skip failing entirely..

I think this might mean those matchers are allowed and broken in 1.8.7? Which would probably be .. well, I doubt anybody on that version expects first-class support, but once I get 1.8.7 working in a container I can probably fix it at least :-)

In the meantime, I guess I should version-limit those tests? If I'm misunderstanding what's going on (which is likely, as I don't feel like I do understand what's going on yet), tell me more legends of how ruby used to be and I will learn again ;-)

pirj

Looks good. Thank you!

nevinera · 2024-05-12T02:46:37Z

@pirj - The other two PRs are merged now, so I rebased this on top of main. That means that it actually has 100% coverage now - would you like me to bump this value up to 100% to keep it there, or leave it at 92% like it is now? I wasn't originally planning on doing so, but I noticed that we were actually trying to enforce a 100% value in rspec-core using such a config, so I thought I'd check.

pirj · 2024-05-12T05:32:48Z

Let’s tighten it up to 100%, we can always loosen up if there’s something that we can’t reasonably cover.

(Because they are specific to versions of ruby, and shouldn't affect overall coverage when run outside those versions)

The message is out of date, since ruby 3 has gone _back_ to ignoring the 'inspect' output (this line was probably covered when it was written, but version-specific, though I didn't bother pinning down exactly what version). I updated the spec structure slightly as well, because the original shape made it difficult to determine what the current message actually _was_, but I'm happy to put it back if there's a style preference.

I.. struggled to get coverage on this through more integration-style behavioral tests like we prefer. It ended up being very verbose and complex, and it felt unreasonable for what it was really testing (which was that, when handling custom matchers that don't specify, we treat them as _not_ expecting call-stack jumps)

This is a pretty odd corner case, and I mocked it, since I didn't think we wanted to open a file or socket in our specs for this.

We don't allow `x.should != y`, and we need to test that we don't.

my prior '.==(VALUE)' solution passed linting on modern rubies, but apparently on older rubies it still caused a message like possibly useless use of == in void context This added expectation doesn't _entirely_ belong, but it's not completely out of place, and it convinces the linter/parser not to worry.

In contexts without RSpec::Mocks::Double defined, we don't need to check this, but of course it's always defined in our _rspec tests._

nevinera · 2024-06-30T02:31:32Z

@JonRowe, this one has been waiting for a while, and ought to be fairly safe as it's test-only. I've updated it with latest if you have a minute to comment; apparently I was the only one to introduce uncovered code since the last push 6 weeks ago >.<

spec/rspec/matchers/built_in/operators_spec.rb

Use skip instead of if

pirj · 2024-08-18T11:20:08Z

Thank you!

100% test coverage

nevinera marked this pull request as ready for review May 8, 2024 20:43

nevinera commented May 8, 2024

View reviewed changes

nevinera force-pushed the nev--improve-test-coverage branch from 8a95966 to d99adb5 Compare May 8, 2024 22:21

nevinera force-pushed the nev--improve-test-coverage branch from 13fa00b to f31af7e Compare May 9, 2024 17:12

nevinera requested a review from pirj May 10, 2024 13:05

pirj approved these changes May 11, 2024

View reviewed changes

nevinera force-pushed the nev--improve-test-coverage branch from a58b458 to c42fc5b Compare May 12, 2024 02:40

pirj requested a review from JonRowe May 12, 2024 12:45

nevinera added 15 commits June 29, 2024 22:06

Mark several regions as :nocov:

fde254e

(Because they are specific to versions of ruby, and shouldn't affect overall coverage when run outside those versions)

Test not-implemented case of count_inclusions

d5a28d4

Add coverage for with_keywords(:x) (single-arity)

219d6c5

Add coverage for supports_value_expectations? failover

977266e

Add at test for a _readable_ IO object

7160e72

This is a pretty odd corner case, and I mocked it, since I didn't think we wanted to open a file or socket in our specs for this.

Add coverage for expect(...).not_to all(...) failure

4e2abe6

Add specs for the inverted truthy/falsey matchers

62ab676

Add tests covering the negative operator errors

c30cb74

We don't allow `x.should != y`, and we need to test that we don't.

Add tests for the descriptions of operator matchers

8424557

Add test covering on_potential_false_positives failure mode

761cfd3

Avoid the inverted negative operator tests before 1.9

3d918a0

enforce 100% coverage now that we have it!

da2288b

nevinera force-pushed the nev--improve-test-coverage branch from 98c6f4a to da2288b Compare June 30, 2024 02:07

Flag the really_responds_to? fallback as nocov

b1ba98f

In contexts without RSpec::Mocks::Double defined, we don't need to check this, but of course it's always defined in our _rspec tests._

pirj approved these changes Aug 18, 2024

View reviewed changes

spec/rspec/matchers/built_in/operators_spec.rb Outdated Show resolved Hide resolved

spec/rspec/matchers/built_in/operators_spec.rb Outdated Show resolved Hide resolved

Apply suggestions from code review

ab8cfa6

Use skip instead of if

pirj merged commit 2b7b4fd into rspec:main Aug 18, 2024
29 of 30 checks passed

nevinera deleted the nev--improve-test-coverage branch August 18, 2024 19:13

nevinera mentioned this pull request Aug 18, 2024

Get test coverage of all code that's not version-specific up to 100% #767

Closed

JonRowe pushed a commit that referenced this pull request Aug 20, 2024

Merge pull request #1458 from nevinera/nev--improve-test-coverage

6b97659

100% test coverage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

100% test coverage #1458

100% test coverage #1458

nevinera commented May 8, 2024

nevinera May 8, 2024

nevinera May 8, 2024

pirj May 9, 2024

nevinera May 9, 2024

nevinera commented May 9, 2024 •

edited

Loading

pirj left a comment

nevinera commented May 12, 2024

pirj commented May 12, 2024

nevinera commented Jun 30, 2024

pirj commented Aug 18, 2024

100% test coverage #1458

100% test coverage #1458

Conversation

nevinera commented May 8, 2024

nevinera May 8, 2024

Choose a reason for hiding this comment

nevinera May 8, 2024

Choose a reason for hiding this comment

pirj May 9, 2024

Choose a reason for hiding this comment

nevinera May 9, 2024

Choose a reason for hiding this comment

nevinera commented May 9, 2024 • edited Loading

pirj left a comment

Choose a reason for hiding this comment

nevinera commented May 12, 2024

pirj commented May 12, 2024

nevinera commented Jun 30, 2024

pirj commented Aug 18, 2024

nevinera commented May 9, 2024 •

edited

Loading