Complete branch coverage for ContextWording

[corsonknowles]

Completes branch coverage for the ContextWording cop.

See related efforts:

• Complete line coverage for Metadata mixin #1970
• Update .simplecov #1971
• Require VoidExpect to operate inside an example block #1975 (split from this PR)
• Deprecate top_level_group? and test it in a Cop class #1976 (split from PR#1970)

Before:

---

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]

@pirj @ydah Thanks for your help on this one!

I think it's good to go now, just simple descriptive specs with no stubbing for this Cop.

Proof of coverage:

Update: revised to just reflect ContextWording cop specs.

[pirj]

It now feels that the VoidExpect needs a slight rework to be more focused on specs.

I’d love to be able to skip specs for what is not valid RSpec.

[corsonknowles]

@pirj @ydah Should we merge this one? It's now just a very simple spec for the missing branch in ContextWording

[ydah]

@corsonknowles Perhaps this problem can be resolved by the following refactoring:

• Remove unnecessary early return in RSpec/ContextWordingcop #1981

[corsonknowles]

Thanks @ydah, it's a little bit philosophical.

There is a measurement problem we want to solve, which is just that it's much easier to know when new lines are uncovered with 100% coverage. Since that is my primary concern in this recent set of PRs, I'm happy with both solutions.

And then there is another concern for this example, what do we mean when something has a spec? There's an edge case here for when the default patterns have been deliberately removed. What do we want the Cop to do? Currently, it runs and no-ops. The spec in this PR describes that behavior. But maybe we don't want to specify what it does? Maybe we want to allow it to raise? Or have it print a warning? Philosophically, I would suggest it is easier to do that when the behavior is already described. But some might take the position that if we leave it unspecified, it's easier to change. I take the former view, because it's harder to see that an edge case needs handling when it is not described.

If we fix it at the code level, but don't add this spec, we still leave this edge case un-described.

[bquorning]

I like this. Describing edge case behavior in specs makes sure that if we change it, it will be an intentional change (where we need to change the spec as well).

[ydah]

No objection to adding this spec.

[ydah]

memo) In this case, it is pointless to enable this cop because it is not always an offense. Should we occur warning or runtime error for this setting? WDYT? @rubocop/rubocop-rspec

[bquorning]

it is not always an offense

I don’t understand. Do you mean it’s not deterministic?

[ydah]

The following checks are always considered non-offenses:

rubocop-rspec/lib/rubocop/cop/rspec/context_wording.rb

Line 88 in 7592cec

return false if allowed_patterns.empty?

This Cop sets valid wording in Prefixes and AllowedPatterns, but it always dose not occur offenses if there is no valid wording setting. I don't think that's the intended setting, but users have no way of knowing right now. Because it's always dose not occur offenses.

[corsonknowles]

Right, with these settings, there will be no offenses, regardless of what is or is not in the Context. It is always not an offense. (I'm not attached to it staying that way).

I looked at the Obsoletion setup recently, and I think it raises runtime errors with the expectation that the user will want to fix those issues. That feels similar to how a user may want to fix the config or disable this cop instead of letting it run "on empty"

[bquorning]

It’s an unlikely combination of settings, so unless it’s very simple to implement, I don‘t think we need to warn our users about it.

[ydah]

If the Prefixes and AllowedPatterns are empty to begin with, there is no allowed Wording, so it might be better to make them all violations. That way, users would notice the misconfiguration and it would not be too difficult to implement.