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

Consider deprecating and_return { value } #558

Closed
yujinakayama opened this issue Feb 5, 2014 · 15 comments
Closed

Consider deprecating and_return { value } #558

yujinakayama opened this issue Feb 5, 2014 · 15 comments

Comments

@yujinakayama
Copy link
Member

I've found a comment “TODO: deprecate and_return { value }here. Should this be removed in RSpec 3?

Actually I think this is not so problematic in comparison to the issue in with { ... }, so I won't insist (and I don't want to have more work on Transpec 😁).

What do you think?

@myronmarston
Copy link
Member

I think it would be good to deprecate and_return { value } in 2.99 and remove it in 3.0. It hasn't been a priority for us yet, though. We'd gladly merge a PR to do this -- want to take a stab at it? It'd be great to get two PRs -- one for 2.99 with the deprecation and one for 3.0.

@yujinakayama
Copy link
Member Author

OK, I'll try.

@yujinakayama
Copy link
Member Author

I've noticed it's more problematic than I thought. It behaves unexpectedly with allow(...).to receive(...) and do end block:

describe '`and_return` without arguments' do
  let(:obj) { double('obj') }

  context 'with `stub`' do
    context 'and `{ }` block' do
      it 'works properly' do
        obj.stub(:foo).and_return {
          'a return value'
        }

        expect(obj.foo).to eq('a return value')
      end
    end

    context 'and `do end` block' do
      it 'works properly' do
        obj.stub(:foo).and_return do
          'a return value'
        end

        expect(obj.foo).to eq('a return value')
      end
    end
  end

  context 'with `allow(...).to receive(...)`' do
    context 'and `{ }` block' do
      it 'works properly' do
        # The block is taken by #and_return.
        allow(obj).to receive(:foo).and_return {
          'a return value'
        }

        expect(obj.foo).to eq('a return value')
      end
    end

    context 'and `do end` block' do
      it 'returns nil unexpectedly' do
        # The block is taken by #to.
        allow(obj).to receive(:foo).and_return do
          'a return value'
        end

        # Describing the unexpected behavior,
        # this should not pass, but passes.
        expect(obj.foo).to be_nil
      end
    end
  end

  context 'without block' do
    it 'returns nil' do
      # Shouldn't this raise error?
      allow(obj).to receive(:foo).and_return

      expect(obj.foo).to be_nil
    end
  end
end

This is because:

  • and_return accepts no arguments. It receives empty array values and [].first returns nil.
  • The AndReturnImplementation is set to self.terminal_implementation_action, so it takes precedence over self.inner_implementation_action (the block passed from to) as a return value implementation.

So, I think we should probably disallow the and_return without arguments whether it's taking block or not.

Thoughts?

The spec gist: https://gist.github.com/yujinakayama/8847524

@myronmarston
Copy link
Member

For the do...end case, it actually works properly in master now. We fixed this 4a8f672. Your it 'returns nil unexpectedly' spec fails on rspec-mocks master currently.

So, I think we should probably disallow the and_return without arguments whether it's taking block or not.

I agree. and_return with no arguments doesn't make sense; it means you aren't specifying what to return even though you are calling a method which is intended to specify what to return. I think that we can get an easy, appropriate error with minimal effort by defining that method like so:

def and_return(first_return, *values)
  values.unshift(first_return)
  # ...
end

...rather than:

def and_return(*values)
  # ...
end

Then ruby itself will provide an appropriate ArgumentError and will enforce they always have to provide at least one arg.

@yujinakayama
Copy link
Member Author

I agree. and_return with no arguments doesn't make sense;

OK. I'll handle it also.

For the do...end case, it actually works properly in master now.

Sorry, I've tested only on 2-99-maintenance branch. By the way, doesn't the patch need to be applied to 2-14-maintenance and 2-99-maintenance?

@myronmarston
Copy link
Member

Sorry, I've tested only on 2-99-maintenance branch. By the way, doesn't the patch need to be applied to 2-14-maintenance and 2-99-maintenance?

If it was a regression in 2.14 or 2.99, then definitely. However, given that the allow syntax had never been available before 2.14, it's a bug in a new 2.14 feature, and it's one that a user had never reported or commented on -- I had discovered it myself and applied a fix.

If there's demand from users I'll backport it but there hasn't been.

@yujinakayama
Copy link
Member Author

Closing since #560 and #561 have been merged.

@bronson
Copy link

bronson commented Mar 3, 2016

Curious why and_return was deprecated... To my eyes:

expect(controller).to receive(:send_file).once.and_return { controller.render :nothing => true }

is more readable than:

expect(controller).to receive(:send_file).once { controller.render :nothing => true }

That second one seems harder to figure out, like a paragraph that's missing punctuation.

@myronmarston
Copy link
Member

and_return hasn't been deprecated. You can still use it:

allow(double).to receive(:foo).and_return(17)

What's been deprecated is passing only a block to and_return.

@bronson
Copy link

bronson commented Mar 3, 2016

That's what I meant. :) The example is only passing a block.

Is there a more readable way of writing this that complies with the deprecation?

@myronmarston
Copy link
Member

The problem is that the block can do literally anything. It's not just for returning values. It can perform any side effect, throw a symbol, raise an error or do whatever. As such, using and_return(value) makes sense (clearly it will return a value) whereas and_return { block_that_could_literally_do_anything_including_preventing_the_stub_from_returning_a_value } makes less sense, and we didn't see a reason to keep supporting that. You can do this:

expect(controller).to receive(:send_file).once.and_return(controller.render :nothing => true)

@JonRowe
Copy link
Member

JonRowe commented Mar 3, 2016

expect(controller).to receive(:send_file) { controller.render :nothing => true } achieves your goal, it's readable enough when you realise that the convention is a blocks return value is returned from the expectation.

@bronson
Copy link

bronson commented Mar 4, 2016

@myronmarston, in this case, 'literally everything' is the behavior I like. :) Calling controller.render in response to send_file marks the action as having rendered. If I call controller.render before the test starts, as you suggest, then Rails would think the action had already rendered and return nothing.

@JonRowe thanks for confirming that this is as readable as it gets. It doesn't seem particularly friendly to people less familiar with rspec (to me) but that's OK.

Thank you to you both for taking your time to explain this. It will work fine.

@JonRowe
Copy link
Member

JonRowe commented Mar 4, 2016

It doesn't seem particularly friendly to people less familiar with rspec (to me) but that's OK.

and_return { ... } actually has it's own issues in that regard, as it's not doing what it says (the block is an implementation, not a return value).

@bronson
Copy link

bronson commented Mar 4, 2016

Well that's a flippin' good point.

expect(controller).to receive(:send_file).and_also_call { controller.render :nothing => true }

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