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?
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
