request_feature_spec_subject.md

Request spec subjects

Who TF cares honestly? 🤷‍♂️ 🤷‍♀️

Great question. Let me preface this by saying I HATE talking about RSpec best practices. It's irrelevant to shipping software and tends to churn up a lot of long, useless discussion. However, I think it's useful to consider best practices when there is a demonstrable functional (not stylistic) difference between two approaches that can affect maintainability, flexibility, and speed.

One style I see quite a lot looks a bit like this:

The before hook tower of doom

# top of the spec

before { get bounties_path(code: 'dilbert') }

it 'something happens' do
  expect(response).to have_http_status :ok
end


describe 'derpy derpy derp I wanna make the request fail' do
  before { allow(MyClass).to receive(:your_class).and_raise(OneHorrificError) }

  it 'something happens' do
    # 10 minutes trying to figure out why your mock didn't get activated
    # spoiler -- it ran _after_ the request because before hooks are run in the order they are defined
  end
end

describe 'derpy derpy derp let me go test another route' do
  before { post bounties_path(code: 'dilbert') }

  it 'something happens' do
    # various hard to track down bugs resulting in accidentically 
    # running two requests for a single example (due to the two hooks)
    
    # hey maybe it goes along fine and nobody notices but OOPS ‼ 
    # you just ran 2 requests for every expectation for no reason
    expect(response).to have_http_status :ok
  end
end

before hooks should be used sparingly. The most insidious form of them is when the code inside the hook is very heavy -- say logging in and visiting a page in Capybara (more on that later). I fix quite a lot of intermittent specs in our codebase 💅 💅 💅 and I find that almost every time a clumsy before hook is involved.

Examples:

https://github.com/bugcrowd/crowdcontrol/pull/7685/files https://github.com/bugcrowd/crowdcontrol/pull/8497/files https://github.com/bugcrowd/crowdcontrol/pull/8662/files

Solutions

it { is expected to live only in the example }

One very reasonable way of dealing with this is by not using before hooks at all and sticking to just being very explicit/intensional about what you're doing, e.g.

it do
 expect { get(:page, param: 1) }.to do something
end

This is a great way of doing things and has very little chance of biting you due to the explicitness

before { request } ; subject { response }

This is another great approach but does not allow for throwing any types of mocks in front of it once you go into deeper contexts.

Request/Reponse approach 💪💪💪💪💪

Let's try something else...something radicial...something that looks terrible at first glance.

Most request specs are really testing the same thing. The "subject under test" (in the RSpec lingo) is the request and reponse cycle.

The request methods (get, post, delete, etc) provided by RSpec return the numeric response code of the request. That's it. Not much to introspec, not much to make assertions on.

It's not common to make the subject of a spec the result of two method calls, but let me explain why it can be a nice way to do DRY things up and have maintainable specs!

# top of the spec

# you can call this anything, but it's helpful to name it so that it's
# clear its executing the request and returning the response 
# (`request_response` could work if you want to be more succinct)

subject(:request_yielding_response) do 
  http_request
  response
end

# @tim made a good point, it's a good idea to name this something other than `request`
# so that it doesn't shadow `request` which is automatically defined by RSpec
let(:http_request) { get bounties_path(code: 'dilbert') } 

# one line syntax 
# (if you're not familar with RSpec this impliclity called subject -- thus eagerly
# evaluating `request_yielding_response` and executing the query)
it { is_expected.to be_ok } 
it 'blah' do
  expect { request_yielding_response }.to change(Bounty, :count).by(1)
  # but maybe don't make assertions on database changes in a request spec? 🤷‍♂️
end

describe 'a different endpoint'
  let(:http_request) { get another_path (code: 'dilbert') }

  it { is_expected.to be_ok }
end

describe 'the json response' do
  subject(:json_response) { JSON.parse(request_yielding_response) }

  it { is_expected.to be a_hash_including(bizzle: 'dizzle') }
end
.
.
.
.

# Here's the part that actually matters and makes this pattern 🚨🚨'Good'🚨🚨
context 'when the saints' do
  context 'come marchin' do
    context 'in' do
      let(:http_request) { get very_specific_path(code: 'dilbert') }

      # dang...I'm deeply nested in a context but now I need to do some spying/mocking
      # no problem -- request_yielding_response is lazy defined, so we can do whatever
      # shenanigans we want here and not need to worry about execution order
      before { allow(Grizzle).to receive(:bizzle).and_return(:zizzle) }

      it { is_expected.to be_ok }
    end
  end
end

# keep in mind we stil have the flexibility to support anyone's preferred style/pecadillos
# if you don't like implicit subjects or before hooks you can throw them directly into the example
it do
  allow(Zerp).to receive(:merp).and_return(:blerp)
  request_yielding_response
  expect('blah').to eq 'blah'
end

describe 'im a eager spy typa person' do
  # eager evaluation of the spy!
  let!(:mailer_spy) { class_spy('TrackerUserMailer', delay: true).as_stubbed_const }
  
  it do
    request_yielding_response
    expect(mailer_spy).to have_received(:delay).exactly(:twice)
  end
end

Pros

1. Lazy subject evaluation allows for:
   • redefining of the subject (useful for testing raw responses as well as json responses)
   • closer control of the order of execution
2. RSpec memoizes subject within an example. That means that it's virtually impossible for us to accidentally reissue the request accidentally. It's pretty much perfect for this style.
3. It's nice to have some basic conventions that we can use (but also not use when it doesn't make sense)
4. Avoids the aforementioned deeply nested before hooks of doom!
5. "Sometimes it's good to have some conventions" -- DHH, probably

Cons

1. Some folks might argue that doing a bunch of deep nesting is uncouth. Totally valid but I think that's a seperate discussion
2. Read this screed and come up with your own cons!

Capybara: The Sequel

Wow congrats on getting all the way down here.

What if we took the same priciples from above and applied them to feature tests? What is the subject under test of a Capybara test MOST of the time?

The request_yielding approach

subject(:page_after_visiting) do
  login_as(user)
  page_to_visit
  page # RSpec feature specs give us this
end

let(:page_to_visit) { visit bounties_page }

it 'has something interesting on it' do
  is_expected.to have_text 'chicken can be a healthy alternative to beef'
end

context 'another page' do
  let(:page_to_visit) { visit submissions page }

  it 'has something interesting on it' do
    is_expected.to have_text 'chicken can be a healthy alternative to beef'
  end
end

context 'with some existing records' do
  # eager evaluate to make sure it's in the db before loading the page
  # think about how many hours you've wasted thinking there should be records only to realize
  # the page had already loaded when you created the records
  let!(:submission) { create :submission } 

  it { is_expected.to have_text 'when you have existing data something shows here' } 
end

The before hook tower of doom

This approach has all the issues described above and more:

before do
  login as somebody
  visit a page
end

it 'has something interesting on it' do
  is_expected.to have_text 'chicken can be a healthy alternative to beef'
end

context 'herp derp here I test a different page' do
  before { visit another_page }

  it 'has something interesting on it' do
    # Works great 👍 👍 👍
    # BUT ooops you just visited the same page twice...and feature tests
    # are slow so this is not great 👎 👎 👎
    is_expected.to have_text 'chicken can be a healthy alternative to beef'
  end
end

The only thing that bugs me is conflating the subject in the request specs. Sometimes you're treating subject like the request, sometimes making assertions on it like its the response. I feel like 99% of request specs could be set up like so, which preserves semantic clarity:

before { http_request }
subject { response }

context 'with bad params' do
   let(:http_request) { ... }
   it { is_expected.to be_ok }
end

Or just leaving it to the example to fire the request and make assertions on the response:
https://relishapp.com/rspec/rspec-rails/docs/request-specs/request-spec#specify-managing-a-widget-with-rails-integration-methods

I just think it's redundant to return the response object in the subject, when the response object is already available for assertions everywhere in the spec.

I raised my views last Friday, and am adding my response here since you took the effort to type this up, but really don't want to waste any more attention or breath on this. If this is the direction for request specs that folks want to go in, I will withold PR comments when it comes up.

If you do something like this:

subject(:request_yielding_response) do 
+  mocks_before_request
  http_request
  response
end

Then you can stub out things you need before the request runs somewhere deep in your chain, eg:

let(:mocks_before_request) { allow(Rails).to receive(:env) { "development".inquiry } }

As a minor point, I don't see why we need to involve subject at all here. +1 what Tim said. response really doesn't seem like it should be the subject -- you get that for free. Thoughtbot also seems to think it should be in a before rather than a subject as well https://github.com/thoughtbot/shoulda-matchers#on-the-subject-of-subject

--

I think doing the mock + request like glenn mentioned (but in a before) is the lesser of the evils, but I think we could benefit a lot from more context in our individual specs, which then makes them much easier to change as there isn't any stepping on toes between the specs (I think this applies to all our unit specs, not just request ones):

# this style doesn't care at all how nested you are, and you also don't care how nested it is when you try and make a change
# this could be lines 340-355 in some file and you don't have to care about any of the rest of it w.r.t befores/subjects
context 'when the saints' do
  context 'come marchin' do
    context 'in' do
      it 'does the right thing' do
        allow(Foo).to receive(:bar) # cool the stubs are happening right here, don't have to worry about other contexts
     
        get some_specific_path # cool, the path is right here in the spec, no need to check the top of the file and every other nested context it's then redefined

        expect(response).to be_bueno
      end
    end
  end
end

The above results in a much more descriptive and easier to manage tests. No before towers of doom, no unexpected subjects or whatever changing through the contexts. The one cost is a bit more duplication, but IMO its well worth it. I'd suggest people try writing request specs without any before/subject at all and seeing how far they get before they really feel they are needed.

insert strong feels against duplication here

Duplication leads to more up front effort, more maintenance effort, and more code to allow potential discrepancies and bugs to slip in.

Using (appropriately scoped, not overloaded with different meanings/contexts) let/subject/etc seems far simpler to me.