On Wed, Aug 22, 2012 at 6:55 AM, David Chelimsky <dchelim...@gmail.com> wrote:
> On Wed, Aug 22, 2012 at 6:43 AM, Bas Vodde <b...@odd-e.com> wrote:
>>
>> On 22 Aug, 2012, at 7:25 PM, David Chelimsky <dchelim...@gmail.com> wrote:
>>
>>> On Wed, Aug 22, 2012 at 12:56 AM, Bas Vodde <b...@odd-e.com> wrote:
>>>>
>>>> Hiya all,
>>>>
>>>> I was trying to get and_raise to raise an exception filled with a message
>>>> and I was struggling with the API for a while (not on the latest RSpec,
>>>> but assume it didn't change).
>>>>
>>>> Based on that, I have a suggestion for improvement. My first attempt was
>>>> to mirror how I use raise, so I tried:
>>>>
>>>>
>>>> subject.should_receive(:do_system).and_raise(Osaka::SystemCommandFailed,
>>>> "execution error: System Events got an error: Access for assistive devices
>>>> is disabled. (-25211)")
>>>>
>>>> This didn't work as the and_raise only has one parameter. Eventually I
>>>> figured out this works:
>>>>
>>>>
>>>> subject.should_receive(:do_system).and_raise(Osaka::SystemCommandFailed.new
>>>> ("execution error: System Events got an error: Access for assistive
>>>> devices is disabled. (-25211)"))
>>>>
>>>> And it does what I want it to do. Still… I would have prefered the first
>>>> one to work too :) Implementing that ought to be reasonably trivial,
>>>> correct?
>>>> (didn't implement it myself yet this time).
>>>>
>>>> Comments? Or is there anyone way of doing this?
>>>>
>>>> Thanks!
>>>>
>>>> Bas
>>>
>>> Documentation here:
>>> http://rubydoc.info/gems/rspec-mocks/RSpec/Mocks/MessageExpectation#and_raise-instance_method.
>>>
>>> That has been the API for something like 5 years so I'm not sure I
>>> want to change it. It is used to prepare an exception to raise, not
>>> compare with one that was already raised, so we'd have to support n
>>> args, e.g.
>>>
>>> and_raise(AnException, with_one_arg)
>>> and_raise(AnException, with_one_arg, and_another_arg)
>>>
>>> etc. I think this would be friendly, but it might also be confusing
>>> because we'd effectively have 2 completely different APIs for the same
>>> method. Also, I'm not sure that either of those examples are much
>>> better than:
>>>
>>> and_raise(AnException.new with_one_arg)
>>> and_raise(AnException.new with_one_arg, and_another_arg)
>>>
>>> I'm open to the idea though for one reason: the rspec-expectations
>>> raise_exception method can accept 1 or two args, so I can imagine
>>> something like:
>>>
>>> describe Foo do
>>> it "raises when x happens" do
>>> foo = Foo.new(Bar.new)
>>> expect { Foo.do_something_bad }.to raise_exception(FooException,
>>> "with this message")
>>> end
>>>
>>> it "does x when its bar raises" do
>>> bar = double
>>> foo = Foo.new(bar)
>>> bar.should_receive(:msg).and_raise(BarException, "with this message")
>>> end
>>> end
>>>
>>> Your suggestion makes these two APIs align better if the exception
>>> initializer accepts one argument and raise_exception gets a String.
>>> But raise_exception can also take a regex, and exception initializers
>>> can take more than one arg. Consider: InsufficientFunds.new(50, :USD).
>>> The way things are today, you might see these two near each other
>>> (hopefully not in the same example):
>>>
>>> expect { transfer.execute }.to raise_exception(InsufficientFunds, /50 USD/)
>>> source_account.should_receive(:withdraw).and_raise(InsufficientFunds.new
>>> 50, :USD)
>>>
>>> With what you propose it would be this:
>>>
>>> expect { transfer.execute }.to raise_exception(InsufficientFunds, /50 USD/)
>>> source_account.should_receive(:withdraw).and_raise(InsufficientFunds,
>>> 50, :USD)
>>>
>>> I think the way it is now tells a better story about what's really
>>> going on (i.e. that you're supplying an initialized object to
>>> and_raise) than the proposed change. But that's one opinion.
>>>
>>> Any others out there?
>>
>> Hi David,
>>
>> My main thinking was to make it consistent with the Kernel.raise.
>>
>> Like, in my production code, I have:
>>
>> raise Osaka::SystemCommandFailed, output_message
>>
>> so, it would make sense to the mock to work the same with:
>>
>> and_raise Osaka::SystemCommandFailed, "Fake output message"
>>
>> I figured it would be a relative minor change as it would add one extra
>> parameter which could default to an empty string.
>> (as reference, see Kernel raise:
>> http://www.ruby-doc.org/core-1.9.3/Kernel.html)
>>
>> I had not considered the consistency with and_raise_exception, but I just
>> noticed that this was one of the cases where my default excepted behavior
>> from RSpec was different from what the API wanted. Therefore, I had to dive
>> in the RSpec doc to see how I could pass the message….
>>
>> Bas
>
> Unfortunately, the Kernel#raise API also accepts a 3rd argument, which
> is an Array of caller information (the backtrace):
> http://www.ruby-doc.org/core-1.9.3/Kernel.html#method-i-raise.
>
> What that doc doesn't tell you is that the first arg can actually be
> an initialized exception:
>
> raise InsufficientFunds.new(49, :USD)
>
> I can't remember where I heard this but there was some advice from a
> respected software developer that said something like "APIs should be
> loose with what they accept and strict with what they return." Based
> on that advice, we should support your suggestion. It's not like if
> you use one API or the other you're going to get a different result.
> Beginning to lean in favor.
>
> More opinions?
Then again ...
raise InsufficientFunds, 50, :USD # raises ArgumentError
and_raise InsufficientFunds, 50, :USD # currently raises
ArgumentError, would not if we implement this suggestion
So it depends on what we want to align with.
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
