From b6706ab9b43d501b8340b62947645f617e3a0c5a Mon Sep 17 00:00:00 2001 From: Nora Howard Date: Tue, 29 Jan 2019 14:45:01 -0700 Subject: [PATCH] [junit-runner] extracted refactoring from sec mgr change (#6860) While working on https://github.com/pantsbuild/pants/pull/6203, I moved some things around that were not directly related to that change. These are those bits. This flattens some nested ifs and adds some helper fns to create places that #6203 can extend from. --- .../tools/junit/impl/ConsoleRunnerImpl.java | 60 ++++++----- .../tools/junit/impl/SpecParser.java | 27 +++-- .../org/pantsbuild/tools/junit/impl/BUILD | 1 - .../junit/impl/ConsoleRunnerImplTest.java | 102 +++++++++++++----- 4 files changed, 122 insertions(+), 68 deletions(-) diff --git a/src/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImpl.java b/src/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImpl.java index 06fd8a95962..6404d954807 100644 --- a/src/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImpl.java +++ b/src/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImpl.java @@ -40,6 +40,7 @@ import org.junit.runner.notification.RunListener; import org.junit.runner.notification.RunNotifier; import org.junit.runners.model.InitializationError; +import org.junit.runners.model.RunnerBuilder; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.CmdLineException; import org.kohsuke.args4j.CmdLineParser; @@ -518,8 +519,7 @@ private int runConcurrentTests(JUnitCore core, SpecSet specSet, Concurrency conc throws InitializationError { Computer junitComputer = new ConcurrentComputer(concurrency, parallelThreads); Class[] classes = specSet.extract(concurrency).classes(); - CustomAnnotationBuilder builder = - new CustomAnnotationBuilder(numRetries, swappableErr.getOriginal()); + RunnerBuilder builder = createCustomBuilder(swappableErr.getOriginal()); Runner suite = junitComputer.getSuite(builder, classes); return core.run(Request.runner(suite)).getFailureCount(); } @@ -533,26 +533,32 @@ private int runLegacy(Collection parsedTests, JUnitCore core) throws Initi if (this.parallelThreads > 1) { ConcurrentCompositeRequestRunner concurrentRunner = new ConcurrentCompositeRequestRunner( requests, this.defaultConcurrency, this.parallelThreads); - if (failFast) { - return core.run(new FailFastRunner(concurrentRunner)).getFailureCount(); - } else { - return core.run(concurrentRunner).getFailureCount(); - } + Runner runner = maybeWithFailFastRunner(concurrentRunner); + return core.run(runner).getFailureCount(); } int failures = 0; Result result; for (Request request : requests) { - if (failFast) { - result = core.run(new FailFastRunner(request.getRunner())); - } else { - result = core.run(request); - } + result = core.run(runnerFor(request)); failures += result.getFailureCount(); } return failures; } + private Runner runnerFor(Request request) { + Runner reqRunner = request.getRunner(); + return maybeWithFailFastRunner(reqRunner); + } + + private Runner maybeWithFailFastRunner(Runner runner) { + if (failFast) { + return new FailFastRunner(runner); + } else { + return runner; + } + } + private List legacyParseRequests(PrintStream err, Collection specs) { Set testMethods = Sets.newLinkedHashSet(); Set> classes = Sets.newLinkedHashSet(); @@ -570,15 +576,11 @@ private List legacyParseRequests(PrintStream err, Collection spec if (!classes.isEmpty()) { if (this.perTestTimer || this.parallelThreads > 1) { for (Class clazz : classes) { - if (legacyShouldRunParallelMethods(clazz)) { - if (ScalaTestUtil.isScalaTestTest(clazz)) { - // legacy and scala doesn't work easily. just adding the class - requests.add(new AnnotatedClassRequest(clazz, numRetries, err)); - } else { - testMethods.addAll(TestMethod.fromClass(clazz)); - } + // legacy doesn't support scala test tests, so those are run as classes. + if (legacyShouldRunParallelMethods(clazz) && !ScalaTestUtil.isScalaTestTest(clazz)) { + testMethods.addAll(TestMethod.fromClass(clazz)); } else { - requests.add(new AnnotatedClassRequest(clazz, numRetries, err)); + requests.add(createAnnotatedClassRequest(err, clazz)); } } } else { @@ -586,8 +588,7 @@ private List legacyParseRequests(PrintStream err, Collection spec // requests.add(Request.classes(classes.toArray(new Class[classes.size()]))); // does, except that it instantiates our own builder, needed to support retries. try { - CustomAnnotationBuilder builder = - new CustomAnnotationBuilder(numRetries, err); + RunnerBuilder builder = createCustomBuilder(err); Runner suite = new Computer().getSuite( builder, classes.toArray(new Class[classes.size()])); requests.add(Request.runner(suite)); @@ -598,12 +599,20 @@ private List legacyParseRequests(PrintStream err, Collection spec } } for (TestMethod testMethod : testMethods) { - requests.add(new AnnotatedClassRequest(testMethod.clazz, numRetries, err) + requests.add(createAnnotatedClassRequest(err, testMethod.clazz) .filterWith(Description.createTestDescription(testMethod.clazz, testMethod.name))); } return requests; } + private AnnotatedClassRequest createAnnotatedClassRequest(PrintStream err, Class clazz) { + return new AnnotatedClassRequest(clazz, numRetries, err); + } + + private RunnerBuilder createCustomBuilder(PrintStream original) { + return new CustomAnnotationBuilder(numRetries, original); + } + private boolean legacyShouldRunParallelMethods(Class clazz) { if (!Util.isRunnable(clazz)) { return false; @@ -799,10 +808,7 @@ public void setNumRetries(int numRetries) { CmdLineParser parser = new CmdLineParser(options); try { parser.parseArgument(args); - } catch (CmdLineException e) { - parser.printUsage(System.err); - exit(1); - } catch (InvalidCmdLineArgumentException e) { + } catch (CmdLineException | InvalidCmdLineArgumentException e) { parser.printUsage(System.err); exit(1); } diff --git a/src/java/org/pantsbuild/tools/junit/impl/SpecParser.java b/src/java/org/pantsbuild/tools/junit/impl/SpecParser.java index 93373ffb7cd..3b85ea78a16 100644 --- a/src/java/org/pantsbuild/tools/junit/impl/SpecParser.java +++ b/src/java/org/pantsbuild/tools/junit/impl/SpecParser.java @@ -75,16 +75,20 @@ Collection parse() throws SpecException { * @throws SpecException if the method passed in is not an executable test method */ private Optional getOrCreateSpec(String className, String specString) throws SpecException { - try { - Class clazz = getClass().getClassLoader().loadClass(className); - if (Util.isTestClass(clazz)) { - if (!specs.containsKey(clazz)) { - Spec newSpec = new Spec(clazz); - specs.put(clazz, newSpec); - } - return Optional.of(specs.get(clazz)); + Class clazz = loadClassOrThrow(className, specString); + if (Util.isTestClass(clazz)) { + if (!specs.containsKey(clazz)) { + Spec newSpec = new Spec(clazz); + specs.put(clazz, newSpec); } - return Optional.absent(); + return Optional.of(specs.get(clazz)); + } + return Optional.absent(); + } + + private Class loadClassOrThrow(final String className, String specString) { + try { + return getClass().getClassLoader().loadClass(className); } catch (ClassNotFoundException | NoClassDefFoundError e) { throw new SpecException(specString, String.format("Class %s not found in classpath.", className), e); @@ -94,9 +98,10 @@ private Optional getOrCreateSpec(String className, String specString) thro throw new SpecException(specString, String.format("Error linking %s.", className), e); // See the comment below for justification. - } catch (RuntimeException e) { + } catch (Exception e) { // The class may fail with some variant of RTE in its static initializers, trap these - // and dump the bad spec in question to help narrow down issue. + // and dump the bad spec in question along with the underlying error message to help + // narrow down issue. throw new SpecException(specString, String.format("Error initializing %s.",className), e); } diff --git a/tests/java/org/pantsbuild/tools/junit/impl/BUILD b/tests/java/org/pantsbuild/tools/junit/impl/BUILD index fd78fb579a0..c16884289b5 100644 --- a/tests/java/org/pantsbuild/tools/junit/impl/BUILD +++ b/tests/java/org/pantsbuild/tools/junit/impl/BUILD @@ -2,7 +2,6 @@ # Licensed under the Apache License, Version 2.0 (see LICENSE). junit_tests( - name='junit', dependencies=[ '3rdparty:guava', '3rdparty:junit', diff --git a/tests/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImplTest.java b/tests/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImplTest.java index 4681d5eb4fb..b53c3012b82 100644 --- a/tests/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImplTest.java +++ b/tests/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImplTest.java @@ -20,6 +20,7 @@ import org.junit.rules.TemporaryFolder; import org.junit.runner.Result; import org.junit.runner.notification.RunListener; +import org.junit.runner.notification.StoppedByUserException; import org.pantsbuild.tools.junit.lib.AllFailingTest; import org.pantsbuild.tools.junit.lib.AllIgnoredTest; import org.pantsbuild.tools.junit.lib.AllPassingTest; @@ -87,17 +88,49 @@ private void resetParameters() { useExperimentalRunner = false; } - private String runTest(Class testClass) { - return runTest(testClass, false); + private String runTestExpectingSuccess(Class testClass) { + return runTests( + Lists.newArrayList(testClass.getCanonicalName()), + false); } - private String runTest(Class testClass, boolean shouldFail) { - return runTests(Lists.newArrayList(testClass.getCanonicalName()), shouldFail); + private String runTestExpectingFailure(Class testClass) { + return runTests( + Lists.newArrayList(testClass.getCanonicalName()), + true + ); } - private String runTests(List tests, boolean shouldFail) { + private String runTests( + List tests, + boolean shouldFail + ) { + PrintStream originalOut = System.out; + PrintStream originalErr = System.err; + ByteArrayOutputStream outContent = new ByteArrayOutputStream(); - PrintStream outputStream = new PrintStream(outContent); + PrintStream outputStream = new PrintStream(outContent, true); + try { + return createAndRunConsoleRunner( + tests, + shouldFail, + originalOut, + outContent, + outputStream + ); + } finally { + System.setOut(originalOut); + System.setErr(originalErr); + } + } + + private String createAndRunConsoleRunner( + List tests, + boolean shouldFail, + PrintStream originalOut, + ByteArrayOutputStream outContent, + PrintStream outputStream + ) { // Clean log files for (File file : outdir.listFiles()) { @@ -119,17 +152,27 @@ private String runTests(List tests, boolean shouldFail) { numRetries, useExperimentalRunner, outputStream, - System.err); + System.err // TODO, if there's an error reported on system err, it doesn't show up in + // the test failures. + ); try { runner.run(tests); if (shouldFail) { - fail("Expected RuntimeException"); + fail("Expected RuntimeException.\n====stdout====\n" + outContent.toString()); } - } catch (RuntimeException e) { + } catch (StoppedByUserException e) { + // NB StoppedByUserException is used by the junit runner to cancel a test run for fail fast. if (!shouldFail) { throw e; } + } catch (RuntimeException e) { + boolean wasNormalFailure = e.getMessage() != null && + e.getMessage().contains("ConsoleRunner exited with status"); + if (!shouldFail || !wasNormalFailure) { + System.err.println("\n====stdout====\n" + outContent.toString()); + throw e; + } } try { @@ -142,12 +185,12 @@ private String runTests(List tests, boolean shouldFail) { @Test public void testFailFast() { failFast = false; - String output = runTest(AllFailingTest.class, true); + String output = runTestExpectingFailure(AllFailingTest.class); assertThat(output, containsString("There were 4 failures:")); assertThat(output, containsString("Tests run: 4, Failures: 4")); failFast = true; - output = runTest(AllFailingTest.class, true); + output = runTestExpectingFailure(AllFailingTest.class); assertThat(output, containsString("There was 1 failure:")); assertThat(output, containsString("Tests run: 1, Failures: 1")); } @@ -156,13 +199,13 @@ public void testFailFast() { public void testFailFastWithMultipleThreads() { failFast = false; parallelThreads = 8; - String output = runTest(AllFailingTest.class, true); + String output = runTestExpectingFailure(AllFailingTest.class); assertThat(output, containsString("There were 4 failures:")); assertThat(output, containsString("Tests run: 4, Failures: 4")); failFast = true; parallelThreads = 8; - output = runTest(AllFailingTest.class, true); + output = runTestExpectingFailure(AllFailingTest.class); assertThat(output, containsString("There was 1 failure:")); assertThat(output, containsString("Tests run: 1, Failures: 1")); } @@ -170,13 +213,13 @@ public void testFailFastWithMultipleThreads() { @Test public void testPerTestTimer() { perTestTimer = false; - String output = runTest(AllPassingTest.class); + String output = runTestExpectingSuccess(AllPassingTest.class); assertThat(output, containsString("....")); assertThat(output, containsString("OK (4 tests)")); assertThat(output, not(containsString("AllPassingTest"))); perTestTimer = true; - output = runTest(AllPassingTest.class); + output = runTestExpectingSuccess(AllPassingTest.class); assertThat(output, containsString( "org.pantsbuild.tools.junit.lib.AllPassingTest#testPassesOne")); @@ -193,7 +236,7 @@ public void testPerTestTimer() { @Test public void testOutputMode() { outputMode = ConsoleRunnerImpl.OutputMode.ALL; - String output = runTest(OutputModeTest.class, true); + String output = runTestExpectingFailure(OutputModeTest.class); assertThat(output, containsString("Output in classSetUp")); assertThat(output, containsString("Output in setUp")); assertThat(output, containsString("Output in tearDown")); @@ -210,7 +253,8 @@ public void testOutputMode() { assertThat(testLogContents, containsString("Output from passing test")); outputMode = ConsoleRunnerImpl.OutputMode.FAILURE_ONLY; - output = runTest(OutputModeTest.class, true); + output = runTestExpectingFailure(OutputModeTest.class); + assertThat(output, containsString("Output in classSetUp")); assertThat(output, containsString("Output in setUp")); assertThat(output, containsString("Output in tearDown")); @@ -227,7 +271,7 @@ public void testOutputMode() { assertThat(testLogContents, containsString("Output from passing test")); outputMode = ConsoleRunnerImpl.OutputMode.NONE; - output = runTest(OutputModeTest.class, true); + output = runTestExpectingFailure(OutputModeTest.class); assertThat(output, containsString("Output in classSetUp")); assertThat(output, not(containsString("Output in setUp"))); assertThat(output, not(containsString("Output in tearDown"))); @@ -247,21 +291,21 @@ public void testOutputMode() { @Test public void testOutputModeExceptionInBefore() { outputMode = ConsoleRunnerImpl.OutputMode.ALL; - String output = runTest(ExceptionInSetupTest.class, true); + String output = runTestExpectingFailure(ExceptionInSetupTest.class); assertThat(output, containsString("There was 1 failure:")); assertThat(output, containsString("java.lang.RuntimeException")); assertThat(output, containsString("Tests run: 0, Failures: 1")); assertThat(output, not(containsString("Test mechanism"))); outputMode = ConsoleRunnerImpl.OutputMode.FAILURE_ONLY; - output = runTest(ExceptionInSetupTest.class, true); + output = runTestExpectingFailure(ExceptionInSetupTest.class); assertThat(output, containsString("There was 1 failure:")); assertThat(output, containsString("java.lang.RuntimeException")); assertThat(output, containsString("Tests run: 0, Failures: 1")); assertThat(output, not(containsString("Test mechanism"))); outputMode = ConsoleRunnerImpl.OutputMode.NONE; - output = runTest(ExceptionInSetupTest.class, true); + output = runTestExpectingFailure(ExceptionInSetupTest.class); assertThat(output, containsString("There was 1 failure:")); assertThat(output, containsString("java.lang.RuntimeException")); assertThat(output, containsString("Tests run: 0, Failures: 1")); @@ -271,7 +315,7 @@ public void testOutputModeExceptionInBefore() { @Test public void testOutputModeTestSuite() { outputMode = ConsoleRunnerImpl.OutputMode.ALL; - String output = runTest(XmlReportTestSuite.class, true); + String output = runTestExpectingFailure(XmlReportTestSuite.class); assertThat(output, containsString("There were 2 failures:")); assertThat(output, containsString("Test output")); assertThat(output, containsString("Tests run: 5, Failures: 2")); @@ -289,19 +333,19 @@ public void testOutputModeIgnoredTest() { new File(outdir.getPath(), AllIgnoredTest.class.getCanonicalName() + ".out.txt"); outputMode = ConsoleRunnerImpl.OutputMode.ALL; - String output = runTest(AllIgnoredTest.class); + String output = runTestExpectingSuccess(AllIgnoredTest.class); assertThat(testSuiteLogFile.exists(), is(false)); assertThat(output, containsString("OK (0 tests)")); assertThat(output, not(containsString("testIgnore"))); outputMode = ConsoleRunnerImpl.OutputMode.FAILURE_ONLY; - output = runTest(AllIgnoredTest.class); + output = runTestExpectingSuccess(AllIgnoredTest.class); assertThat(testSuiteLogFile.exists(), is(false)); assertThat(output, containsString("OK (0 tests)")); assertThat(output, not(containsString("testIgnore"))); outputMode = ConsoleRunnerImpl.OutputMode.NONE; - output = runTest(AllIgnoredTest.class); + output = runTestExpectingSuccess(AllIgnoredTest.class); assertThat(testSuiteLogFile.exists(), is(false)); assertThat(output, containsString("OK (0 tests)")); assertThat(output, not(containsString("testIgnore"))); @@ -319,7 +363,7 @@ class AbortInTestRunFinishedListener extends RunListener { } ConsoleRunnerImpl.addTestListener(new AbortInTestRunFinishedListener()); - String output = runTest(AllPassingTest.class, true); + String output = runTestExpectingFailure(AllPassingTest.class); assertThat(output, containsString("OK (4 tests)")); assertThat(output, containsString("java.io.IOException: Bogus IOException")); } @@ -327,21 +371,21 @@ class AbortInTestRunFinishedListener extends RunListener { @Test public void testOutputAfterTestFinished() { outputMode = ConsoleRunnerImpl.OutputMode.ALL; - String output = runTest(LogOutputInTeardownTest.class); + String output = runTestExpectingSuccess(LogOutputInTeardownTest.class); assertThat(output, containsString("Output in tearDown")); assertThat(output, containsString("OK (3 tests)")); String testLogContents = getTestLogContents(LogOutputInTeardownTest.class, ".out.txt"); assertThat(testLogContents, containsString("Output in tearDown")); outputMode = ConsoleRunnerImpl.OutputMode.FAILURE_ONLY; - output = runTest(LogOutputInTeardownTest.class); + output = runTestExpectingSuccess(LogOutputInTeardownTest.class); assertThat(output, not(containsString("Output in tearDown"))); assertThat(output, containsString("OK (3 tests)")); testLogContents = getTestLogContents(LogOutputInTeardownTest.class, ".out.txt"); assertThat(testLogContents, containsString("Output in tearDown")); outputMode = ConsoleRunnerImpl.OutputMode.NONE; - output = runTest(LogOutputInTeardownTest.class); + output = runTestExpectingSuccess(LogOutputInTeardownTest.class); assertThat(output, not(containsString("Output in tearDown"))); assertThat(output, containsString("OK (3 tests)")); testLogContents = getTestLogContents(LogOutputInTeardownTest.class, ".out.txt");