Implement #[should_panic(expected = "...")] handling
#3293
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daxpedda
force-pushed
the
should-panic
branch
from
2ac981f
to
2357053
Compare
daxpedda
commented
Feb 9, 2023
daxpedda
force-pushed
the
should-panic
branch
from
2357053
to
77ce92d
Compare
ranile
reviewed
Feb 9, 2023
|
I can't find a place for failing tests. |
|
CI is failing for unrelated reasons. |
Liamolucko
reviewed
Feb 10, 2023
Comment on lines
+159
to
+175
| #[wasm_bindgen_test] | ||
| #[should_panic] | ||
| async fn should_panic() { | ||
| panic!() | ||
| } | ||
|
|
||
| #[wasm_bindgen_test] | ||
| #[should_panic = "error message"] | ||
| async fn should_panic_string() { | ||
| panic!("error message") | ||
| } | ||
|
|
||
| #[wasm_bindgen_test] | ||
| #[should_panic(expected = "error message")] | ||
| async fn should_panic_expected() { | ||
| panic!("error message") | ||
| } |
There was a problem hiding this comment.
Why are these tests a part of wasm-bindgen-futures's test suite?
There was a problem hiding this comment.
I was testing it with async functions to make sure that the parsing is correct.
daxpedda
force-pushed
the
should-panic
branch
3 times, most recently
from
3de85ce
to
542ba73
Compare
daxpedda
force-pushed
the
should-panic
branch
from
542ba73
to
d917ae4
Compare
Liamolucko
reviewed
Feb 10, 2023
daxpedda
force-pushed
the
should-panic
branch
from
490bc52
to
2804245
Compare
daxpedda
force-pushed
the
should-panic
branch
2 times, most recently
from
352238f
to
135f8b7
Compare
daxpedda
force-pushed
the
should-panic
branch
4 times, most recently
from
36b972a
to
b596082
Compare
daxpedda
force-pushed
the
should-panic
branch
from
b596082
to
76469b3
Compare
Liamolucko
reviewed
Feb 11, 2023
Co-Authored-By: Liam Murphy <43807659+Liamolucko@users.noreply.github.com>
daxpedda
force-pushed
the
should-panic
branch
from
16193f6
to
8e0a9d7
Compare
Liamolucko
added a commit
to Liamolucko/wasm-bindgen
that referenced
this pull request
Feb 16, 2023
Apparently, rustwasm#3293 didn't quite update them properly, causing CI to fail. I'm not sure why CI passed when checking the PR but failed on `main`, but this should fix the CI failure.
Closed
alexcrichton
pushed a commit
that referenced
this pull request
Feb 16, 2023
* Update expected results for UI tests Apparently, #3293 didn't quite update them properly, causing CI to fail. I'm not sure why CI passed when checking the PR but failed on `main`, but this should fix the CI failure. * Use updated trybuild It looks like trybuild moved where it puts its output, and so CI's up-to-date trybuild and my older local trybuild disagreed on what a path should be.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Catching panics was already in place, it was just a matter of properly parsing
should_panicand printing the correct error messages.I tried to stay as close as possible to the output Rust gives.
EDIT:
I found out later that there was no proper way to "only" get the panic message, so I just customized the panic hook to store any panic message in the thread local output we already had around.
Fixes #2286.