Add an else condition to correct StubbedMock behavior

[corsonknowles]

Fix edge case for StubbedMock where it could return a blank suggestion for unhandled Expectation types.

Completes branch coverage for this file.

---

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.

[corsonknowles]

Hmm, I think this spec passes both before and after the change, so there is some issue here:

  it 'flags `are_expected`' do
    expect_offense(<<~RUBY)
      are_expected.to receive(:bar).and_return(:baz)
      ^^^^^^^^^^^^ Prefer `` over `are_expected` when configuring a response.
    RUBY
  end

[corsonknowles]

Okay, I pushed up a real change -- now it only flags on the methods it can correct.

Thoughts?

[pirj]

RSpec 2 legacy, deprecated a decade ago and removed in next major RSpec. We don’t want to support this.

[pirj]

Is this from rspec-its? It’s soft-maintained, and is unlikely can be named an integral part of RSpec. I’d rather documented how to add it to the co figuration if the cop was configurable.

[corsonknowles]

Yes: https://github.com/search?q=org%3Arspec%20are_expected&type=code

My understanding is that it is included in the Expectations.all that the current Cop is using.

[pirj]

I would prefer a case.
Why not?

[bquorning]

SimpleCov pointed out that we don’t have any test coverage for when method_name didn’t match any of the options. And correctly so.

I also prefer the case statement, but perhaps we should add an else raise ArgumentError or KeyError or similar, and add test coverage (where relevant).

[pirj]

Ha! We can actually configure expectations DSL, and add eg the mentioned are_expected, for which this cop won’t find a substitute.

But there is no substitute for eg are_expected. Can we skip correction in this case instead of throwing an argument error?

fetch would raise, and this is also an undesirable outcome, and a breaking change.

I was never a big fan of 100% coverage, but it really starts to manifest its benefits here!

[pirj]

Correcting myself. Dsl conf link. are_expected, and should_* are included.

Quick suggestion: keep the case, but limit matching to just the three options we have substitutes for. Wdyt?

[bquorning]

Quick suggestion: keep the case, but limit matching to just the three options we have substitutes for. Wdyt?

That is what we had before, no? Should we add an else case, and how should it respond?

[pirj]

Previously I think it was returning nil, and we auto-corrected to nonsense. Now, I think, it should just raise an offence and don’t autocorrect

[corsonknowles]

Now, I think, it should just raise an offence and don’t autocorrect

I'm not exactly sure how to accomplish this.

Would you like me to close this PR so you can take it a new direction?

Or I'm happy to continue helping out here if you are willing to coach me through it.

[pirj]

My bad - this cop does not autocorrect 🙈

Since there’s no valid suggestion for the message:

1. add a new MSG_NO_REPLACEMENT with a different message
2. use it for anything that doesn’t have a built-in counterpart

I insist on keeping the case statement, as ‘fetch’ may conceal execution paths from our coverage detection

[corsonknowles]

Okay, how's this approach?

[pirj]

Perfect, thank you, @corsonknowles !