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.

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

fix intercept for AssertionError #683

Merged
merged 3 commits into from
Apr 16, 2024
Merged

fix intercept for AssertionError #683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1773ca8
fix intercept for AssertionError
mzuehlke Aug 1, 2023
30e97c3
Update tests/shared/src/test/scala/munit/FailExceptionSuite.scala
mzuehlke Apr 16, 2024
767a553
adopted suggestion from @tgodzik
mzuehlke Apr 16, 2024
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
9 changes: 5 additions & 4 deletions munit/shared/src/main/scala/munit/Assertions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,15 @@ trait Assertions extends MacroCompat.CompileErrorMacro {
expectedExceptionMessage: Option[String],
body: => Any
)(implicit T: ClassTag[T], loc: Location): T = {
val expectedExceptionMsg =
s"expected exception of type '${T.runtimeClass.getName()}' but body evaluated successfully"
try {
body
fail(
s"expected exception of type '${T.runtimeClass.getName()}' but body evaluated successfully"
)
fail(expectedExceptionMsg)
} catch {
case e: FailExceptionLike[_]
if !T.runtimeClass.isAssignableFrom(e.getClass()) =>
if !T.runtimeClass.isAssignableFrom(e.getClass()) ||
e.getMessage.contains(expectedExceptionMsg) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a separate case:

```suggestion
           case failed: FailException if  failed.getMessage() == expectedExceptionMsg  =>
             throw e

to be super sure? So that we only employ this logic if someone throws exactly the same error as we would. Then they're on their own

Copy link

Choose a reason for hiding this comment

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

I was caught by this AssertionError bug today, and found this fix. @tgodzik 's suggestion is good, but

failed.getMessage() == expectedExceptionMsg

need to be changed to

failed.getMessage.contains(expectedExceptionMsg)

Tested this and @mzuehlke 's original change, both worked.

Can this be merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally forgot about this, @mzuehlke what do you think about suggested changes? I am ok also merging as is, we could update the test case though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had forgotten about it, too. Sorry for that.
I adopted your suggestion.

throw e
case NonFatal(e) =>
if (T.runtimeClass.isAssignableFrom(e.getClass())) {
Expand Down
15 changes: 15 additions & 0 deletions tests/shared/src/test/scala/munit/FailExceptionSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,19 @@ class FailExceptionSuite extends BaseSuite {
}
assert(clue(e).isInstanceOf[Serializable])
}

test("assertion-error no exception") {
try {
intercept[AssertionError] {
println("throwing no exception!")
}
} catch {
case e: FailException =>
assert(
e.getMessage.contains(
"expected exception of type 'java.lang.AssertionError' but body evaluated successfully"
)
)
}
}
mzuehlke marked this conversation as resolved.
Show resolved Hide resolved
}