Provide better feedback to the user in case of invalid test classes by alb-i986 · Pull Request #1286 · junit-team/junit4

alb-i986 · 2016-04-29T17:57:32Z

Only one exception per invalid test class is now fired, rather than one per validation error, with all of the validation errors as part of its message.
Example:

org.junit.runners.InvalidTestClassError: Invalid test class 'InvalidTest':
  1. Method staticAfterMethod() should not be static
  2. Method staticBeforeMethod() should not be static

Validation errors for the same test class now count only once in the failure count (Result#getFailureCount).

Implementation:

ParentRunner#validate throws InvalidTestClassError in case of validation errors
ErrorReportingRunner fires the InvalidTestClassError above without unpacking the causes

Follows how this change looks like in build tools.

Gradle 2.13

Before this change:

Invalid1Test > initializationError FAILED
    java.lang.Exception: Method staticAfterMethod() should not be static

Invalid1Test > initializationError FAILED
    java.lang.Exception: Method staticBeforeMethod() should not be static

Invalid2Test > initializationError FAILED
    java.lang.Exception: Method staticAfterMethod() should not be static

Invalid2Test > initializationError FAILED
    java.lang.Exception: Method staticBeforeMethod() should not be static

ValidTest > asd FAILED
    java.lang.AssertionError
        at ValidTest.asd(ValidTest.java:12)

5 tests completed, 5 failed
:test FAILED

After this change:

Invalid1Test > initializationError FAILED
    org.junit.runners.InvalidTestClassError: Invalid test class 'Invalid1Test':
      1. Method staticAfterMethod() should not be static
      2. Method staticBeforeMethod() should not be static

Invalid2Test > initializationError FAILED
    org.junit.runners.InvalidTestClassError: Invalid test class 'Invalid2Test':
      1. Method staticAfterMethod() should not be static
      2. Method staticBeforeMethod() should not be static

ValidTest > asd FAILED
    java.lang.AssertionError
        at ValidTest.asd(ValidTest.java:12)

3 tests completed, 3 failed
:test FAILED

maven-surefire-plugin 2.19.1 (on maven 3.3.9)

Before:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running Asd2Test
Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.809 sec <<< FAILURE! - in Asd2Test
initializationError(Asd2Test)  Time elapsed: 0.098 sec  <<< ERROR!
java.lang.Exception: Method staticAfterMethod() should not be static

initializationError(Asd2Test)  Time elapsed: 0.01 sec  <<< ERROR!
java.lang.Exception: Method staticBeforeMethod() should not be static

Running AsdTest
Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.001 sec <<< FAILURE! - in AsdTest
initializationError(AsdTest)  Time elapsed: 0 sec  <<< ERROR!
java.lang.Exception: Method staticAfterMethod() should not be static

initializationError(AsdTest)  Time elapsed: 0 sec  <<< ERROR!
java.lang.Exception: Method staticBeforeMethod() should not be static

Running WorkingTest
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.001 sec <<< FAILURE! - in WorkingTest
asd(WorkingTest)  Time elapsed: 0.001 sec  <<< FAILURE!
java.lang.AssertionError: asd
    at WorkingTest.asd(WorkingTest.java:12)


Results :

Failed tests: 
  WorkingTest.asd:12 asd
Tests in error: 
Asd2Test.initializationError(Asd2Test)
  Run 1: Asd2Test.initializationError »  Method staticAfterMethod() should not be stati...
  Run 2: Asd2Test.initializationError »  Method staticBeforeMethod() should not be stat...

AsdTest.initializationError(AsdTest)
  Run 1: AsdTest.initializationError »  Method staticAfterMethod() should not be static
  Run 2: AsdTest.initializationError »  Method staticBeforeMethod() should not be stati...


Tests run: 3, Failures: 1, Errors: 2, Skipped: 0

After:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running Asd2Test
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.29 sec <<< FAILURE! - in Asd2Test
initializationError(Asd2Test)  Time elapsed: 0.095 sec  <<< ERROR!
org.junit.runners.InvalidTestClassError: 
Invalid test class 'Asd2Test':
  1. Method staticAfterMethod() should not be static
  2. Method staticBeforeMethod() should not be static

Running AsdTest
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.001 sec <<< FAILURE! - in AsdTest
initializationError(AsdTest)  Time elapsed: 0.001 sec  <<< ERROR!
org.junit.runners.InvalidTestClassError: 
Invalid test class 'AsdTest':
  1. Method staticAfterMethod() should not be static
  2. Method staticBeforeMethod() should not be static

Running WorkingTest
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0 sec <<< FAILURE! - in WorkingTest
asd(WorkingTest)  Time elapsed: 0 sec  <<< FAILURE!
java.lang.AssertionError: asd
    at WorkingTest.asd(WorkingTest.java:12)


Results :

Failed tests: 
  WorkingTest.asd:12 asd
Tests in error: 
  Asd2Test.initializationError » InvalidTestClass Invalid test class 'Asd2Test':...
  AsdTest.initializationError » InvalidTestClass Invalid test class 'AsdTest':
 ...

Tests run: 3, Failures: 1, Errors: 2, Skipped: 0

Please note that maven-surefire-plugin 2.19.1 is already smart enough to compact the different validation errors for the same test class together, in the Results section. With the change I'm proposing here, build tools won't need this extra logic.

Only one exception per invalid test class is now fired, rather than one per validation error, with all of the validation errors as part of its message. Example: org.junit.runners.InvalidTestClassError: Invalid test class 'InvalidTest': 1. Method staticAfterMethod() should not be static 2. Method staticBeforeMethod() should not be static Validation errors for the same test class now count only once in the failure count (Result#getFailureCount). Implementation: - ParentRunner#validate throws InvalidTestClassError in case of validation errors - ErrorReportingRunner fires the InvalidTestClassError above without unpacking the causes

alb-i986 · 2016-04-29T18:01:00Z

+    }
+
+    @Test
+    public void givenInvalidTestClass_integrationTest() {


Please note the integration test here. I'm not sure if this is a good place for it?

This is fine. Thanks!

kcooney · 2016-04-29T22:40:37Z

I really like this. @junit-team/junit-committers any thoughts?

marcphilipp · 2016-05-02T10:07:33Z

👍

stefanbirkner · 2016-05-03T21:39:52Z

        assertThat(
                testResult(DataPointFieldsMustBeStatic.class),
-                CoreMatchers.<PrintableResult>both(failureCountIs(2))
+                CoreMatchers.<PrintableResult>both(failureCountIs(1))


Verifying the failure count does not make sense anymore. I think it should be removed. @junit-team/junit-committers What do you think?

@stefanbirkner only at line 123 or do you mean all of the tests asserting on result.failureCount?

I've applied this to some other tests where it made sense. See d5fc61c.
Please take a look @stefanbirkner
Thanks

stefanbirkner · 2016-05-03T21:41:57Z

+1

alb-i986 · 2016-05-13T02:30:45Z

Should be ready to be squashed and then merged.

marcphilipp · 2016-05-14T11:33:45Z

+        sb.append(String.format("Invalid test class '%s':", testClass.getName()));
+        int i = 1;
+        for (Throwable error : validationErrors) {
+            sb.append("\n  " + i++ + ". " + error.getMessage());


I think "\n " + (i++) + ". " would be more readable.

True, thanks

alb-i986 · 2016-05-17T14:20:02Z

Fixed conflicts with master

marcphilipp · 2016-05-19T13:38:39Z

@@ -0,0 +1,43 @@
+package org.junit.runners;


I think this class should be in the org.junit.runners.model package (with InitializationError etc.).

marcphilipp · 2016-05-19T13:39:56Z

@alb-i986 I found another minor thing. Sorry!

alb-i986 · 2016-05-19T14:11:06Z

@marcphilipp done

marcphilipp · 2016-05-19T15:15:22Z

Thanks, looks good to me!

@junit-team/junit-committers Please take a look. :-)

… well!

kcooney · 2016-05-20T01:38:43Z

LGTM

marcphilipp · 2016-05-31T17:25:36Z

+package org.junit.runners.model;
+
+import org.junit.Test;
+import org.junit.runners.model.InvalidTestClassError;


This import is unused now, isn't it?

Right, thanks! Fixed now

marcphilipp · 2016-06-17T18:31:29Z

+public class InvalidTestClassError extends InitializationError {
+    private static final long serialVersionUID = 1L;
+
+    private final Class<?> testClass;


I don't think we have to store testClass since it's only used in the constructor.

Uhm ya right.

marcphilipp · 2016-06-23T18:54:31Z

@alb-i986 Thanks for your hard work! Please add a description of this improvement to the draft of the release notes.

marcphilipp · 2016-06-25T11:31:15Z

@alb-i986 Merging this broke the build on JDK 5. Can you please take a look? https://junit.ci.cloudbees.com/job/JUnit/78/console

alb-i986 · 2016-06-25T13:20:35Z

Sorry about that, should be fixed by #1333 . I'm on a mobile phone now so i can't do any m
ore than that.

How come Travis didn't catch that anyways?

alb-i986 · 2016-06-30T18:47:07Z

@marcphilipp

@alb-i986 Thanks for your hard work! Please add a description of this improvement to the draft of the release notes.

Done:

https://github.com/junit-team/junit4/wiki/4.13-Release-Notes#pull-request-1286-provide-better-feedback-to-the-user-in-case-of-invalid-test-classes

Please let me know if you're happy enough.
Thanks.

marcphilipp · 2016-07-04T19:03:48Z

Great, thanks!

alb-i986 reviewed Apr 29, 2016
View reviewed changes

stefanbirkner reviewed May 3, 2016
View reviewed changes

alb-i986 added 10 commits May 5, 2016 00:42

InvalidTestClassError: add javadoc

5c91a16

clean code: move method 'createMessage' close to its caller

93a315a

Verifying the failure count does not make sense anymore

4a6e166

revert refactoring: I'll handle that in junit-team#1285

fa8381b

improve unit tests

2c3a896

InvalidTestClassError: improve param names

45e06ba

fix unused import

ef98f6f

fix unused import

7e7787b

revert unrelated changes

4765762

fixup! Verifying the failure count does not make sense anymore

d5fc61c

marcphilipp reviewed May 14, 2016
View reviewed changes

alb-i986 added 3 commits May 17, 2016 15:12

improve readability

2147b76

fix star imports

e96a5a5

Merge branch 'master' into ErrorReportingRunner-provide-better-feedback

feec1be

marcphilipp reviewed May 19, 2016
View reviewed changes

mv InvalidTestClassError to org.junit.runners.model

800ad80

clanup: rm unused import

eedafa8

mv InvalidTestClassError to org.junit.runners.model: mv test class as…

5e80291

… well!

marcphilipp reviewed May 31, 2016
View reviewed changes

alb-i986 added 2 commits June 17, 2016 19:11

rm unused import

a1610b6

Merge branch 'master' into ErrorReportingRunner-provide-better-feedback

3c73d3e

marcphilipp reviewed Jun 17, 2016
View reviewed changes

no need for the test class to be part of the state

a227269

marcphilipp merged commit 0e4cb7e into junit-team:master Jun 23, 2016

alb-i986 deleted the ErrorReportingRunner-provide-better-feedback branch June 23, 2016 20:01

alb-i986 mentioned this pull request Jun 25, 2016

Fix broken build for jdk5 #1333

Merged

This was referenced Aug 14, 2020

Bump junit from 4.12 to 4.13 jmock-developers/jmock-library#182

Merged

Adapt to changed output in junit 4.13 jmock-developers/jmock-library#200

Merged

Uh oh!

Conversation

alb-i986 commented Apr 29, 2016

Gradle 2.13

maven-surefire-plugin 2.19.1 (on maven 3.3.9)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcooney commented Apr 29, 2016

Uh oh!

marcphilipp commented May 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanbirkner commented May 3, 2016

Uh oh!

alb-i986 commented May 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alb-i986 commented May 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcphilipp commented May 19, 2016

Uh oh!

alb-i986 commented May 19, 2016

Uh oh!

marcphilipp commented May 19, 2016

Uh oh!

kcooney commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcphilipp commented Jun 23, 2016

Uh oh!

marcphilipp commented Jun 25, 2016

Uh oh!

alb-i986 commented Jun 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alb-i986 commented Jun 30, 2016

Uh oh!

marcphilipp commented Jul 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alb-i986 commented Jun 25, 2016 •

edited

Loading