Recommend preferring helper methods to hooks

[pirj]

Don't overuse hooks, prefer explicitness.
Helper methods are more explicit and even provide more flexibility

An example off the top of my head:

# bad
let(:article) { Article.new }

before do
  article.author = author
end

context 'with an author' do
  let(:author) { Author.new('John') }

  it "returns article's author name" do
    expect(article.author_name).to eq 'John'
  end
end

context 'without an author' do
  let(:author) { nil }

  it "returns a placeholder" do
    expect(article.author_name).to eq 'Unknown'
  end
end

# good
def article_for_author(author)
  article = Article.new
  article.author = author
  article
end

context 'with an author' do
  let(:author) { Author.new('John') }

  it "returns article's author name" do
    article = article_for_author(author)
    expect(article.author_name).to eq 'John'
  end
end

context 'without an author' do
  it "returns a placeholder" do
    article = article_for_author(nil)
    expect(article.author_name).to eq 'Unknown'
  end
end

Just like with #56, this should be used when there's a compelling benefit for extraction - improved readability or code reuse.

[sshaw]

Both of these are not ideal.

Given the provided examples, def article_for_author(author) goes against #56. Maybe if you had 2+ tests that needed an article for author then article_for_author may make sense but I don't think this example is good as it's not clear what we're even testing. Seems to be Article#==.

[sshaw]

If it's not clear what we're testing, then let, article_for_author and it {} and context only cause more confusion.

[pirj]

As we've discussed previously, let are overused/often abused. Not sure if parts of the test code being extracted to a plain method fall into the same category, and is not a compelling benefit.

No doubt that it doesn't make much sense to extract something if it is not reused. I imagine something like:

it "returns article's author name" do
  author = Author.new('John')
  article = Article.new
  article.author = author

  expect(article.author_name).to eq 'John'
end

Agree that original examples are confusing, thanks for bringing this up.

PS Fixed. Please take another look.

[dgollahon]

I'm not sure how strong the motivating case is. I think switching from a let to a helper method makes sense when you start getting more complex setup dependencies (ex: you want to make author nil but some other setup tries to call author.something, so you really can't change them independently).

In this case (assuming this is forced by something like ActiveRecord) i think the assignment after mutation is part of what's awkward. If you had an article that was initialized like Article.new(author) then leting an author would seem extremely natural to me and a helper method wouldn't seem necessary. If I have to work with a setter API, I might even embed it in my let if I can:

let(:article) do
  Article.new.tap do |article|
    article.author = author
  end
end

or just inline it entirely if it's not many times.

I think helper methods are good sometimes, but in this case it feels equally overkill-ish to me as the let version.

As a note, the let'ed author in the example seems a bit distracting to me because it doesn't make sense with #56 which might be what @sshaw was talking about, or a different instance.

[pirj]

This recommendation is taken from an Effective Testing with RSpec book.
I did not take the example from it, however, it is using the same argumentation, the difference that in the book there are two properties and two setters are called in a before block.

I tend to agree that "recommend preferring helper methods to hooks" is way too strong compared to something like:

When the amount of code in a hook is getting out of control, consider extracting logical parts of it into helper methods.

and

Consider using helper methods instead of heavily inter-dependent let definitions when it simplifies the code.

WDYT about those two?

[dgollahon]

Sure, I agree with those statements.

I think a realistic motivating example is probably pretty hard to construct since this is more for non-trivial tests, but I agree with and follow that general advice in various real-world situations.

[mockdeep]

I commented over in #56, but I would actually advocate for more strongly saying that before, let, and friends should be avoided. Saying "use when it makes sense" adds some subtlety that I think hurts more than helps. One of the themes that keeps coming up in my own work is "consistency". Consistency is, to me, one of the most important factors to maintainable code. It means that the code is easier to interpret at a glance without needing to spend extra ticks deciphering different patterns. It means that when code is different, there is something meaningful about that difference. And it means that when I'm writing code, I don't need to spend extra time deciding which pattern to follow, nor justifying which pattern I chose and why with my teammates. This all to the point where I will often choose to stick with a less than perfect pattern until I can commit to transitioning all of our code over to a new one.

In this case, allowing for both patterns brings up several questions while I'm updating tests that I have to spend time considering: Is it time to refactor this to the other pattern? Do I have the time to do it now? Should I add it as part of this change or make a separate PR? And when I do refactor it, it likely leads to a sweeping refactor across the tests where I have to now add references to the new helper functions. And that's all if I even remember to think about it. I'd rather a slightly more verbose format in all cases in order to make tests consistent, and easy to understand and modify individually.

[pirj]

I completely agree about your point of consistency in the scope of a given project.
There's this note in the Ruby style guide:

There are some areas in which there is no clear consensus in the Ruby community regarding a particular style (like string literal quoting, spacing inside hash literals, dot position in multi-line method chaining, etc.). In such scenarios, all popular styles are acknowledged and it’s up to you to pick one and apply it consistently.

WDYT of adding a similar note to this guide?

[pirj]

On a different note, there clearly seems to be no single way of doing things.

Ask a hundred developers how to test an application, and you’ll get a hundred different answers.

RSpec Rails provides thoughtfully selected features to encourage good testing practices, but there’s no “right” way to do it. Ultimately, it’s up to you to decide how your test suite will be composed.

-- rspec-rails's README.

I'm more inclined to add some options with soft wording:

When the amount of code in a hook is getting out of control, consider extracting logical parts of it into helper methods.

and

Consider using helper methods instead of heavily inter-dependent let definitions when it simplifies the code.

that implies no pressure and still allows to use let/hooks, or to use helper methods exclusively from the very beginning and avoid let/hooks, depending on the project style.

[mockdeep]

@pirj I think what you've said here makes sense. I'd probably still prefer stronger wording against using let. The implicit assumption is that it makes the code simpler in some situations, though I'd argue we're still better off without it. But I guess maybe the softer approach keeps the guide more in line with current practices while giving people a gentle nudge towards using it less.