Introduce Rspec/DescribeMethodFormat Cop

[filakhtov]

Rspec/DescribeMethodFormat

Ensure that describe method name starts from hash for instance methods or dot for class methods.
This Cop is disabled by default, so that backwards compatibility is preserved.
This Cop ignores root level describe blocks and blocks that use constants.
Following: http://www.betterspecs.org/#describe

Bad:

describe MyBadClass do
  describe 'method_name' do
    # ...
  end
end

Good:

describe MyGoodClass do
  describe '#instance_method_name' do
    # ...
  end

  describe '.class_method_name' do
    # ...
  end
end

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.
• Added an entry to the changelog 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 documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.

[mikegee]

Seems legit 👍

I'm curious whether you think this cop should complain or accept "sentences" as description strings?

describe SomeClass do
  describe 'the happy path' do
    # ...
  end
  describe 'the way it works when things go wrong' do
    # ...
  end
end

Thanks!

[Darhazer]

This is already covered by RSpec/DescribeMethod

it turns out DescribeMethod only covers the second argument of a describe

[Darhazer]

👍

[dgollahon]

@Darhazer, re:

This is already covered by RSpec/DescribeMethod

it turns out DescribeMethod only covers the second argument of a describe

I think this cop should just be a configuration option for RSpec/DescribeMethod. If you look at the proposed name for this cop v.s. the old one, there's no obvious reason one is named one v.s. the other and i think that's a strong hint that these shouldn't be separate. We're doing effectively the same work.

In favor of having a mechanism to enforce this though 👍

[filakhtov]

Thanks a lot for your feedback, everyone! I was operating under the SRP principle, but if you see that it fits into Rspec/DescribeMethod I will make appropriate refactoring and provide an option for it!

[dgollahon]

@Darhazer / @bquorning - Do you agree with my reasoning above? I think two cops would result in a worse / more confusing user experience, but if you think differently I'd be interested to hear what you think and why.

[Darhazer]

@dgollahon I agree. I even thought that’s what DescribeMethod is handling

[bquorning]

What about the case that @mikegee asks about above, where describe is called with one argument, a string that is a sentence. I would estimate that is how I call describe 30% of the time.

I think merging the two cops makes sense, but it would be nice to still pass a sentence to describe.

[filakhtov]

@bquorning @mikegee What if I add two options instead:

• One for enabling this cop on top of DescribeMethod
• Another for allowing sentences

My specific use case is to disallow pretty much anything except describe on methods and constants, so I'm happy as long as there is a way to turn sentences mode off.

[dgollahon]

What about the case that @mikegee asks about above, where describe is called with one argument, a string that is a sentence. I would estimate that is how I call describe 30% of the time.

Personally, I very rarely use describe this way. I almost always use it to describe classes, modules, and method selectors, since that is the thing i consider my test to be describing. I use context when I want a more sentence-like construct. The only exception to this for me are in end-to-end tests where I might describe a route, for instance. In that case, I usually disable this cop for the directory.

@bquorning Just out of curiosity, do you have any examples of how you would use describe in the way you've mentioned? I think in @mikegee's case, i'd be inclined to do something more like "when in the happy path", "when [something] is wrong" with context...

@bquorning @mikegee What if I add two options instead:

• One for enabling this cop on top of DescribeMethod
• Another for allowing sentences

How would another option for allowing sentences be different from disabling the cop entirely?

[mikegee]

Truthfully, I'd use context over describe for those "sentences" I wrote. I was mostly asking for clarification because the specs didn't have examples like that.

[filakhtov]

@dgollahon as earlier mentioned my use-case involves only strict describe messages for method names too. I was looking at allowing sentences case purely because people were asking about it. I'm using context for descriptions and find tests more readable and expressive this way.

How would another option for allowing sentences be different from disabling the cop entirely?

My thinking on sentences would purely be something simple: if there is just one word - make sure it is method name prefixed with # or ., otherwise - skip.

I'm equally agree with e2e tests where you can't apply same set of rules due to their different nature.

[bquorning]

Personally, I very rarely use describe this way. I almost always use it to describe classes, modules, and method selectors, since that is the thing i consider my test to be describing. I use context when I want a more sentence-like construct. The only exception to this for me are in end-to-end tests where I might describe a route, for instance. In that case, I usually disable this cop for the directory.

That sounds like a good rule of thumb, but I would be hesitant to enforce that as a rule for all users of rubocop-rspec. I don’t think that’s how most people use describe.

My thinking on sentences would purely be something simple: if there is just one word - make sure it is method name prefixed with # or ., otherwise - skip.

That sounds reasonable, but I think there will be many false positives.

[filakhtov]

That sounds like a good rule of thumb, but I would be hesitant to enforce that as a rule for all users of rubocop-rspec. I don’t think that’s how most people use describe.

What is the problem of providing this option disabled by default and allow people to enable it?
Or other way around - enable by default and let people who doesn't care disable it?

[dgollahon]

That sounds like a good rule of thumb, but I would be hesitant to enforce that as a rule for all users of rubocop-rspec. I don’t think that’s how most people use describe.

Yeah, I'm sure it's not uniform for most RSpec users, though helping make things uniform is kind of the point of rubocop-rspec, right?

What is the problem of providing this option disabled by default and allow people to enable it?

I personally don't see any. What about extending the existing logic on the top level describe to all describes (if you have a constant followed by a string, that string should be a method selector) for consistency's sake, but then add an option (e.g. require_method_names) that checks inner describes where the first argument is a string and enforces that the strings are method selectors, but then leave that option off by default. I think the sentence case should just be handled by leaving the option off, rather than trying to special case it. @bquorning thoughts?

Or other way around - enable by default and let people who doesn't care disable it?

If @bquorning is right and most people use describe differently, this makes upgrading rubocop-rspec seem overly work-intensive and might turn some people off from using it (even though it'd be perfectly reasonable for them to disable the cop). I think it's tricky to strike a balance between making rubocop-rspec seem helpful v.s. arduous. And this might be one of those cases where it's better to be off by default, though I'm honestly not positive.

[bquorning]

We had @JonRowe from the RSpec team chime in on #226 (comment) and #228, warning that “rubocop tends to give the impression that something is "recommended best practise"”.

Can we link to anyone (blog post, documentation, repositories) besides ourselves (me included, 70% of the time) saying that the suggested style is a “recommended best practise”? Even the RSpec repositories frequently use describe with a sentence:

• https://github.com/rspec/rspec-expectations/blob/20788e1919123058a678b93c1d7062e669e698f0/spec/rspec/matchers/composable_spec.rb#L66
• https://github.com/rspec/rspec-core/blob/17485dea7830b4b8f52f414e043f5743766977a8/spec/rspec/core/backtrace_formatter_spec.rb#L164
• https://github.com/rspec/rspec-mocks/blob/master/spec/rspec/mocks/stub_implementation_spec.rb#L29

[filakhtov]

@bquorning, see http://www.betterspecs.org/ and betterspecs/betterspecs#2

[JonRowe]

BetterSpecs is not endorsed by the RSpec team. They do several things we think of as anti-patterns and have historically not been interested in any input. My 2¢ here is that this:

# unit specs
RSpec.describe "SomeClass" do
  describe "#method"
  describe ".method"
end

Is the best way to describe a class and its methods as unit specs, the # and . trigger some RSpec niceties in formatting. The docstring class name here is deliberate, as it means the spec can be loaded without the Class being loaded, this is important especially in Rails apps, as it means you can delay expensive load times until you are sure the spec is going to run.

[filakhtov]

@JonRowe, IMO if it works for them then they "don't have to" accept input. Also, if they have several things that are considered anti-pattern not necessarily mean they can't have something valuable we can use. What do you say?

I'm just starting with Ruby, so I'm not aware of all lower level details and so if describe as a string for top level class node is preferred I'm totally supportive of that. What you say I think, you are in favour of having method-based describes, am I correct?

[JonRowe]

@filakhtov If you're promoting a resource for writing better anything of course you have to accept input, as there is no golden standard but a process for improving what we do, things change :) When was the last time anyone accepted "works for me" as an acceptable metric for production code :p

Method based describes are useful for unit level specs, I wouldn't recommend them for integration level. Again theres no one best practise here, its a process for figuring out what describes what you want to test best.

[bquorning]

Three quotes from betterspecs/betterspecs#2:

For me this notation is too technical. RSpec was created to express specs/tests using human language.

If your describe is ONLY a method name, like describe ".admin?" then you're not longer describing behaviors, you're now writing unit tests.

I fundamentally disagree with describing methods -- I write specs to describe behavior.

So, from the betterspecs discussion it doesn’t sound like there’s 100% agreement. I would say that describe "Foo.bar"/describe "Foo"; describe ".bar"/describe Foo; describe ".bar" is definitely a neat convention for unit tests/specs, and I can support us having a cop for that. But we shouldn’t enforce it for everyone/everything. The problem is figuring out when and where the “unit test” style should be enforced, and when/where it shouldn’t.

[filakhtov]

@bquorning in my code provided it is disabled by default. And it was this way from the very beginning, because I know that people do all sorts of things to describe and it is up to them to decide if they're happy to opt-in for this behavior.

@JonRowe I have mixed feelings about this and it gets a bit philosophical at this stage, because it is "better" than no standard or recommendation at all and they don't claim it to be the "best". Back to the topic however, how would one tell Rubocop that certain spec file is a unit and therefore this rule should be applied vs this spec is an integration test? Is there even a way? I would think it will just be exclude pattern in configuration file, but again, my knowledge of Ruby/RSpec here isn't up to the scratch, so I'm open for suggestions.