Exception handling of suite() method breaks Jenkins JUnit test history reporting

[k1w1m8]

My team is dynamically creating tests using the JUnit3/4 static suite method + TestSuite API

    public static junit.framework.Test suite() {
        TestSuite testSuite = new TestSuite(xxx);
        for (Test test: tests) {
            testSuite.addTest(yyy);
        }
        return testSuite;
    }

and then running this using Jenkins CI with maven, Surefire + JUnit 4.12.

There is a nasty failure pattern which periodically deletes the Jenkins test history - it resets the Age of all tests in Jenkins back to 1. The history / Age report in Jenkins is key for us as it reveals which commit caused the failure. We definitely do not want to lose that information.

The failure pattern goes like this:

• many tests are running fine, with some failures
• a commit is made, CI triggers, Jenkins runs surefire.
• This results in a problem inside the suite() method, which throws a RuntimeException. This is the dynamic test creation phase, before any tests are run.
• This results in Surefire reporting a successful run of a single "fake/synthetic" test which is reported as failed.
• a commit is made to fix the test creation phase, CI again triggeers, Jenkins runs surefire
• This results in many tests again running fine, with the same failures as before
• However, Jenkins now reports all the old failures from the first step as Age 1 - all the Age history is lost

The synthetic test failure looks like so:

 [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.064 s <<< FAILURE! - in com.company.package.MySuite
 [ERROR] initializationError(com.company.package.MySuite) Time elapsed: 0.019 s <<< ERROR!
 java.lang.RuntimeException: java.net.ConnectException: Connection refused (Connection refused)
 ...
 at com.company.package.MySuite.suite(MySuite.java:xx)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.lang.reflect.Method.invoke(Method.java:498)
 at org.junit.internal.runners.SuiteMethod.testFromSuiteMethod(SuiteMethod.java:35)
 at org.junit.internal.runners.SuiteMethod.<init>(SuiteMethod.java:24)
 at org.junit.internal.builders.SuiteMethodBuilder.runnerForClass(SuiteMethodBuilder.java:11)
 at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:59)
 at org.junit.internal.builders.AllDefaultPossibilitiesBuilder.runnerForClass(AllDefaultPossibilitiesBuilder.java:26)
 at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:59)
 at org.junit.internal.requests.ClassRequest.getRunner(ClassRequest.java:33)
 at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:362)
 at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:273)
 at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
 at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159)
 at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
 at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
 at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
 at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
 Caused by: java.net.ConnectException: Connection refused (Connection refused)
 ...
 ... 23 more

 [INFO]
 [INFO] Results:
 [INFO]
 [ERROR] Errors:
 [ERROR] MySuite.suite:xx » Runtime java.net.ConnectException: Connection refused (...
 [INFO]
 [ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0
 [INFO]
 [INFO] ------------------------------------------------------------------------
 [INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

There is no "initializationError" test in our source code.

It seems that the Jenkins JUnit report analysis gets completely confused when it does the history analysis in this case. Presumably it tries to compare

• the "many" passing and failing tests from run N-1 with
• the single fake test from run N

After that I presume it decides that those many N-1 tests have been deleted, and therefore deletes the history of those tests.

So it seems that if we value the history analysis, we need to keep the test names stable. If so, I suspect the fix is here is that JUnit should never create and pretend to run synthetic tests, at least in the case of dynamically create tests. Rather, the entire run should simply fail - I presume throw an exception out of junit / surefire back to maven?

Hopefully this would then prevent Jenkins from doing the test analysis and it will not break the history / Age reporting.

So, I did a bit of static analysis and checked the JUnit 4 code from github. There in RunnerBuilder, it does this

    public Runner safeRunnerForClass(Class<?> testClass) {
        try {
            Runner runner = runnerForClass(testClass);
            if (runner != null) {
                configureRunner(runner);
            }
            return runner;
        } catch (Throwable e) {
            return new ErrorReportingRunner(testClass, e);
        }
    }

Seems that the catch and then ErrorReportingRunner synthesizes the fake tests.

    private Description describeCause() {
        return Description.createTestDescription(classNames, "initializationError");
    }

I guess the underlying issue is that SuiteMethodBuilder overrides RunnerBuilder, but doesn't override safeRunnerForClass, which provides behaviour which is not safe for dynamically created suites run in Jenkins (i.e. generated fake fails in place of the actual generated tests).

Also this catch / ErrorReportingRunner pattern is spread out in quite a few different places.

Is it feasible to fix this old code for this particular case?

[k1w1m8]

I tried some local hacking via direct JUnit in IDEA.

I created local versions of JUnit 4 SuiteMethodBuilder and AllDefaultPossibilitiesBuilder and changed the catch from returning ErrorReportingRunner to throwing RuntimeException. This then gave a me "No tests were found" in IDEA UI, which is what I was looking for. I raised https://issues.apache.org/jira/browse/SUREFIRE-2144 for that.

However didnt work in Surefire, it seems it creates a slightly different synthetic test of its own (no "initiatizationError") if the exception makes it that far up. Sigh. So to fix this I would need to change at least three different catch clauses in two different projects. Sounds easy, right?

Starting to think it would be easier to complete the reporting of Surefire / JUnit5 dynamic tests rather than this. I raised https://issues.apache.org/jira/browse/SUREFIRE-2147 for that.

I guess thinking forward, there is better ROI over time for fixing JUnit5 than JUnit4, so if nobody else is interested, I may as well focus on a PR for SUREFIRE-2147.

[kcooney]

I don't see what fixes can be made in the JUnit side. If your suite() method throws an exception, JUnit cannot create "the actual generated tests"

I suggest changing your code so your suite() methods cannot throw exceptions. If you need to do any global setup before running the tests, use junit.extensions.TestSetup():

private class PossiblyFailingSetup extends TestSetup {
    @Override
    protected void setUp() throws Exception {
        // Do global setup
    }
}

public static junit.framework.Test suite() {
        TestSuite testSuite = new TestSuite(xxx);
        for (Test test: tests) {
            testSuite.addTest(yyy);
        }
        return new PossiblyFailingSetup(testSuite);
    }

Of course, in an ideal world, you would write your tests so that they can be run independently. Possibly PossiblyFailingSetup creates a temporary database, and each test checks if that database was already created, and if not, creates it.

[k1w1m8]

Thanks for your helpful reply.

I will have a look at preventing exceptions in the suite() method and see how that looks. Hopefully this would lead to maven surefure failing saying no tests were run, which I think would avoid the Jenkins reporting history issue,

Saying that, I don't think I did a good job of explaining the design issue in JUnit which this approach would be working around.

The issue here is the inappropriate use of ErrorReportingRunner with suite(). When a suite() method throws an exception, ErrorReportingRunner creates a single synthetic test and reports that if fails. This "JUnit created" synthetic test is what breaks the history of Jenkins JUnit reporting.

To make JUnit compatible with Jenkins JUnit reporting history, I believe that ErrorReportingRunner should not be used with suite(), so that any exception is passed back up to the caller (e.g. maven surefire) and no "JUnit created" synthetic test is reported as run.

[kcooney]

To make JUnit compatible with Jenkins JUnit reporting history, I believe that ErrorReportingRunner should not be used with suite(), so that any exception is passed back up to the caller (e.g. maven surefire) and no "JUnit created" synthetic test is reported as run.

When running tests, the JUnit4 runner intentionally does not throw exceptions. Instead, it reports exceptions to the listener.

[k1w1m8]

Apologies, let me try and make it clearer. The case I have is this:

• In surefire, I have defined a single suite using <test>org.whatever.MySuite</test>
• MySuite.suite() method throws a RuntimeException rather than returning a TestSuite, thus the number of tests created by the suite method is zero.
• JUnit is reporting that 1 test was run and it failed. It seems that this test was synthesized by ErrorReportingRunner when JUnit caught the RuntimeException.
• Jenkins JUnit reporting detects the 1 synthetic test, compares that to the previous run of N tests, and determines all the previous N tests were "deleted", and therefore deletes the history of those tests.

To avoid this issue, it seems that we should avoid synthesising failed tests in the JUnit error handling of suite(), i.e. avoid ErrorReportingRunner.

Also, I tried the workaround suggestion above. I caught and ignored exceptions in suite() method and returned an empty TestSuite. This works in surefire - it defaults failIfNoTests to true in my case, and fails the test run as expected. It doesn't work in failsafe due my use of <dependenciesToScan> not being compatible with <failIfNoTests> and https://issues.apache.org/jira/browse/SUREFIRE-1024.

[kcooney]

MySuite.suite() method throws a RuntimeException rather than returning a TestSuite, thus the number of tests created by the suite method is zero.

Suite methods should not throw exceptions.

If they do, JUnit will report them the same way it reports all exceptions: via the registered RunListener instances. That's why ErrorReportingRunner is used in this case.

Also, I tried the workaround suggestion above. I caught and ignored exceptions in suite() method and returned an empty TestSuite

That's not what I suggested.