Fixes gh-121 (ExpectedException handles JUnit exceptions) #323

Merged
merged 5 commits into from Apr 9, 2012

Conversation

Projects
None yet
3 participants
Contributor

stefanbirkner commented Sep 21, 2011

ExpectedException rule no longer handles AssertionErrors and
AssumptionViolatedExceptions by default. This means that the
rule doesn't intercept if your test fails because of an violated
assertion or assumption.

If you want to test assertions or assumptions you have to tell
the rule to handle such exceptions via handleAssertionErrors()
or handleAssumptionViolatedExceptions().

@stefanbirkner stefanbirkner Fixes gh-121 (ExpectedException handles JUnit exceptions)
ExpectedException rule no longer handles AssertionErrors and
AssumptionViolatedExceptions by default. This means that the
rule doesn't intercept if your test fails because of an violated
assertion or assumption.

If you want to test assertions or assumptions you have to tell
the rule to handle such exceptions via handleAssertionErrors()
or handleAssumptionViolatedExceptions().
128553f

@dsaff dsaff and 1 other commented on an outdated diff Sep 22, 2011

...ava/org/junit/runner/notification/EventCollector.java
@@ -0,0 +1,58 @@
+package org.junit.runner.notification;
@dsaff

dsaff Sep 22, 2011

Owner

It looks like this class is added in two different places?

@stefanbirkner

stefanbirkner Sep 22, 2011

Contributor

Ooops, I remove this one.

@dsaff dsaff commented on the diff Sep 22, 2011

...t/tests/experimental/rules/ExpectedExceptionTest.java
+ public ExpectedExceptionTest(Class<?> classUnderTest,
+ Matcher<EventCollector> matcher) {
+ this.classUnderTest= classUnderTest;
+ this.matcher= matcher;
+ }
+
+ @Test
+ public void runTestAndVerifyResult() {
+ EventCollector collector= new EventCollector();
+ JUnitCore core= new JUnitCore();
+ core.addListener(collector);
+ core.run(classUnderTest);
+ assertThat(collector, matcher);
+ }
+
+ public static class EmptyTestExpectingNoException {
@dsaff

dsaff Sep 22, 2011

Owner

For years now, I've wanted to be able to write tests like this, but with a hitch: the matcher for results would be attached to the test class, like this:

public static class EmptyTestExpectingNoException {
public static Matcher<?> EXPECTED = everyTestRunSuccessful()

@rule public ExpectedException thrown= none();
@test public void throwsNothing() {}
}

Would you be willing and able to rewrite this test class in that style?

@stefanbirkner

stefanbirkner Sep 22, 2011

Contributor

I give it a try.

@Tibor17

Tibor17 Sep 23, 2011

Contributor

@david, Stefan

I think that we can the best if we use the Matcher.
I have reported IsThrowable matcher
https://github.com/KentBeck/junit/issues/322
which might be used.

I prefer a generic form rather than a concrete method names handleAssertionErrors(), handleAssumptionViolatedExceptions().

What about to use the rules like this

@Rule public ExpectedException thrown= new ExpectedException(Matcher);

We can use anyOf() with these matcher. The only thing is to implement IBlock with run() method which throws anything.

@dsaff

dsaff Sep 23, 2011

Owner

Tibor,

ExpectedException already has expect(Matcher). What are you proposing to use instead?

@stefanbirkner

stefanbirkner Nov 10, 2011

Contributor

Finally I created the JUnitSelfTestRunner. See #361.

@dsaff

dsaff Apr 9, 2012

Owner

Stefan,

Would you prefer we pull this change in unchanged, or should we finish #361, and use that for these tests here?

@stefanbirkner

stefanbirkner Apr 9, 2012

Contributor

I would prefer to pull this change in unchanged.

Contributor

Tibor17 commented Sep 25, 2011

Hi David,
In a test, where all test case are supposed to throw same, you have to use
@rule ee = EE.none().expect(Matcher)
because the expect() method is on instance. That's not much verbose form of expression.

Then the most simple form would be just
@rule ee = new EE(Matcher)

Any other consequent call to expect() makes the macher more narrow as before, which is ok.

It would be a formal change for the variable fMatcher been not null, with a small effort, IMHO.

Owner

dsaff commented Sep 26, 2011

Tibor17, I can see your feature request--it seems orthogonal to this pull request, however. Do you not think so?

Contributor

Tibor17 commented Sep 26, 2011

Well yes, that one feature req. really can not be applicable. What you think of the proposals regarding @rule ee = new EE(Matcher) ?

Owner

dsaff commented Sep 26, 2011

Tibor17,

I think that's a pretty good idea, but can be handled in another thread.

Owner

dsaff commented Sep 26, 2011

Stefan,

Looking back at this, it's troubling that this test would fail:

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

I think I would change the goals of this pull a bit:

  1. Don't handle AssertionErrors differently. The error message is a bit weird, but maybe we can handle that differently in a different pull.

  2. Handle AssumptionViolatedExceptions as you've implemented here, with one change: have handleAssumptionViolatedExceptions return this, so people can say:

@rule public void ExpectedException expect = ExpectedException.none().handleAssumptionViolatedExceptions();

Thoughts?

stefanbirkner added some commits Sep 27, 2011

@stefanbirkner stefanbirkner deleted duplicate EventCollector bfd54d6
@stefanbirkner stefanbirkner the two handle... methods are returning the rule
The two methods handleAssertionError() and
handleAssumptionViolatedExceptions() are returning the rule.

Now you can write
  @Rule
  public ExpectedException thrown = none().handleAssertionError();
or
  @Rule
  public ExpectedException thrown = none().handleAssumptionViolatedExceptions()

Thank you David, for this suggestion.
df70c10

@dsaff dsaff commented on the diff Feb 1, 2012

src/main/java/org/junit/rules/ExpectedException.java
@@ -120,17 +168,32 @@ public void evaluate() throws Throwable {
}
}
+ private void optionallyHandleException(Throwable e, boolean handleException)
+ throws Throwable {
+ if (handleException)
+ handleException(e);
+ else
+ throw e;
+
@dsaff

dsaff Feb 1, 2012

Owner

Please remove blank line.

@dsaff dsaff commented on an outdated diff Feb 1, 2012

...rg/junit/tests/experimental/rules/EventCollector.java
+
+import static org.hamcrest.core.IsEqual.equalTo;
+import static org.junit.matchers.JUnitMatchers.both;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Matcher;
+import org.junit.runner.Description;
+import org.junit.runner.Result;
+import org.junit.runner.notification.Failure;
+import org.junit.runner.notification.RunListener;
+
+class EventCollector extends RunListener {
+ private final List<Description> testRunsStarted= new ArrayList<Description>();
@dsaff

dsaff Feb 1, 2012

Owner

Please use f prefix. Sorry, it's the JUnit standard. :-/

@dsaff dsaff commented on an outdated diff Feb 1, 2012

...rg/junit/tests/experimental/rules/EventCollector.java
+ sb.append(" test runs finished, ");
+ sb.append(testsStarted.size());
+ sb.append(" tests started, ");
+ sb.append(testsFinished.size());
+ sb.append(" tests finished, ");
+ sb.append(failures.size());
+ sb.append(" failures, ");
+ sb.append(assumptionFailures.size());
+ sb.append(" assumption failures, ");
+ sb.append(testIgnored.size());
+ sb.append(" tests ignored");
+
+ return sb.toString();
+ }
+
+ static Matcher<EventCollector> everyTestRunSuccessful() {
@dsaff

dsaff Feb 1, 2012

Owner

Please move static Matcher factories to the top of the class.

@dsaff dsaff and 1 other commented on an outdated diff Feb 1, 2012

...rg/junit/tests/experimental/rules/EventCollector.java
+ sb.append(assumptionFailures.size());
+ sb.append(" assumption failures, ");
+ sb.append(testIgnored.size());
+ sb.append(" tests ignored");
+
+ return sb.toString();
+ }
+
+ static Matcher<EventCollector> everyTestRunSuccessful() {
+ return both(hasNoFailure()).and(hasNoAssumptionFailure());
+ }
+
+ private static Matcher<EventCollector> hasNumberOfFailures(
+ final int numberOfFailures) {
+ return new BaseMatcher<EventCollector>() {
+ public boolean matches(Object item) {
@dsaff

dsaff Feb 1, 2012

Owner

TypeSafeMatcher would simplify this a bit.

@dsaff

dsaff Apr 3, 2012

Owner

Did you decide not to use TypeSafeMatcher here?

@stefanbirkner

stefanbirkner Apr 3, 2012

Contributor

I was not accurate enough. Fixed it today.

Owner

dsaff commented Mar 6, 2012

Any progress on this pull? Thanks.

Contributor

stefanbirkner commented Mar 6, 2012

Applied your code review.

dsaff merged commit c4279e4 into junit-team:master Apr 9, 2012

Owner

dsaff commented Apr 9, 2012

Many thanks!

@stefanbirkner stefanbirkner added a commit to stefanbirkner/junit that referenced this pull request Aug 17, 2013

@stefanbirkner stefanbirkner Handle expected AssertionError and AssumptionViolatedException.
Fixes #687. #121 is still fixed. (Pull request #323 was too rigid.)
b6c581a

@stefanbirkner stefanbirkner added a commit to stefanbirkner/junit that referenced this pull request Aug 17, 2013

@stefanbirkner stefanbirkner Handle expected AssertionError and AssumptionViolatedException.
Fixes #687. #121 is still fixed. (Pull request #323 was too rigid.)
fc7bcdb

@stefanbirkner stefanbirkner added a commit to stefanbirkner/junit that referenced this pull request Aug 17, 2013

@stefanbirkner stefanbirkner Handle expected AssertionError and AssumptionViolatedException.
Fixes #687. #121 is still fixed. (Pull request #323 was too rigid.)
9fef81b

@stefanbirkner stefanbirkner added a commit to stefanbirkner/junit that referenced this pull request Aug 17, 2013

@stefanbirkner stefanbirkner Handle expected AssertionError and AssumptionViolatedException.
Fixes #687. #121 is still fixed. (Pull request #323 was too rigid.)
33553ab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment