Fix an error RSpec/ChangeByZero cop when without expect block

[lee266]

issue number: #2069

Replace this text with a summary of the changes in your PR. The more detailed you are, the better.

---

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.
  • No changes needed
• 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).

[pirj]

I would very much rather this cop to raise an exception than to swallow such a spec.

[ydah]

Indeed, that example might be unnecessary. However, it makes sense that code like change(foo, :bar).by(0) doesn't produce an error, so perhaps we could simply remove it from the test examples.

[ydah]

Could you rebase to the latest master branch?

[ydah]

Thank you. Could you squashed related commits together?

[ydah]

Thank you❤️

[ydah]

@pirj If your concerns have now been resolved, merge this PR.

[pirj]

Again. I would rather have this current error and let users know about such a mistake in their specs than to swallow it.

Is there another cop that would catch this?

Presently, this cop’s deficiency alerts users in a way, and prevents them from writing specs with false negatives.

I’m not convinced that fixing this error is an improvement when there’s no other cop to catch this.

I clearly understand that this only “detects” such cases with the “change” matcher, and others are left out.

[lee266]

@pirj
The RSpec/NoExpectationExample cop is designed to detect examples that do not contain proper expectations. In fact, it already flags cases like the following:

# spec/sample_spec.rb:13:3: C: RSpec/NoExpectationExample: No expectation found in this example.
  it { change(numbers, :size).by(0) }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Given this, wouldn't it be more appropriate to let RSpec/NoExpectationExample handle such cases rather than adding this handling to RSpec/ChangeByZero?
Since RSpec/NoExpectationExample is already responsible for detecting examples that lack assertions, it seems more natural and preferable from a responsibility separation perspective to rely on it for these cases.

I would appreciate your thoughts on this.

[ydah]

I created RSpec/ChangeWithoutExpect to try it out

• Add new RSpec/ChangeWithoutExpect cop #2074

[pirj]

@lee266 This is reasonable. I apologise to ask this, can’t check this as I’m away from my computer. Does it also detect the case with two statements?

it { subject; change(numbers, :size).by(0) }

[lee266]

@pirj
Yes, it does.

spec/sample_spec.rb:10:3: C: RSpec/NoExpectationExample: No expectation found in this example.
  it { subject; change(numbers, :size).by(0) }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[pirj]

Sorry for the confusion, and thank you for the fix! 🙌

[lee266]

@pirj Also, thank you for sharing your thoughts — I appreciate your input.
@ydah. Thank you for the quick follow-up. I appreciate it!