Style guide

[JonRowe]

It'd be nice for the RSpec website to include a brief "recommended" style guide, it's something that's been talked about before and occasionally pops up in discussion (today's via twitter). I know betterspecs.org exists but in the past we've disagreed over what constitutes "better" and I think it'd be nice to have a short synopsis of what we think are good practises to follow.

/cc @keithpitty

[keithpitty]

I'm happy to contribute to this because I think there's a need. For example, on Friday the topic of recommended RSpec style came up in a discussion at work. Someone pointed to http://betterspecs.org/, obviously having googled, and I recalled that @JonRowe had, some time ago, told me that the RSpec core team had disagreed with some of what the folks behind http://betterspecs.org/ were recommending.

I agree that the guide should be brief.

[myronmarston]

I definitely want the new site to expand to include more resources (I know @cupakromer has some ideas here) and I can see a style guide being part of that. My primary beef with betterspecs.org (and where I'd want us to do better) is that it presents things in a binary good/bad format without discussion of the tradeoffs involved of one over the other. It also seems to encourage users to use as many RSpec features as possible (with guidelines like "use subject", "use let and let!", "use contexts", "shared examples", etc) and while every RSpec feature has its place, I believe it's generally better to use fewer, simpler features until a situation warrants using the more advanced features.

I also have a bit of a concern that an official style guide could be used to silence discussion ("the rspec team recommends this, end of story, let's move on...") and I don't want that. (If anything, I want it to foster discussion due to discussing the pros/cons and various things).

This is a lower priority to me than the blog (which I have partially working locally, but haven't pushed yet) and the docs (which I think @JonRowe is working on?) which are the things I was hoping to have in place before launching the site in tandem with RSpec 3.2. I expect this to come sometime later.

[pirj]

We've briefly discussed with @JonRowe, and those are some takeaways:

https://rspec.rubystyle.guide/, "community RSpec style guide", "unofficial RSpec style guide" is a good starting point. It absorbed the useful parts of unmaintained betterspecs, and modernized them.
Confession - I maintain this guide, so biased towards it.
At the moment, the guide has reached the state when I have little to none things to add/change that are non-controversial.

Other interesting guides worth mentioning: https://about.lessonly.engineering/styleguide/testing/ and https://github.com/thoughtbot/guides/tree/master/best-practices#testing

The unofficial RSpec style guide strives to be in harmony with rubocop-rspec, but this is nowhere near complete.

I and Jon both agreed with Myron that guidelines should be less insisting and present the choice and implications of the options.
The unofficial style guide strives to provide links to reasoning and discussions (even though it's a time-consuming task yet to be completed), and to not be to insisting, e.g.:

Do not overuse lets for primitive data, find the balance between frequency of use and complexity of the definition.

Context descriptions should describe the conditions shared by all the examples within. Full example names (formed by the concatenation of all nested block descriptions) should form a readable sentence.

For examples two styles are considered acceptable. The first variant is a separate example for each expectation, which comes with a cost of repeated context initialization. The second variant is multiple expectations per example with aggregate_failures tag set for a group or example. Use your best judgement in each case, and apply your strategy consistently.

I see two options to approach the official RSpec style guide:

1. narrow down the guide to the base RSpec style guide which RSpec core team will all agree on, and provide all the rest separately as a community-submitted best common practices not necessarily recommended by the RSpec core team
2. extract the guidelines we agree on to RSpec documentation, and not necessarily keep it in sync with the unofficial style guide

The amendments to the unofficial style guide

Jon proposed:

1. In layout, we always prefer using RSpec. at the top level, the monkey patching will be removed in RSpec 4.

# bad
describe Article

# good
RSpec.describe "Article"

It's a sane default, it will fit the community style guide quite well.

2. With subject, we always prefer a name (our own specs don't do this but we would change them to do so as we encounter them).

# bad
subject { … }

# good
subject(:name) { … }

3. Separate subject them from lets

#bad 
subject(:name) { … }
let(:name) { … }
let(:name) { … }

#good
let(:name) { … }
let(:name) { … }

subject(:name) { … }

4. Always use a docstring

# bad
specify { … }

# good
it "does something" do
end

5. Naming.
   The entire description should make sense in English, but that doesn’t mean you need when/with/without.
   This is pretty much in a good shape

6. Using a before to trigger the subject under test is contentious and not agreed upon.

7. Out of scope for a pure RSpec guide but we don’t recommend FactoryBot as there are a lot of circumstances where using your own fixture generators or just using create directly is more efficient. FactoryBot hides problems with overly complex data structure/collaborators too easily.
   Note We do recommend FactoryBot in rspec-rails https://github.com/rspec/rspec-rails#recommended-third-party-extensions

8. Similarly, we’d recommend handing in times/dates rather than stubbing or using time cop. Timecop is just stubbing by a 3rd party.

9. We don’t really recommend 3rd party expectation libraries like shoulda, but as this guide is unofficial that's ok to mention.

10. Controller specs are deprecated, a guide should mention that!

From me (mostly gathered from RSpec repos' discussions, Effective testing with RSpec 3, and RSpec source code comments):

11. Don't overuse let and hooks Use before and let when it's beneficial rubocop/rspec-style-guide#56
12. Use subject when it makes sense
13. Avoid described_class Confusing described_class rubocop/rspec-style-guide#104
14. Prefer using class names in example group descriptions as a string literal to class literal due to eager loading of classes (resonates with Jon's guideline amendment Fix book cover image and avoid resize #1)
15. Don't use the implicit block style

# bad
subject { -> { perform } }
it { is_expected.to change { User.count } }

The definitive list of things I'd personally love to update in the community guide https://github.com/rubocop-hq/rspec-style-guide/issues

[benoittgt]

Ouhao 🙌🏻. Super happy to see this discussion. I am completely in favor of this.

I love both options but I have a little preference for:

extract the guidelines we agree on to RSpec documentation, and not necessarily keep it in sync with the unofficial style guide

[pirj]

One guideline to add is to avoid stubbing Ruby core classes, as this may result in weird and hard to track down errors. We have Mutex.new, File.read (find references to the code and PRs/issues) safeguarded.