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 stackoverflow in ArgumentsAreDifferent reporting #2292

Merged
merged 1 commit into from May 11, 2021

Conversation

TimvdLippe
Copy link
Contributor

When using Mockito without opentest4j, reporting an
ArgumentsAreDifferent exception would throw a StackOverflowError when
attempting to obtain the message from the exception.

The root problem was that super.toString() would call its own
getMessage(). Instead, we should obtain the message from the super, to
avoid the circular call.

When using Mockito without opentest4j, reporting an
ArgumentsAreDifferent exception would throw a StackOverflowError when
attempting to obtain the message from the exception.

The root problem was that super.toString() would call its own
getMessage(). Instead, we should obtain the message from the super, to
avoid the circular call.
@TimvdLippe
Copy link
Contributor Author

Note that I tried to create a regression test for this, but sadly was unable to. To make the integration test work, we would need to exclude both opentest4j and junit4. While that would technically be possible, Gradle wouldn't be able to pick up a test that is not annotated with @Test. I also tried using JUnit5, but they have a transitive dependency on opentest4j. Any attempt to exclude the transitive dependency causes the Gradle task to crash. I have manually verified that removing

try {
Class.forName("org.opentest4j.AssertionFailedError");
theFactory = org.mockito.exceptions.verification.opentest4j.ArgumentsAreDifferent::new;
} catch (ClassNotFoundException onlyIfOpenTestIsNotAvailable) {
try {
Class.forName("junit.framework.ComparisonFailure");
theFactory = org.mockito.exceptions.verification.junit.ArgumentsAreDifferent::new;
} catch (ClassNotFoundException onlyIfJUnitIsNotAvailable) {
}
}
and running BasicVerificationTest would reproduce the issue and that the fix is making the test pass as expected.

Copy link
Member

@mockitoguy mockitoguy left a comment

Choose a reason for hiding this comment

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

Should we get rid of removeFirstLine? Otherwise we might be losing first line of the message.

@TimvdLippe
Copy link
Contributor Author

Are all of our implementations are doing this:

and

I am not sure what the effect of the removal is. Can we separate that change out, to unblock non-opentest4j users and figure out what the impact of removing removeFirstLine will be?

@mockitoguy
Copy link
Member

Ok, let's move on. If all tests pass with your change, we should be good to go.

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.

None yet

2 participants