Remove expect_raises matcher without type argument
#5096
Conversation
spec/std/char_spec.cr
Outdated
| @@ -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 | |||
straight-shoota
force-pushed
the
jm-remove-expect-raises
branch
from
27788ea to
6b727eb
Compare
spec/std/char_spec.cr
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
@straight-shoota I think should be:
expect_raises ArgumentError, "Invalid range c-a" do
spec/std/char_spec.cr
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Similar to the other changes, please use () around method call when a block is used. Thank you.
There was a problem hiding this comment.
Ah yes, I started doing this somewhere in between, but later forgot to edit the prior ones...
There was a problem hiding this comment.
Hm, there are 218 occassions in the stdlib specs ...
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Well, I was already at it... and it was nearly easier to take all at once
|
@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: |
|
Well, you can look at the individual commits like 6b727eb. But okay, I'll reorganize this in a different PR. |
straight-shoota
force-pushed
the
jm-remove-expect-raises
branch
from
f1841c3 to
8a86dbe
Compare
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>
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>
As @yxhuvud mentioned in #5092 (line-comment), the arg-less
expect_raisesmatcher 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 typeException(no subtypes -- but that's debatable) with a matching message, being the equivalent matcher for exceptions created byraise(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.Slicecould also be a candidate for a custom exception.Enum.from_yamlparsing an integer id raisesException: Unknown enum YAMLSpecEnum value: 3, but parsing a enum name raisesArgumentError: Unknown enum YAMLSpecEnum value: Three(messages taken fromyaml/serialization_spec.cr). I think both should raise the same error type, since there is no real semantic difference.