Cover and then remove ancestor.send_type? branch in UnspecifiedException

[corsonknowles]

Wraps some non-send type branches around raise_error specs

From:

To:

Then we remove this line, because all specs pass without it due to the node pattern covering both branches.

---

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]

While looking at these PRs to improve our test/code coverage, I have been going a bit back and forth on whether 100% coverage should be a goal in itself. We should at least take care not to add nonsensical test cases, just to achieve better coverage. I think such tests may become a source of confusion for developers fixing another edge case, perhaps 6 months or 2 years from now. I think it’s better to look back through the commit history, find out why a particular piece of code was added, and use that information to inspire a more realistic test case.

On the other hand, having (and enforcing) a high coverage percentage will ensure that no new code is added without having a meaningful test case to complement it. It will be harder to introduce code that is not well explained by its tests. So yeah, let’s continue increasing the coverage toward the 100% 👍🏼

That said, even with your added test case, you could delete next unless ancestor.send_type? from the lib/ code and all specs still pass. I think because the type is also checked by the expect_to? node matcher.

[corsonknowles]

Thanks @bquorning, I'm happy to close this PR if you want to delete that line instead!

[corsonknowles]

Although, it might still be worth documenting this. I.e. that one cannot escape the Cop here by adding layers of nodes inside the .to clause. Do you want to do both?

[bquorning]

Yeah, let's do both. In one PR?

[corsonknowles]

@aarestad Thoughts on this discovery? Are we missing anything here?

I see you added both guard clauses here:

• RSpec/UnspecifiedException: break on non-send blocks to avoid false positives on functions named raise_exception #1960

We now think only the 1st is needed, but let me know if I'm missing anything!

[corsonknowles]

The other thing is, does this give us any reasonable performance benefit?

[bquorning]

I don’t think so. It’s not an optimization we’ve deliberately done elsewhere.