Skip to content

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.

Sign up for GitHub

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

Merged
eregon merged 1 commit into from
Dec 22, 2025
+2 −2
Merged

Fix names of error in test descriptions of Kernel#raise #1315

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/kernel/raise_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@
-> { raise(cause: nil) }.should raise_error(ArgumentError, "only cause is given with no arguments")
end

it "raises an ArgumentError when given cause is not an instance of Exception" do
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 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")
Copy link
Contributor

@trinistr trinistr Jan 5, 2026

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?

Copy link
Member

@eregon eregon Jan 5, 2026

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

Copy link
Contributor

@trinistr trinistr Jan 5, 2026

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".

Copy link
Member Author

@herwinw herwinw Jan 5, 2026

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")
end

We'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

Copy link
Member

@eregon eregon Jan 5, 2026 •
edited
Loading

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.

Copy link
Contributor

@trinistr trinistr Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering it's me who was confused, I will make a PR for this soon™

end

Expand Down