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
On Wed, Aug 22, 2012 at 12:56 AM, Bas Vodde <basv at 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! > > BasDocumentation 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 On 22 Aug, 2012, at 7:25 PM, David Chelimsky <dchelimsky at gmail.com> wrote:> On Wed, Aug 22, 2012 at 12:56 AM, Bas Vodde <basv at 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? > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users
On Wed, Aug 22, 2012 at 6:43 AM, Bas Vodde <basv at odd-e.com> wrote:> > On 22 Aug, 2012, at 7:25 PM, David Chelimsky <dchelimsky at gmail.com> wrote: > >> On Wed, Aug 22, 2012 at 12:56 AM, Bas Vodde <basv at 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?. > > BasUnfortunately, 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?
On Wed, Aug 22, 2012 at 6:55 AM, David Chelimsky <dchelimsky at gmail.com> wrote:> On Wed, Aug 22, 2012 at 6:43 AM, Bas Vodde <basv at odd-e.com> wrote: >> >> On 22 Aug, 2012, at 7:25 PM, David Chelimsky <dchelimsky at gmail.com> wrote: >> >>> On Wed, Aug 22, 2012 at 12:56 AM, Bas Vodde <basv at 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.
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 <dchelimsky at gmail.com> wrote:> On Wed, Aug 22, 2012 at 6:55 AM, David Chelimsky <dchelimsky at gmail.com> wrote: >> On Wed, Aug 22, 2012 at 6:43 AM, Bas Vodde <basv at odd-e.com> wrote: >>> >>> On 22 Aug, 2012, at 7:25 PM, David Chelimsky <dchelimsky at gmail.com> wrote: >>> >>>> On Wed, Aug 22, 2012 at 12:56 AM, Bas Vodde <basv at 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 at rubyforge.org > http://rubyforge.org/mailman/listinfo/rspec-users