rspec / rspec-core

RSpec runner and formatters
http://rspec.info
MIT License
1.22k stars 762 forks source link

`obj.stub!(:foo).with { ... }` triggers twice #254

Closed lsegal closed 13 years ago

lsegal commented 13 years ago

When .with is supplied with a block, the block seems to be called twice. The following spec fails:

class Test; def foo; puts "Hello" end end 

describe Test do
  it "should work" do
    data, test = 0, Test.new
    test.stub!(:puts).with { data += 1 }
    test.foo
    data.should == 1
  end
end

Gives "expected 1, got 2 (using ==)"

A somewhat more elaborate example is at https://gist.github.com/744568

This worked fine in rspec-1.3.0, but fails in 2.x.

dchelimsky commented 13 years ago

The with method is designed to constrain the stub, not define return values. For example:

adder = double('adder')
adder.stub(:add) { 0 }
adder.stub(:add).with(2,3) { 5 }
adder.add(1,2).should eq(0)
adder.add(2,3).should eq(5)

It is not intended to accept a block as its data. If that worked in rspec-1 it was purely accidental.

In your case, it works fine if you don't say with. This passes:

class Test; def foo; puts "Hello" end end 

describe Test do
  it "should work" do
    data, test = 0, Test.new
    test.stub!(:puts) { data += 1 }
    test.foo
    data.should == 1
  end
end

I think the best thing to do is raise an informative error when with receives a block without any other arguments. WDYT?

lsegal commented 13 years ago

I don't mind using the block syntax, but the problem here is that any mutative code executed in the with block will "unexpectedly" duplicate its mutation. My example perhaps was a little poor, but consider an example that actually is checking argument constraints:

class Test
  def foo
    create(1); create(2); create(4)
  end
end

def next_id; (($id ||= 0) += 1) * 2 end

it "should work" do
  test = Test.new
  test.stub!(:create).with {|id| id.should == next_id }
  test.foo
end

This example seems normal enough, but will cause unexpected failures because with triggers twice. Although I understand you can refactor this, I imagine a few users will bump their heads against this behaviour, because it's a little unintuitive.

Is there a reason the block can't just trigger one time as the regular stub block does?

dchelimsky commented 13 years ago

It's not a matter of minding using the block syntax. It is not supported. It just works by accident and incorrectly, so don't use it.

The question now is whether to explicitly "un-support" it by raising an error when passing a block to with.

lsegal commented 13 years ago

If it is not supported, why is the block being executed at all? Is this just for legacy purposes? If so, yes, if not raise an error, a deprecation warning message would be useful and it should eventually be removed completely (or is that the plan?).

In either case, perhaps the documentation should be more clear that with(&block) should no longer(?) be used.

kaiwren commented 13 years ago

@dchelimsky Please take a look at rspec/rspec-mocks#47 and let me know if this makes sense for this ticket?

dchelimsky commented 13 years ago

@kaiwren - actually, the problem is not that there is a block, but that there is no arg preceding it. So, this is legal:

foo.should_receive(:bar).with(:baz) { 'bam' }

But this is not:

foo.should_receive(:bar).with { 'bam' }

The bug is that, in the latter case, the block gets passed to ArgumentExpectation.new, so it thinks that the block is the expected argument AND the block is treated as the return value, which is why it's being eval'd twice.

I've got a fix in the works and will post it shortly.

dchelimsky commented 13 years ago

Closed by https://github.com/rspec/rspec-mocks/commit/042784ae718249d02220e2e40ee82904cbcdf1a2 in the rspec-mocks repo.