rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
794 stars 272 forks source link

Add new RSpec/EmptyOutput cop #1854

Closed bquorning closed 3 months ago

bquorning commented 3 months ago

Instead of calling output with an empty string, you should use the inverse runner and call output without an argument. E.g. instead of

expect { foo }.to output('').to_stdout

you should call

expect { foo }.not_to output.to_stdout

Before submitting the PR make sure the following are checked:

If you have created a new cop:

bquorning commented 3 months ago

cc @pirj

pirj commented 3 months ago

One exception I can think of is a compund expectation like output(‘’).to_stdout.and output(‘’).to_stderr. We had a cop that suggested using a custom negation matcher for compound expectations. Was that not_change?

Otherwise - looks good, thanks!

bquorning commented 3 months ago

I actually did look into the compound marchers, and I thinkto_stderr and to_stdout both return true so they cannot be chained. I may be wrong of course.

pirj commented 3 months ago

At a glance, it returns self, and the vase class does include Composable that defines and/or.

Not a big deal, just an edge case. But since to_std… is mandatory, it’s not unrealistic.

bquorning commented 3 months ago

I’ll have a look.

Actually what should we recommend if someone matches e.g.

expect {}.to output("foo").to_stdout.and output("").to_stderr

I don’t think there’s a way to do e.g.

expect {}.to output("foo").to_stdout.and not output.to_stderr
# or
expect {}.to_not output.to_stderr.and output("foo").to_stdout

The last code tells me that

expect(...).not_to matcher.and matcher is not supported, since it creates a bit of an ambiguity. Instead, define negated versions of whatever matchers you wish to negate with RSpec::Matchers.define_negated_matcher and use expect(...).to matcher.and matcher.

Maybe in that case it’s fine that we don’t detect an offense on compound expectations.

pirj commented 3 months ago

For the other cop, we register an offence, but don’t autocorrect, since it’s not guaranteed that the negated matcher (not_change) is defined in the user’s codebase.

expect { :nop }
  .to not_output.to_stdout
  .and not_output.to_stderr

should work, just like a combination of output.to_stderr with eg not_output.to_stdout

bquorning commented 3 months ago

Yeah, I think it’s too much if we ask the user to implement a #not_output method when output("") works fine.

bquorning commented 3 months ago

Just to be sure, the all matcher cannot be used together with output, right? E.g. expect([proc_1, proc_2]).to all(output("").to_stderr)

ydah commented 3 months ago

I checked with real-world-rspec. There seems to be no errors or false positives ✅

Offenses:

/ydah/real-world-rspec/rubocop/spec/rubocop/config_loader_spec.rb:1016:74: C: [Correctable] RSpec/EmptyOutput: Use not_to instead of matching on an empty output.
        expect { expect(configuration_from_file.to_h).to eq(config) }.to output('').to_stderr
                                                                         ^^^^^^^^^^
/ydah/real-world-rspec/rubocop/spec/rubocop/config_loader_spec.rb:1213:18: C: [Correctable] RSpec/EmptyOutput: Use not_to instead of matching on an empty output.
          end.to output('').to_stderr
                 ^^^^^^^^^^
/ydah/real-world-rspec/rubocop/spec/rubocop/config_loader_spec.rb:1249:18: C: [Correctable] RSpec/EmptyOutput: Use not_to instead of matching on an empty output.
          end.to output('').to_stderr
                 ^^^^^^^^^^
/ydah/real-world-rspec/rubocop/spec/rubocop/config_loader_spec.rb:1317:47: C: [Correctable] RSpec/EmptyOutput: Use not_to instead of matching on an empty output.
        expect { configuration_from_file }.to output('').to_stderr
                                              ^^^^^^^^^^

22407 files inspected, 4 offenses detected, 4 offenses autocorrectable