-
-
Notifications
You must be signed in to change notification settings - Fork 395
Fix names of error in test descriptions of Kernel#raise #1315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This was probably a leftover of a copy-paste, these should check TypeError instead of ArgumentError.
|
|
||
| it "doesn't raise an ArgumentError when given cause is nil" do | ||
| it "doesn't raise a TypeError when given cause is nil" do | ||
| -> { raise "message", cause: nil }.should raise_error(RuntimeError, "message") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, shouldn't this be a TypeError here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't raise a TypeError, i.e. it succeeds but this is raise so succeeding means throwing an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, somehow completely blinded the "doesn't" part. Maybe the description would be better as "raises RuntimeError".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was written to contrast the test above it:
it "raises a TypeError when given cause is not an instance of Exception" do
-> { raise "message", cause: Object.new }.should raise_error(TypeError, "exception object expected")
end
it "doesn't raise a TypeError when given cause is nil" do
-> { raise "message", cause: nil }.should raise_error(RuntimeError, "message")
endWe're testing (speccing?) the different types that can be given as the cause argument. An object that is not nil and not an Exception is not allowed, so the cause argument causes the exception.
In the second test, we're passing a nil as cause argument, this is accepted and ignored, and now the raised exception is what we explicitly do with raise.
So both cases throw an exception, but it's for very different reasons, and the second one is the happy path.
Maybe something like "it accepts nil as a value for cause is accepted and ignored" would be a better description. And to put the icing on the cake, we should probably add a block to raise_error and verify e.cause.should be_nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A describe "the cause: argument" do would also help clarity & structure, and make it clearer those examples are related.
This was probably a leftover of a copy-paste, these should check TypeError instead of ArgumentError.