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

Support @test_throws just checking the error message #41888

Merged
merged 3 commits into from
Aug 17, 2021
Merged

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 14, 2021

With this PR,

@test_throws "reducing over an empty collection" reduce(+, ())

allows you to check the displayed error message without caring about
the details of which specific Exception subtype is used.

Now let me give a little more background: as mentioned in #41885, the exception type of errors has changed repeatedly. IMO, the exception type's main purpose is to ensure that the correct message gets delivered to the user. The fact that @test_throws focused on the exception type is therefore basically a mis-feature, because it should really be testing the thing we actually care about rather than the internals of the mechanism used to achieve it. This PR allows one to focus on what actually matters.

@vtjnash, I normally wouldn't ask you to review something so simple as this, but in light of the scary bugs you and Jeff were squashing in #37809 and #38127, can I ask you to take a peek and make sure I haven't broken anything?

NEWS.md Outdated Show resolved Hide resolved
stdlib/Test/src/Test.jl Outdated Show resolved Hide resolved
stdlib/Test/src/Test.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Aug 14, 2021

You might need to also edit the serialize methods to preserve this type too. Or use an extra bool field instead of branching on type?

With this PR,

    @test_throws "reducing over an empty collection" reduce(+, ())

allows you to check the displayed error message without caring about
the details of which specific Exception subtype is used.

The pattern-matching options are the same as for `@test_warn`.
@tkf
Copy link
Member

tkf commented Aug 15, 2021

Are these (arguably very wacky) cases still supported?

julia> @test_throws "\"" throw("\"")
Test Passed
      Thrown: String

julia> @test_throws Returns(false) throw(Returns(false))
Test Passed
  Expression: throw(Returns(false))
      Thrown: Returns{Bool}

Not sure if they should be working, but maybe good edge cases to test against?

(The functionality added by this PR looks very useful BTW 👍)

stdlib/Test/src/Test.jl Outdated Show resolved Hide resolved
stdlib/Test/src/Test.jl Outdated Show resolved Hide resolved
stdlib/Test/src/Test.jl Outdated Show resolved Hide resolved
stdlib/Test/src/Test.jl Outdated Show resolved Hide resolved
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

LGTM!

@timholy
Copy link
Member Author

timholy commented Aug 17, 2021

Are these (arguably very wacky) cases still supported?

julia> @test_throws "\"" throw("\"")
Test Passed
  Expression: throw("\"")
     Message: "\"\\\"\""

julia> @test_throws Returns(false) throw(Returns(false))
Test Failed at REPL[6]:1
  Expression: throw(Returns(false))
    Expected: < match function >
     Message: "Returns{Bool}(false)"
ERROR: There was an error during testing

So the first yes, the second no. But is the second actually desirable?

julia> throw(()->startswith("cat", "nope"))
ERROR: var"#11#12"()
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1

and since most people will probably use the functional form anonymously, I think we'd get lots of opaque error messages.

To help me wrap my head around this, can you give me an example of a use of throwing something besides an Exception?

@timholy
Copy link
Member Author

timholy commented Aug 17, 2021

Nvm, it was pretty easy to get both of those to pass: I made this logic conditional on the thrown object being an Exception but the match object not being an exception or exception type.

Co-authored by: Jameson Nash <vtjnash@gmail.com>
Co-authored by: Takafumi Arakaki <aka.tkf@gmail.com>
@timholy
Copy link
Member Author

timholy commented Aug 17, 2021

What's up with all the buildkite failures? Is this something I broke? I'm seeing similar failures in some other jobs that ran recently.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Aug 17, 2021
@timholy timholy merged commit a03392a into master Aug 17, 2021
@timholy timholy deleted the teh/test_exc branch August 17, 2021 18:53
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Aug 25, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
With this PR,

    @test_throws "reducing over an empty collection" reduce(+, ())

allows you to check the displayed error message without caring about
the details of which specific Exception subtype is used.

The pattern-matching options are the same as for `@test_warn`.

Co-authored by: Jameson Nash <vtjnash@gmail.com>
Co-authored by: Takafumi Arakaki <aka.tkf@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
With this PR,

    @test_throws "reducing over an empty collection" reduce(+, ())

allows you to check the displayed error message without caring about
the details of which specific Exception subtype is used.

The pattern-matching options are the same as for `@test_warn`.

Co-authored by: Jameson Nash <vtjnash@gmail.com>
Co-authored by: Takafumi Arakaki <aka.tkf@gmail.com>
@ericphanson
Copy link
Contributor

Would be nice to have a compat annotation for this feature in the @test_throws docstring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants