Complete branch coverage for more files

[corsonknowles]

Increase branch coverage minimum threshold.

Modernize .rubocop_todo.yml with Rubocop 1.6.8

In the further pursuit of full coverage for Rubocop/RSpec, this solves for additional files.

Note

We remove a safe operator from a method used in a node matcher here, since it should always get a node.

Reconciled with:

• Improve branch coverage #1989

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

• Set VersionChanged: "<<next>>" in config/default.yml.

[bquorning]

Surprisingly little duplication between this and #1989 :yay:

[corsonknowles]

@bquorning @ydah I reconciled this with #1989 and it should be ready to go!

[corsonknowles]

technically this is all that's needed in here to solve for the case statement branch coverage

[bquorning]

I don’t particularly like this kind of spec. Testing a private method with very edge case input doesn’t provide a whole lot of value. Actually we could change StubbedMock#replacement to do a hash lookup, and SimpleCov would claim 100% coverage:

        def replacement(method_name)
          {
            expect: 'allow',
            is_expected: 'allow(subject)',
            expect_any_instance_of: 'allow_any_instance_of'
          }.fetch(method_name)
        end

I think it would be better look at whether there might actually be a problem with the method_name value which is passed into #replacement. It seems to be found in #on_send by the expectation node matcher, which is looking for #Expectations.all… That method is define in language.rb as returning the language configuration for Expectations, which in default.yml is set to this array of values:

• are_expected
• expect
• expect_any_instance_of
• is_expected
• should
• should_not
• should_not_receive
• should_receive

I’m thinking, could we make test case (using expect_offense or expect_no_offenses) using one of those other values, and thereby hit the code that’s currently not covered by tests?

[corsonknowles]

The Ruby head CI failure comes down to how rocket hashes are spaced in the parser.

[bquorning]

The Ruby head CI failure comes down to how rocket hashes are spaced in the parser.

See

rubocop-rspec/spec/rubocop/rspec/hook_spec.rb

Lines 77 to 83 in 00b6c03

	if RUBY_VERSION >= '3.4'
	let(:expected_special) { 's(:sym, :special) => true' }
	let(:expected_symbol) { 's(:sym, :symbol) => true' }
	else
	let(:expected_special) { 's(:sym, :special)=>true' }
	let(:expected_symbol) { 's(:sym, :symbol)=>true' }
	end

for inspiration on how to work around that.

[bquorning]

This test case is good 👍🏼

I actually found again that the lib/ code can be optimized a bit changes a bit, moving code into the node pattern:

diff --git a/lib/rubocop/cop/rspec/instance_spy.rb b/lib/rubocop/cop/rspec/instance_spy.rb
index cca5a1e6..43298d35 100644
--- a/lib/rubocop/cop/rspec/instance_spy.rb
+++ b/lib/rubocop/cop/rspec/instance_spy.rb
@@ -36,7 +36,7 @@ module RuboCop
         def_node_search :have_received_usage, <<~PATTERN
           (send
             (send nil? :expect
-            (lvar $_)) :to
+            (lvar %1)) :to
             (send nil? :have_received
             ...)
           ...)
@@ -46,9 +46,7 @@ module RuboCop
           return unless example?(node)
 
           null_double(node) do |var, receiver|
-            have_received_usage(node) do |expected|
-              next if expected != var
-
+            have_received_usage(node, var) do
               add_offense(receiver) do |corrector|
                 autocorrect(corrector, receiver)
               end

It would be nice to find a way to calculate test coverage of the node patterns…

[bquorning]

I don’t particularly like this kind of spec. Testing a private method with very edge case input doesn’t provide a whole lot of value. Actually we could change StubbedMock#replacement to do a hash lookup, and SimpleCov would claim 100% coverage:

        def replacement(method_name)
          {
            expect: 'allow',
            is_expected: 'allow(subject)',
            expect_any_instance_of: 'allow_any_instance_of'
          }.fetch(method_name)
        end

I think it would be better look at whether there might actually be a problem with the method_name value which is passed into #replacement. It seems to be found in #on_send by the expectation node matcher, which is looking for #Expectations.all… That method is define in language.rb as returning the language configuration for Expectations, which in default.yml is set to this array of values:

• are_expected
• expect
• expect_any_instance_of
• is_expected
• should
• should_not
• should_not_receive
• should_receive

I’m thinking, could we make test case (using expect_offense or expect_no_offenses) using one of those other values, and thereby hit the code that’s currently not covered by tests?

[bquorning]

First up, I think that since 1fa9ce3 is long merged, we can just remove SUBDEPARTMENTS now.

[bquorning]

And second, it looks like AMENDMENTS is currently a String and not an Array. Which is… confusing, to put it mildly. Aren’t we essentially doing this:

"abcde".gsub(*%(bc), 'x')

🙈

[bquorning]

👍🏼

[bquorning]

Thanks