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

fix intercept for AssertionError #683

Merged
merged 3 commits into from
Apr 16, 2024
Merged

fix intercept for AssertionError #683

merged 3 commits into from
Apr 16, 2024

Conversation

mzuehlke
Copy link
Contributor

@mzuehlke mzuehlke commented Aug 1, 2023

When trying to intercept an AssertionError till now the test passed successfully.
This fix treats the FailException special.

closes: #679

When trying to `intercept`and `AssertionError` till now the test passed successfully.
This fix treats the FailException special.
@mzuehlke
Copy link
Contributor Author

mzuehlke commented Jan 2, 2024

Is there a reason that this pull request hasn't been merged, yet ?

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Sorry! I wasn't super sure about the changes and was hoping for someone more knowledgeable to come by. I double checked and rethought the PR to offer some suggestions.

tests/shared/src/test/scala/munit/FailExceptionSuite.scala Outdated Show resolved Hide resolved
Comment on lines 199 to 200
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

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.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgodzik tgodzik merged commit 51a37a2 into scalameta:main Apr 16, 2024
9 checks passed
@mzuehlke mzuehlke deleted the bug/679 branch April 16, 2024 09:49
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.

intercept[AssertionError] should fail if no AssertionError is thrown
3 participants