Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block matchers (e.g. #change) only support passing procs to expect. #1106

Closed
lukeredpath opened this issue Apr 1, 2019 · 6 comments
Closed

Comments

@lukeredpath
Copy link
Contributor

lukeredpath commented Apr 1, 2019

Subject of the issue

Certain matchers, like #change, require that you pass a proc to #expect - this makes sense, it needs to be able to defer invoking the operation inside the block in order to test the before and after values.

However, this hard requirement for a Proc denies us the opportunity to create higher-level abstractions. For instance, wrapping a request in a Rails request test, I'm quite keen of using the pattern of defining a spec's subject as the request inside a proc that can be passed to expect in order to succinctly test the request's side effects, for example:

subject do
  -> { delete :widget, id: 1 }
end

it "deletes the widget" do
  expect(subject).to change(Widget, :count).by(-1)
end

The downside to this pattern is that if I want to test something after the request is made, I need to explicitly call it:

it "has the right response" do
  subject.call
  expect(response).status).to eql(200)
end

It would be nice to be able to wrap this pattern up in a callable object which encapsulates both the request and the response and when combined with something like rspec-its can lead to some nicely succinct specs:

DeferredRequest = Struct.new(:method, :args, :context, keyword_init: true) do
  attr_accessor :request_made

  def call
    context.send(method, *args)
    self.request_made = true
  end

  def response
    call unless request_made
    context.response
  end
end

# This method is included in specs as a helper method
def make_deferred_request(method, *args()
  DeferredRequest.new(method: method, args: args, context: self)
end

describe "DELETE /widget/:id" do
  subject { make_deferred_request(:delete, "/widget/#{widget.id}") }

  it { is_expected.to change(Widget, :count).by(-1))
  its(:response) { is_expected.to eql(:found) }
end

Unfortunately this fails with the error:

expected Widget.count not to have changed, but was not given a block

Expected behavior

It would be better if matchers that expected a proc argument to expect in fact accepted any object that responded to #call - this would allow the following abstraction to work. This would be a backwards-compatible API because Proc already responds to #call.

I'm happy to look at submitting a PR for this change but wanted to get some feedback first.

@JonRowe
Copy link
Member

JonRowe commented Apr 1, 2019

This has sort of come up before, and a gem was created to expand the inline syntax to add something similar. https://github.com/d-unseductable/rspec-has in this case I feel you could happily solve the problem in a similar fashion by creating an alias for expect { subject.call } without adding complexity to the matcher. I'm not shooting down a PR but I am questioning is the trade off worth it.

@benoittgt
Copy link
Member

I am wondering if it is a good idea too. I find it a little less explicit, but maybe because I have too many habits. 🤷‍♂️
@lukeredpath does the gem linked by Jon is not good enough for you?

I would prefer the approach of rspec-has if we want to implement something into rspec-expectations

@pirj
Copy link
Member

pirj commented May 8, 2019

Would you consider a nested expectation as an option @lukeredpath ?

describe "DELETE /widget/:id" do
  subject(:request) { make_deferred_request(:delete, "/widget/#{widget.id}") }

  specify do
    expect {
      expect(request.response).to eql(:found)
    }.to change(Widget, :count).by(-1)
end

Another option would be to use explicit block syntax:

describe "DELETE /widget/:id" do
  subject(:request) { make_deferred_request(:delete, "/widget/#{widget.id}") }

  it { expect { request }.to change(Widget, :count).by(-1))
  its(:response) { is_expected.to eql(:found) }
end

Related: some thoughts on implicit block syntax's complications.

@pirj
Copy link
Member

pirj commented Jun 18, 2019

@lukeredpath what are your thoughts on this?

@pirj
Copy link
Member

pirj commented Jul 18, 2019

@lukeredpath What do you think of alternative ways to achieve the same goal?
Appreciate if you can provide arguments for the syntax in question, and against alternative approaches.

@pirj
Copy link
Member

pirj commented Nov 13, 2019

There are multiple mechanisms in RSpec and Ruby that would allow to write a concise spec like this:

describe "DELETE /widget/:id" do
  subject(:request) { make_deferred_request(:delete, "/widget/#{widget.id}") }

  it { expect { request }.to change(Widget, :count).by(-1) }
  it { expect { request }.to change { response.status }.to(:found) }
end

or for those who don't blindly follow the one literal expectation per example rule:

it do
  expect { request }
    .to change(Widget, :count).by(-1)
    .and change { response.status }.to(:found)
end

All at the same time without blurring the line between block and value expectations.
I clearly understand that the composition of those two kinds of expectations when checking side effects of an HTTP requests is non-trivial.

@lukeredpath Please feel free to open if you have a clear vision on how to proceed with this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants