Hola,
I went ahead and wrote some code instead of emails and implemented it and send
a pull request.
The way I implemented was just pretty simple:
- and_raise has a second parameter which is an empty string by default
- when raise_exception is called and the exception passed is a Class, then it
will pass the message
- The spec that didn't allow for exception classes with constructor arguments
has been changed to allow for 1, the message
The pull request stayed close to the original code. However, an alternative
implementation would be:
- The and_raise itself creates an object when a class is passed. This allows
for the "too much arguments" check to be done earlier and it would fail earlier.
- The raise_exception would simply raise an object (always) and never a Class
This does change the behavior from late fail to early fail. I'm not sure if
there is a good reason to the current late fail, so I sticked with the original
implementation.
Pull request done. Feel free to decline it if you want it implemented
differently. If you want to see the alternative, I can make a new pull request
with the alternative implementation.
Thanks,
Bas
ps. If you want to ignore it completely, thats also ok with me. It is not a
major point, it just was one of the surprises I had working with rspec, and
surprise usually suggest a potential API improvement :)
On 22 Aug, 2012, at 7:59 PM, David Chelimsky <dchelim...@gmail.com> wrote:
> 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
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
