Add new RSpec/NegatedExpectation cop

[ydah]

This cop checks for expectations with !.
The autocorrection is marked as unsafe, because it may change the expectation from a positive to a negative one, or vice versa.

# bad
!expect(foo).to be_valid

# good
expect(foo).not_to be_valid

---

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.

[bquorning]

I don’t think this cop reflects a real problem. For the example !expect(foo).to be_valid to fail, somewhere in RSpec’s internals expect(foo).to be_valid (without !) would need to raise the exception that means “this example failed”. Whether there’s a ! in front or not doesn’t matter.

When I run this small spec file, both examples pass.

require "rspec"

RSpec.describe "problem" do
  specify { !expect(1).to be_odd }
  specify {  expect(1).to be_odd }
end

[ydah]

@bquorning One of the real trouble cases I encountered was when there was only a test !expect(foo).to be_valid, and the developer did not notice a test that should have failed in fact. The developer who wrote the test had written the test with the intention of expect(foo).not_to_be_valid.
Maybe it could simply be a cop that points out the redundant !. At any rate, it would be nice to have a cop that indicates that the ! in front of the expectation is meaningless. For example, what if we also change the name of the cop to RedundantNegation? WDYT?

[bquorning]

Hmm, I’m not sure if this is something we should have a cop for. There are probably many other ways to badly write a spec which this cop wouldn’t catch.

I’d be interested to hear what @pirj thinks.

[ydah]

FYI: A recent example I encountered was the capybara cheat sheet:

• Remove a case that is not negative examples in the capybara sheet rstacruz/cheatsheets#2122

[ydah]

memo) For example, a Lint/Void offense will result if expect is followed by do_something:

  it do
    !expect(nil).to be_nil
#   ^ Lint/Void: Operator ! used in void context.
    do_something
  end

However, the following is not an offense:

  it do
    !expect(nil).to be_nil
  end

[pirj]

I could argue with Void, as overridden unary ! can have side effects even if its return value is discarded !rm_rf(root) </>

RSpec supports binary & and | as aliases to and/or like in expect(light.color).to eq("green") | eq("yellow") | eq("red"), so I’m not surprised some may think that negating the whole thing would negate the expectation.

With the first paragraph in mind, won’t it be better adding a pika-yoke on rapec-expectations side as an implementation of def ! in ExpectationTarget emitting a warning that such syntax is unsupported?

[ydah]

I could argue with Void, as overridden unary ! can have side effects even if its return value is discarded !rm_rf(root) </>

RSpec supports binary & and | as aliases to and/or like in expect(light.color).to eq("green") | eq("yellow") | eq("red"), so I’m not surprised some may think that negating the whole thing would negate the expectation.

With the first paragraph in mind, won’t it be better adding a pika-yoke on rapec-expectations side as an implementation of def ! in ExpectationTarget emitting a warning that such syntax is unsupported?

I agree with your opinion. I think it would be better if the poka-yoke is in rspec-expectations instead of rubocop-rspec. Should I close this PR? @bquorning @pirj

[bquorning]

I agree that adding a check to rspec-expectations would be better.