From 901178b183e695cf8fa000641f14248929ee9cf6 Mon Sep 17 00:00:00 2001 From: Simon Mavi Stewart Date: Wed, 8 Nov 2023 13:00:25 +0000 Subject: [PATCH] Respond to review comments --- .gitignore | 2 + comparative-tests/README.md | 13 ++++++ ...rib_rules_jvm.comparative.DynamicTests.xml | 9 ++++ ...ules_jvm.comparative.ParameterisedTest.xml | 12 +++++ ...rules_jvm.comparative.PathologicalTest.xml | 9 ++++ ...ntrib_rules_jvm.comparative.SimpleTest.xml | 7 +++ ...ntrib_rules_jvm.comparative.SuiteTest1.xml | 10 +++++ ...ntrib_rules_jvm.comparative.SuiteTest2.xml | 8 ++++ .../gradle/wrapper/gradle-wrapper.properties | 2 +- ...terisedTest.java => PathologicalTest.java} | 4 +- ...rib_rules_jvm.comparative.DynamicTests.xml | 36 +++++++++++++++ ...ules_jvm.comparative.ParameterisedTest.xml | 45 +++++++++++++++++++ ...rules_jvm.comparative.PathologicalTest.xml | 36 +++++++++++++++ ...ntrib_rules_jvm.comparative.SimpleTest.xml | 34 ++++++++++++++ ...ntrib_rules_jvm.comparative.SuiteTest1.xml | 37 +++++++++++++++ ...ntrib_rules_jvm.comparative.SuiteTest2.xml | 34 ++++++++++++++ ...ntrib_rules_jvm.comparative.SuiteTests.xml | 33 ++++++++++++++ .../junit5/BazelJUnitOutputListener.java | 21 ++++++--- .../contrib_rules_jvm/junit5/README.md | 7 ++- 19 files changed, 350 insertions(+), 9 deletions(-) create mode 100644 comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.DynamicTests.xml create mode 100644 comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml create mode 100644 comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.PathologicalTest.xml create mode 100644 comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SimpleTest.xml create mode 100644 comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest1.xml create mode 100644 comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest2.xml rename comparative-tests/src/test/java/com/github/bazel_contrib/contrib_rules_jvm/comparative/{DisabledSkippedParameterisedTest.java => PathologicalTest.java} (86%) create mode 100644 comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.DynamicTests.xml create mode 100644 comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml create mode 100644 comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.PathologicalTest.xml create mode 100644 comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SimpleTest.xml create mode 100644 comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest1.xml create mode 100644 comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest2.xml create mode 100644 comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTests.xml diff --git a/.gitignore b/.gitignore index 61983805..b4037f1b 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,5 @@ bazel-* .bazelrc.user .idea .ijwb + +/comparative-tests/.gradle/ diff --git a/comparative-tests/README.md b/comparative-tests/README.md index 802b2127..d8371fb7 100644 --- a/comparative-tests/README.md +++ b/comparative-tests/README.md @@ -21,3 +21,16 @@ Tests logs may be found: * Maven: `comparative-tests/target/surefire-reports` * Gradle: `comparative-tests/build/test-results/test` * Bazel: `bazel-testlogs/comparative-tests/src/test/java/com/apple/sdp/gradle/` + + +## Updating `gradlew` + +Gradle projects typically ship with a `gradlew` script, and this directory +is no exception. By doing this, we avoid the need to make users install +`gradle` on their systems, but it does mean that we have seemingly random +files scattered around. + +To [update `gradlew`][gradlew] run the command: `./gradlew wrapper +--gradle-version latest` + +[gradlew]: https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:upgrading_wrapper \ No newline at end of file diff --git a/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.DynamicTests.xml b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.DynamicTests.xml new file mode 100644 index 00000000..0216ba0b --- /dev/null +++ b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.DynamicTests.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml new file mode 100644 index 00000000..bde5bfb0 --- /dev/null +++ b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml @@ -0,0 +1,12 @@ + + + + + + + + + diff --git a/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.PathologicalTest.xml b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.PathologicalTest.xml new file mode 100644 index 00000000..7a4a5254 --- /dev/null +++ b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.PathologicalTest.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SimpleTest.xml b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SimpleTest.xml new file mode 100644 index 00000000..57722694 --- /dev/null +++ b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SimpleTest.xml @@ -0,0 +1,7 @@ + + + + + + + diff --git a/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest1.xml b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest1.xml new file mode 100644 index 00000000..8d8b102a --- /dev/null +++ b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest1.xml @@ -0,0 +1,10 @@ + + + + + + + + diff --git a/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest2.xml b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest2.xml new file mode 100644 index 00000000..78e78638 --- /dev/null +++ b/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest2.xml @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/comparative-tests/gradle/wrapper/gradle-wrapper.properties b/comparative-tests/gradle/wrapper/gradle-wrapper.properties index ac72c34e..3fa8f862 100644 --- a/comparative-tests/gradle/wrapper/gradle-wrapper.properties +++ b/comparative-tests/gradle/wrapper/gradle-wrapper.properties @@ -1,6 +1,6 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-8.3-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-8.4-bin.zip networkTimeout=10000 validateDistributionUrl=true zipStoreBase=GRADLE_USER_HOME diff --git a/comparative-tests/src/test/java/com/github/bazel_contrib/contrib_rules_jvm/comparative/DisabledSkippedParameterisedTest.java b/comparative-tests/src/test/java/com/github/bazel_contrib/contrib_rules_jvm/comparative/PathologicalTest.java similarity index 86% rename from comparative-tests/src/test/java/com/github/bazel_contrib/contrib_rules_jvm/comparative/DisabledSkippedParameterisedTest.java rename to comparative-tests/src/test/java/com/github/bazel_contrib/contrib_rules_jvm/comparative/PathologicalTest.java index 9c954dc3..fb25c23b 100644 --- a/comparative-tests/src/test/java/com/github/bazel_contrib/contrib_rules_jvm/comparative/DisabledSkippedParameterisedTest.java +++ b/comparative-tests/src/test/java/com/github/bazel_contrib/contrib_rules_jvm/comparative/PathologicalTest.java @@ -10,7 +10,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; -class DisabledSkippedParameterisedTest { +class PathologicalTest { private static Stream argsProvider() { return Stream.of(Arguments.of("alpha"), Arguments.of("beta"), Arguments.of("gamma")); @@ -21,6 +21,6 @@ private static Stream argsProvider() { @MethodSource("argsProvider") public void bootstrap(String goGreek) { System.out.println(goGreek); - fail("This should be ignored if we are running in bazel"); + fail("This test should never be run because it's disabled"); } } diff --git a/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.DynamicTests.xml b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.DynamicTests.xml new file mode 100644 index 00000000..bc2e0f44 --- /dev/null +++ b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.DynamicTests.xml @@ -0,0 +1,36 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml new file mode 100644 index 00000000..002b0bea --- /dev/null +++ b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml @@ -0,0 +1,45 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.PathologicalTest.xml b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.PathologicalTest.xml new file mode 100644 index 00000000..5136cc84 --- /dev/null +++ b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.PathologicalTest.xml @@ -0,0 +1,36 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SimpleTest.xml b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SimpleTest.xml new file mode 100644 index 00000000..c2071986 --- /dev/null +++ b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SimpleTest.xml @@ -0,0 +1,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest1.xml b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest1.xml new file mode 100644 index 00000000..cc00b4db --- /dev/null +++ b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest1.xml @@ -0,0 +1,37 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest2.xml b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest2.xml new file mode 100644 index 00000000..8105a430 --- /dev/null +++ b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTest2.xml @@ -0,0 +1,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTests.xml b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTests.xml new file mode 100644 index 00000000..7c258451 --- /dev/null +++ b/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.SuiteTests.xml @@ -0,0 +1,33 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/BazelJUnitOutputListener.java b/java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/BazelJUnitOutputListener.java index 91b37416..8a6b577e 100644 --- a/java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/BazelJUnitOutputListener.java +++ b/java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/BazelJUnitOutputListener.java @@ -12,6 +12,8 @@ import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.xml.stream.XMLOutputFactory; @@ -25,6 +27,7 @@ import org.junit.platform.launcher.TestPlan; public class BazelJUnitOutputListener implements TestExecutionListener, Closeable { + public static final Logger LOG = Logger.getLogger(BazelJUnitOutputListener.class.getName()); private final XMLStreamWriter xml; private final Map results = new ConcurrentHashMap<>(); @@ -46,7 +49,7 @@ public void testPlanExecutionStarted(TestPlan testPlan) { this.testPlan = testPlan; try { - // Closed when we call `testPlanExecutionFinished + // Closed when we call `testPlanExecutionFinished` xml.writeStartElement("testsuites"); } catch (XMLStreamException e) { throw new RuntimeException(e); @@ -76,8 +79,10 @@ private Map> matchTestCasesToSuites(List test for (TestData testCase : testCases) { TestData parent = testCase.getId().getParentIdObject().map(results::get).orElse(null); if (parent == null) { - // We should really log this, because something has gone wildly wrong - continue; + // Something has gone wildly wrong. I really hope people file some + // bugs with us if they run into this.... + LOG.warning("Unable to find parent test for " + testCase.getId()); + throw new IllegalStateException("Unable to find parent test for " + testCase.getId()); } knownSuites.computeIfAbsent(parent, id -> new ArrayList<>()).add(testCase); @@ -142,7 +147,13 @@ private void outputIfTestRootIsComplete(TestIdentifier testIdentifier) { throw new RuntimeException(e); } - // Delete the results we've used to conserve memory + // Delete the results we've used to conserve memory. This is safe to do + // since we only do this when the test root is complete, so we know that + // we won't be adding to the list of suites and test cases for that root + // (because tests and containers are arranged in a hierarchy --- the + // containers only complete when all the things they contain are + // finished. We are leaving all the test data that we have _not_ written + // to the XML file. Stream.concat(testCases.stream(), testSuites.keySet().stream()) .forEach(data -> results.remove(data.getId().getUniqueIdObject())); } @@ -161,7 +172,7 @@ public void close() { xml.writeEndDocument(); xml.close(); } catch (XMLStreamException e) { - // LOG.log(Level.SEVERE, "Unable to close xml output", e); + LOG.log(Level.SEVERE, "Unable to close xml output", e); } } } diff --git a/java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/README.md b/java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/README.md index 8aaa2253..ec61c15f 100644 --- a/java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/README.md +++ b/java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/README.md @@ -193,6 +193,8 @@ The `name` attribute is the name of the test method without type information #### Suite +Skipped + #### Dynamic Skipped @@ -236,7 +238,10 @@ Neither Gradle nor Maven expose the root containers, so it's appropriate that we don't do that either. The reason that the legacy XML listener is outputting top-level test suites -with names like `JUnit Platform Suite` is that +with names like `JUnit Platform Suite` is that these are the names of the +engines being used. That is, rather than looking at the _nearest_ container, +each `testcase` is nested in the ancestor container that does not have a +parent. In no case do we want the "engine" suite appearing in our outputs.