Use subject block

[telyn]

(Sorry for making 2 PRs for the same thing - poor organisation on my part, will close the other shortly)

Hello! First off thanks for making such a cool project - rswag is AWESOME with or without this PR.

I only have one minor quibble with rswag-specs, and that's addressed by this PR. Please consider this PR as much a suggestion as a demand - I'm not trying to say the way rswag-specs is wrong or anything, just that I personally have a bit of a hard time working with it as it is.

The context

I'm a big fan of the change matcher. I use it everywhere! But especially in request specs, which I treat as mini-integration tests where I want to start with the database in some state (e.g. a user exists), perform a request (PUT /users/:that_user/password), and verify that something happened (that_user's encrypted_password has changed).

And I like the subject method in RSpec - pretty much all my request specs until now looked vaguely like this:

RSpec.describe "PUT /users/:user_id/password" do
  subject do
    put "/users/#{user_id}/password", params: {
      current_password: current_password,
      new_password: new_password,
    }
    response
  end

  let!(:user) { create(:user, password: user_password) }
  let(:user_password) { "IAmCoolJeff" }
  let(:new_password) { "ThisIsCoolJeffSpeaking" }

  # i tend to have predictable names for my test vars, so "when logged in as user" here would probs
  # have a line like the below, then go on to use it to call `sign_in(user)` in a `before` block or something.
  # let(:user) { create(:user) } unless defined?(user)
  include_context "when logged in as user" do
    context "when current_password is correct" do
      let(:current_password) { "IAmCoolJeff" }

      it { expect(subject).to be_successful }
      it { expect { subject }.to change { user.reload.encrypted_password } }
    end
  end
end

But that doesn't work too good in this new run_test! paradigm I'm finding myself in - the before block causes the request to already have been made before my poor it block even runs - so I can't do my usual verification.

The change

• In run_test!:
  • Swap out before for subject
  • put subject at the top of the it block

This doesn't seem to break any of the tests for rswag-specs. I do worry that I might have broken other peoples' tests though - if they're assuming the request will have already been made in their own it blocks, then perhaps their tests won't pass. Like, if they were doing something like this:

response "200", "creates the user" do
  run_test!
  it "sets the new user's name correctly" do
    expect(User.last.name).to eq("Bob Newbie")
  end
end

So I can see why you might want to hold off on merging this, for sure :-)

The example

For completeness' sake, here's my vision for how I'd write a request spec with the changes I've made to rswag-specs. I do realise that my way will cause more requests to be made to the server. If I were concerned by the amount of time my tests were taking I would implement them in a different way, but until that happens I much prefer the expressiveness and modularity of having an it block per type of assertion I'm making (e.g. I'll have a whole bunch of expects in the same it block if I'm testing the content of a hash, but I'll have one expect per it for things like checking subject creates a User, and a LogEntry to say that the user was created, etc.)

RSpec.describe "API" do
  path "/users/{user_id}/password" do
    put "reset the user's password" do
      parameter name: :user_id,
                in: :path,
                type: :integer
      parameter name: :password_reset,
                in: :body,
                schema: {
                  # elided for brevity
                }

      let!(:user) { create(:user, password: user_password) }
      let(:user_password) { "IAmCoolJeff" }
      let(:new_password) { "ThisIsCoolJeffSpeaking" }

      response "200", "resets the user's password" do
        include_context "when logged in as user" do
          run_test!

          it "changes the user's password" do
            expect { subject }.to change { user.reload.encrypted_password }
          end
        end
      end

      response "400", "bad request when current_password is incorrected" do
        include_context "when logged in as user" do
          run_test!

          it "does not change the user's password" do
            expect { subject }.not_to change { user.reload.encrypted_password }
          end
        end
      end
    end
  end
end

Because I worry about how a PR like this might seem, I just want to make extra clear that I am not trying to tell anyone else how to write their tests - and if the rswag maintainers decide that changing before to subject is too likely to break stuff or doesn't fit their vision of how rswag-specs is intended to be used, that'll be fine. Just opening the door for a conversation! :smile:

[jamie]

I like this, and am also a big fan of change, but definitely I don't want to break everyone else's usage. Lemme noodle on this for a bit - simplest thing to do here might be adding a second helper method for this usage.

[telyn]

simplest thing to do here might be adding a second helper method for this usage.

🤦‍♀️ that's such an obvious and good solution, how did I not think of it!?

[jaydorsey]

I'm also a big fan of the change syntax. A new helper method name for this would be fine for this, I think. I don't know if run_test is used (or would be too confusing/risky) but I think it would be fine

[jtannas]

@telyn heyo - sorry that it has taken us so long to get around to this. The project stalled for a few years but now I'm hoping to get us caught up on issues & PRs. Is this something you still want to pursue?

As for the change itself, I like the concept but I'm still quite new to code on the project so I'm hesitant about making significant changes just yet :sweat_smile: On the plus side of it though, I'm not attached to anything just yet so no worries about making big suggestions :slightly_smiling_face:

I think you might be able to get you want using some existing tools :thinking:

RSpec.describe "API" do
+ subject(:api_request) { submit_request(example.metadata) } 
  path "/users/{user_id}/password" do
    put "reset the user's password" do
      parameter name: :user_id,
                in: :path,
                type: :integer
      parameter name: :password_reset,
                in: :body,
                schema: {
                  # elided for brevity
                }

      let!(:user) { create(:user, password: user_password) }
      let(:user_password) { "IAmCoolJeff" }
      let(:new_password) { "ThisIsCoolJeffSpeaking" }

      response "200", "resets the user's password" do
        include_context "when logged in as user" do
-          run_test!
-
          it "changes the user's password" do
            expect { subject }.to change { user.reload.encrypted_password }
          end
        end
      end

      response "400", "bad request when current_password is incorrected" do
        include_context "when logged in as user" do
-          run_test!
-
          it "does not change the user's password" do
            expect { subject }.not_to change { user.reload.encrypted_password }
          end
        end
      end
    end
  end
end

Does this match the vision you have in mind? If so I'd be happy to add it to the wiki as an alternative to run_test!

[danielolivaresd]

@jtannas thanks for jumping into this. FYI I couldn't use your example as-is, as the subject has no notion of the example, but submit_request(example.metadata) in each example did the trick.

[jtannas]

@jtannas thanks for jumping into this. FYI I couldn't use your example as-is, as the subject has no notion of the example, but submit_request(example.metadata) in each example did the trick.

Thanks for testing it - I probably should've done that myself before posting that snippet :sweat_smile: I'll fiddle around with it a bit to see if there's a way to make it work without repeating submit_request everywhere, and from there write a piece in the wiki for it.

[jtannas]

Attempt no. 2:

RSpec.describe "API" do
- subject(:api_request) { submit_request(example.metadata) }
+ subject(:api_request) { |example| submit_request(example.metadata) }

I've tested it on a few simple cases and it seems to work but it's possible I've missed an edge case or two. It allows you to have the single subject definition at the top of the file and it'll be usable all over.

response '200', 'Okay' do
  context 'when a valid request is made' do
    it 'creates a new user' do |example|
      expect { api_request }.to change(User, :count).by(1)
      assert_response_matches_metadata(example.metadata)
    end
  end
end

[jtannas]

No updates to this thread for 2 months so I'm guessing interest has waned. I'll add the example to the wiki and close the PR. Feel free to bring it about again though if you think the issue needs further attention though.

https://github.com/rswag/rswag/pull/363