Fix swallowed exception in Request.classes(...)

[acanda]

The method classes(Computer computer, Class<?>... classes) in
org.junit.runner.Request swallows an InitializationError and throws a
new RuntimeException instead when it cannot create a request. This
prevents the user from finding out why the request could not be created.

This change replaces throwing a new RuntimeException with generating a
proper error report. The report contains the causes of the
InitializationError so the user has a chance to find out what is causing
the problem.

Instead of an incomplete stack trace with the message "Bug in saff's brain: Suite constructor, called as above, should always complete" like

java.lang.reflect.InvocationTargetException
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    <snip>
Caused by: java.lang.RuntimeException: Bug in saff's brain: Suite constructor, called as above, should always complete
    at org.junit.runner.Request.classes(Request.java:72)
    at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.createRequestAndRun(JUnitCoreWrapper.java:102)
    at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.executeEager(JUnitCoreWrapper.java:85)
    at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:54)
    ... 27 more

the user gets an error report like

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running ch.acanda.eclipse.pmd.unsupported.ProjectPropertyTesterTest
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.002 sec <<< FAILURE! - in ch.acanda.eclipse.pmd.unsupported.ProjectPropertyTesterTest
initializationError(ch.acanda.eclipse.pmd.unsupported.ProjectPropertyTesterTest)  Time elapsed: 0.002 sec  <<< ERROR!
org.apache.maven.surefire.testset.TestSetFailedException: Unspecified thread-count(s). See the parameters useUnlimitedThreads, threadcount, threadcountsuites, threadcountclasses, threadcountmethods.
    at org.apache.maven.surefire.junitcore.pc.ParallelComputerUtil.resolveConcurrency(ParallelComputerUtil.java:69)
    at org.apache.maven.surefire.junitcore.pc.ParallelComputerBuilder$PC.determineThreadCounts(ParallelComputerBuilder.java:312)
    at org.apache.maven.surefire.junitcore.pc.ParallelComputerBuilder$PC.getSuite(ParallelComputerBuilder.java:272)
    at org.junit.runner.Request.classes(Request.java:75)
    <snip>
    at org.eclipse.equinox.launcher.Main.main(Main.java:1386)


Results :

Tests in error: 
  Unspecified thread-count(s). See the parameters useUnlimitedThreads, threadcount, threadcountsuites, threadcountclasses, threadcountmethods.

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

[acanda]

PS: if you came here because you just updated Tycho to version 0.22, your tests don't run any more and you googled java.lang.RuntimeException: Bug in saff's brain: Suite constructor, called as above, should always complete because this was the only hint you have, here's the solution to your problem: you are executing your tests in parallel but you did not specify the number of threads JUnit should use. You need to set one of the parameters useUnlimitedThreads, threadcount, threadcountsuites, threadcountclasses, threadcountmethods in the configuration of your tycho-surefire-plugin, e.g.

            <plugin>
                <groupId>org.eclipse.tycho</groupId>
                <artifactId>tycho-surefire-plugin</artifactId>
                <version>${tycho-version}</version>
                <configuration>
                    <parallel>classes</parallel>
                    <perCoreThreadCount>true</perCoreThreadCount>
                    <threadCount>1</threadCount>
                 </configuration>
            </plugin>

[kcooney]

Could you include in this pull a test that calls Request.classes() that triggers this exception? It sounds like David Saff didn't expect us to get to this throw statement

[acanda]

I added a test that triggers the exception when calling Request.classes(...) and asserts that the run listener is notified with a failure instead of throwing a RuntimeException.

[stefanbirkner]

LGTM

[kcooney]

This is an externally visible change for the case where the existing constructor is used.

Instead of making suggestions here, I'm going to create a pull with your commits and one additional commit to fix this and to fix some other minor issues.

The other pull is just to make reviewing my proposed changes easier; I still plan on merging your pull. Details to follow soon.

[kcooney]

I patched your branch into my repository, added a match to fix the backwards compatibility issue. You can view it at #1072

Note there isn't much tests for the changes to ErrorReportingRunner; I'll add them if you guys think this is the right direction to take.

It's a bit more complicated than I would like. I'm tempted to say that we should not change ErrorReportingRunner at all, and have Request.classes() pass in Request.class as the class.

If you like the direction, but want to make some changes, feel free to pull from my branch back to yours. To make reviewing simpler, I suggest not squashing commits for now.

[kcooney]

@acanda can you comment on pull #1072 please?

[acanda]

@kcooney your changes look similar to something I also had but though to be too complicated ;-)
I just looked at your changes on github as I am a bit busy at the moment. I have more time later this week. Maybe we find a less complicated way without the regression issue.
Generally though I'm happy as long as the exception is reported somewhere.

[kcooney]

@acanda as I stated before, I wasn't excited about the complexity of my changes either. That's why I was wondering whether we really need to include all of the classes passed to Request.classes() in the top-level Description for the case we get an exception trying to create a runner.

[stefanbirkner]

This pull request has been continued in #1072.