Skip to content
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

Remove expect_raises matcher without type argument #5096

Merged
merged 1 commit into from Oct 14, 2017

Conversation

straight-shoota
Copy link
Member

As @yxhuvud mentioned in #5092 (line-comment), the arg-less expect_raises matcher doesn't verify the type of an exception: it rescues any exception raised by the example block and thus can lead to hidden errors because even unexpected exceptions are accepted.

That's why this matcher has been deprecated in rspec.

An example should only be expected to raise one specific type of exception.

I'd suggest to remove this matcher entirely. There is no reason to have it besides implicitly advising people to use this instead of a proper type-checking matcher. There were quite a few in the standard lib alone.
If, for some reason, there is a very unlikely need to rescue any spec, it can simply be replaced by expect_raises(Exception).

It's also an option to deprecate it at first and remove it later, but it's only breaking (badly-designed) specs.

As a replacement, it might be considered to add expect_raises(message) which only matches an exception of the exact type Exception (no subtypes -- but that's debatable) with a matching message, being the equivalent matcher for exceptions created by raise(message : String).

I also noticed a few exceptions which could be improved:

  • Nil#not_nil! could raise a custom exception (NilAssertionError). It is commonly used such that a specific exception type would be great.
  • writing to a read-only Slice could also be a candidate for a custom exception.
  • Enum.from_yaml parsing an integer id raises Exception: Unknown enum YAMLSpecEnum value: 3, but parsing a enum name raises ArgumentError: Unknown enum YAMLSpecEnum value: Three (messages taken from yaml/serialization_spec.cr). I think both should raise the same error type, since there is no real semantic difference.

@@ -326,7 +326,7 @@ describe "Char" do
it { 'a'.in_set?("ab", "ac", "ad").should be_true }

it "rejects invalid ranges" do
expect_raises do
expect_raises ArgumentError, "Invalid range c-a"do
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space before do.

@@ -326,7 +326,7 @@ describe "Char" do
it { 'a'.in_set?("ab", "ac", "ad").should be_true }

it "rejects invalid ranges" do
expect_raises do
expect_raises ArgumentError, "Invalid range c-a"do
Copy link
Contributor

Choose a reason for hiding this comment

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

@straight-shoota I think should be:

expect_raises ArgumentError, "Invalid range c-a" do

@@ -326,7 +326,7 @@ describe "Char" do
it { 'a'.in_set?("ab", "ac", "ad").should be_true }

it "rejects invalid ranges" do
expect_raises do
expect_raises ArgumentError, "Invalid range c-a" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the other changes, please use () around method call when a block is used. Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, I started doing this somewhere in between, but later forgot to edit the prior ones...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, there are 218 occassions in the stdlib specs ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, there are 218 occassions in the stdlib specs

Leave those for another PR, let's focus this PR only on the ones modified by you 😉

Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I was already at it... and it was nearly easier to take all at once

@luislavena
Copy link
Contributor

@straight-shoota ouch on last commit noise 😞

As suggested, this PR should focus on one specific thing. The cosmetic details of other elements across the codebase can be solved in a separate PR.

It now makes the entire PR review a bit more tedious, basically:

@straight-shoota
Copy link
Member Author

Well, you can look at the individual commits like 6b727eb. But okay, I'll reorganize this in a different PR.

@RX14 RX14 added this to the Next milestone Oct 14, 2017
@RX14 RX14 merged commit 3f99c77 into crystal-lang:master Oct 14, 2017
@straight-shoota straight-shoota deleted the jm-remove-expect-raises branch October 14, 2017 18:11
miketheman added a commit to miketheman/statsd.cr that referenced this pull request Dec 26, 2017
In 0.24.x, the syntax required for the `expect_raises` matcher has been
changed to require a specific exception class.

Refs crystal-lang/crystal#5096

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
miketheman added a commit to miketheman/statsd.cr that referenced this pull request Dec 26, 2017
In 0.24.x, the syntax required for the `expect_raises` matcher has been
changed to require a specific exception class.

Refs crystal-lang/crystal#5096

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants