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

Undocumented change to ExpectedException in JUnit 4.11 changes default behavior when expected class is AssertionError. #687

Closed
xrisfg opened this issue May 31, 2013 · 4 comments

Comments

@xrisfg
Copy link

xrisfg commented May 31, 2013

Prior to JUnit 4.11, a test like this passes:

@rule
public ExpectedException thrown = ExpectedException.none();

@test
public void test() {
thrown.expect(AssertionError.class);
assertTrue(false);
}

With JUnit 4.11, tests like this suddenly fail with the thrown AssertionError, instead of being intercepted and verified by ExpectedException.

On inspecting the code, we see that the behavior of ExpectedException was changed to conditionally include AssertionErrors as a valid Throwable,and that the default behavior was changed from prior versions to ignore AssertionError. The flag handleAssertionErrors defaults to false; a call to the new handleAssertionErrors() method is needed to force it to true. This call is required even whenAssertionError is explicitly expected in the call to expect().

We have test frameworks that throw AssertionErrors, and unit tests for those frameworks that verify that the expected AssertIionErrors are thrown. All those tests fail with JUnit 4.11, unless the tests are changed to force handleAssertionErrors=true.

  1. This change in the default behavior should be documented in the JUnit 4.11 Release Notes. We spent considerable time tracking down why our test framework unit tests were suddenly failing.
  2. The behavior should be fixed, either by restoring the previous default behavior (which presumably was changed to fix some other problem?), or to set handleAssertionErrors=true when AssertionError is explicitly given as the expected class.
@marcphilipp
Copy link
Member

This change has been introduced in #323.

@marcphilipp
Copy link
Member

It seems there was some controversy about the default behaviour for assertion errors:
#323 (comment)

@dsaff @stefanbirkner Do you remember the details?

@xrisfg
Copy link
Author

xrisfg commented May 31, 2013

Reading back to #323 and #121, it seems the originally stated problem was that ExpectedException was wrapping thrown Assertion and Assumption violations, so that the original exception was masked. However, in the examples I can find there, in no case was ExpectedException told to expect an AssertionError or AssumptionViolationException.

This comment by @dsaff comes closest to describing the current issue:

#323 (comment)

"It's troubling that this test would fail:

@test public void throwExpectedAssertionError() {
thrown.expect(AssertionError.class);
throw new AssertionError();
}"

The implementation fixes one problem, but introduces another.

@dsaff
Copy link
Member

dsaff commented Jun 3, 2013

Ugh. Agreed that this is a bug. Sorry.

Could be low-hanging fruit for a contributor with free time. We should block 4.12 on a fix.

stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Aug 6, 2013
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Aug 6, 2013
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Aug 6, 2013
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Aug 17, 2013
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Aug 17, 2013
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Aug 17, 2013
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Aug 17, 2013
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Sep 3, 2013
about handling assumes and asserts in conjunction with the
ExpectedException rule.
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Sep 3, 2013
about handling assumes and asserts in conjunction with the
ExpectedException rule.
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Sep 3, 2013
Still fixes junit-team#121, but the original fix was superfluous. Added documentation
about handling assumes and asserts in conjunction with the
ExpectedException rule.
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Sep 3, 2013
Still fixes junit-team#121, but the original fix was superfluous. Added documentation
about handling assumes and asserts in conjunction with the
ExpectedException rule.
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Oct 16, 2013
Still fixes junit-team#121, but the original fix was superfluous. Added documentation
about handling assumes and asserts in conjunction with the
ExpectedException rule.
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Oct 16, 2013
Still fixes junit-team#121, but the original fix was superfluous. Added documentation
about handling assumes and asserts in conjunction with the
ExpectedException rule.
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Oct 16, 2013
Still fixes junit-team#121, but the original fix was superfluous. Added documentation
about handling assumes and asserts in conjunction with the
ExpectedException rule.
@dsaff dsaff closed this as completed in bb13b31 Oct 16, 2013
dsaff pushed a commit that referenced this issue Oct 16, 2013
Extend ExpectedException's documentation and fix #687.
stefanbirkner added a commit to stefanbirkner/junit that referenced this issue Jan 23, 2014
Still fixes junit-team#121, but the original fix was superfluous. Added documentation
about handling assumes and asserts in conjunction with the
ExpectedException rule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants