Brew: Enable `brew style --rspec` by default

[MikeMcQuaid]

It would be good if we could fix up all the rules that made sense in brew style --rspec and disable those which do not. This would mean we could enable brew style --rspec by default and guide future contributors and maintainers to write better tests.

[aamarill]

Hello,

I am looking to make my first open source contribution. Is this a good issue to take on?
If not, feel free to point me other issues that might be good for someone that's new to open source.
Thanks!

[reitermarkus]

Is this a good issue to take on?

If you have experience with Ruby (and RSpec), go for it. Just run brew style --rspec and try to fix some issues. 😉

[kennethlarsen]

@MikeMcQuaid I'm looking through the output of brew style --rspec and a bit curious to what rules wouldn't make sense to keep? Would be a bit sad to fix 100s of Name your test subject if you need to reference it explicitly if we don't want to keep the rule.

[MikeMcQuaid]

@kennethlarsen You could ask here before you fix a given rule and we can discuss it in this thread? How's that sound? Would be great if you worked on this ❤️

[aamarill]

@kennethlarsen just FYI, I am not working on this. So feel free to take it on if you want/can! 😄

[reitermarkus]

I didn't see any rules which aren't useful. All are meant to make the tests easier to maintain and easier to read, so I don't think we want to disable any for now.

[kennethlarsen]

@MikeMcQuaid Sounds good! I'm currently looking at this rule Start context description with 'when', 'with', or 'without'

Basically, a lot of context descriptions in the codebase start with should as in

        context "should not warn on an newer version revision removal" do
          before do
            formula_gsub_commit "revision 2", ""
            formula_gsub_commit "foo-1.0.tar.gz", "foo-1.1.tar.gz"
          end

          it { is_expected.to be_nil }
        end

Seems like a good idea to follow the suggested convention.

[reitermarkus]

Seems like a good idea to follow the suggested convention.

It's definitely a good idea to start with those which you think are useful.

[MikeMcQuaid]

It's definitely a good idea to start with those which you think are useful.

Agreed 👍

Seems like a good idea to follow the suggested convention.

Agreed.

---

In general I think with both RuboCop RSpec and general RuboCop: other than style preferences or Weird Things We Do That We Have A Good Reason for I think it makes the codebase easier for everyone to grok (particularly new contributors) if we deviate as little from the defaults as possible.

[apjanke]

This would be a good thing to fix up. Note that currently, if you've never run brew style --rspec, you'll get a warning every time you run brew style.

$ brew style
Warning: unrecognized cop Rspec/ExpectActual found in Library/Homebrew/.rubocop.yml
Inspecting 657 files
....................

I think this is because Library/Homebrew/.rubocop.yml is referencing a rubocop-rspec cop:

Rspec/ExpectActual:
  Exclude:
    - 'test/missing_formula_spec.rb'

but it won't be defined if you don't have the rubocop-rspec gem installed and required, and that only happens when the --rspec option is passed in.

UPDATE: Fix for this in #4301.

[apjanke]

Couple questions:

• The RSpec/ExampleLength cop limits examples to just 5 lines. That seems awfully short, especially when configuring test formulae. Maybe we could bump it up to 15 or so?
• RSpec/MultipleExpectations disallows having more than one expectations in a single example. Do we really want to do that? Seems like it's just requiring more boilerplate. (Though it does mean the output messages uniquely identify what failed. Hmm.)
• For most tests in test/cmd and test/dev-cmd, the RSpec/DescribeClass cop complains that "The first argument to describe should be the class or module being tested". But we're using RSpec to test command behavior here, not a class or module per se, and are saying stuff like describe "brew bottle". Seems like that should be allowed as a Weird Thing We Do For Good Reason, at least in test/cmd and test/dev-cmd.

That covers hundreds of the current violations. And a lot of them have named-subject fixes where one change in a file will clear up several violations.

[MikeMcQuaid]

UPDATE: Fix for this in #4301.

Thanks for that!

The RSpec/ExampleLength cop limits examples to just 5 lines. That seems awfully short, especially when configuring test formulae. Maybe we could bump it up to 15 or so?

The intent from when I've been writing Good RSpec is that setting up multiple formulae should be in e.g. before blocks. This encourages sharing between examples so that the example is ideally just running a single method and checking the behaviour/result.

RSpec/MultipleExpectations disallows having more than one expectations in a single example. Do we really want to do that? Seems like it's just requiring more boilerplate. (Though it does mean the output messages uniquely identify what failed. Hmm.)

Yes. The main thing this helps us with is if you have three expectations and the first one failed you'll not be notified whether the latter two passed or failed.

Seems like that should be allowed as a Weird Thing We Do For Good Reason, at least in test/cmd and test/dev-cmd.

Agreed. I think those two directories can be excluded from this cop.

I'd love to see you work on this, even if just for some of the above! Thanks again @apjanke; you're rocking pretty hard right now!

[kennethlarsen]

Heads up: I'm still working on the Start context description with 'when', 'with', or 'without' - I've just been busy.

[apjanke]

Okay, that should keep me busy for a while.

First thing I need is this: #4304. That makes it feasible to grind through the files one at a time, instead of having the test-everything brew style --rspec spam your terminal with violations.

[apjanke]

@kennethlarsen - I'll try not to step on your work.

Maybe instead of working on a particular cop violation, you want to claim an alphabetical range of test files to work on? That'd make it easy to stay out of each other's hair. Here's a sheet I'm using to track my progress - https://docs.google.com/spreadsheets/d/16GtxbiWKi3iisWqqiM-KVkXqslnAjHDVseA36pJO0wA/edit?usp=sharing. If you want to use it, I'll add you as an editor.

[MikeMcQuaid]

I no longer care about this.