This is an automated email from the ASF dual-hosted git repository.
ashishvijaywargiya pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new 0f6b6b2b46 Harden the Jupiter test bridge and catch two classes of
silent test-infra failures (#1571)
0f6b6b2b46 is described below
commit 0f6b6b2b46c6ba05196693c032fac210a209e631
Author: Ashish Vijaywargiya <[email protected]>
AuthorDate: Thu Aug 6 11:01:14 2026 +0530
Harden the Jupiter test bridge and catch two classes of silent test-infra
failures (#1571)
Harden the Jupiter test bridge and catch two classes of silent
test-infra failures
Fixes a bug where a throwing @BeforeAll or an exception escaping
Jupiter's launcher could silently drop or abort test suites in
testIntegration
with no build failure. Also adds two build-time checks (testdef class-name
typos
and bare @ExtendWith(JupiterTestExtension.class) usage), fixes a stale
JUnit-3 doc example, and adds checkstyleTest to the pre-push hook.
Verified: full unit suite (580 tests), checkstyle/codenarc, the two new
verification tasks, and the full integration suite all pass.
---
build.gradle | 106 ++++++++++++++++++++-
.../testtools/src/docs/asciidoc/unit-tests.adoc | 43 ++++++++-
.../ofbiz/testtools/JupiterTestExtension.java | 26 +++++
.../apache/ofbiz/testtools/TestRunContainer.java | 38 +++++++-
.../testtools/JupiterInjectionGuardsTest.java | 41 ++++++++
.../ofbiz/testtools/TestRunContainerTest.java | 67 +++++++++++++
6 files changed, 317 insertions(+), 4 deletions(-)
diff --git a/build.gradle b/build.gradle
index 07b8f1261c..9ac503e168 100644
--- a/build.gradle
+++ b/build.gradle
@@ -341,7 +341,7 @@ gitHooks {
// where "$rootDir/.git" (the plugin's default) is a gitlink file, not a
directory.
def gitCommonDir = "git -C ${rootDir} rev-parse
--git-common-dir".execute().text.trim()
hooksDirectory.set(new File(rootDir,
gitCommonDir).toPath().resolve('hooks').toFile())
- hooks = ['pre-push': 'checkstyleMain codenarcMain codenarcTest']
+ hooks = ['pre-push': 'checkstyleMain checkstyleTest codenarcMain
codenarcTest']
}
// Checks OFBiz Groovy coding conventions.
@@ -396,6 +396,110 @@ test {
}
}
+// 'gradlew test --tests <ClassName>' against a @JunitJupiterTest class fails
with Gradle's generic
+// "No tests found for given includes" rather than pointing at
testIntegration: the excludeTags
+// filter above excludes the class from discovery entirely before --tests
filtering ever runs, so
+// from Gradle's perspective the class simply doesn't exist. This doesn't
change that failure - it's
+// otherwise a genuinely useful safety net for a real --tests typo - it just
appends a pointer to the
+// right command for the specific case where the cause is a Jupiter
integration-test class.
+gradle.taskGraph.afterTask { Task task, TaskState state ->
+ if (task.path == ':test' && state.failure) {
+ Throwable rootCause = state.failure
+ while (rootCause.cause != null) {
+ rootCause = rootCause.cause
+ }
+ if (rootCause.message?.contains('No tests found for given includes')) {
+ logger.error('')
+ logger.error('[hint] If the class above is annotated
@JunitJupiterTest, this failure is expected: '
+ + 'that tag excludes it from \'gradlew test\' discovery
entirely (see JunitJupiterTest\'s '
+ + 'javadoc), so --tests never gets a chance to filter it
in. Run it via '
+ + '\'gradlew testIntegration\' or \'ofbiz --test\'
instead.')
+ logger.error('')
+ }
+ }
+}
+
+// ModelTestSuite.parseTestElement() resolves every
<junit-test-suite>/<jupiter-test-suite>
+// class-name attribute at runtime via ObjectType.loadClass(), but a lookup
failure there is
+// only Debug.logError()'d - the whole suite is silently dropped from
testIntegration with no
+// build failure and no signal short of reading the log output. This task
repeats that same
+// lookup at build time, against the compiled test classpath, so a typo'd or
stale (post-rename)
+// class-name fails the build instead of quietly vanishing from the test run.
+task verifyTestdefClassNames(group: 'Verification', dependsOn: testClasses) {
+ description = 'Fails the build if any testdef XML class-name attribute
cannot be resolved on the classpath'
+ def testdefDirs = getDirectoryInActiveComponentsIfExists('testdef')
+ def testdefXmlFiles = files(testdefDirs.collect { fileTree(it) { include
'**/*.xml' } })
+ def testRuntimeClasspath = sourceSets.test.runtimeClasspath
+ inputs.files(testdefXmlFiles)
+ inputs.files(testRuntimeClasspath)
+ doLast {
+ URL[] classpathUrls = testRuntimeClasspath.files.collect {
it.toURI().toURL() }
+ URLClassLoader classpathLoader = new URLClassLoader(classpathUrls,
getClass().classLoader)
+ List<String> unresolved = []
+ testdefXmlFiles.each { xmlFile ->
+ def root = new XmlParser(false, false).parse(xmlFile)
+ root.depthFirst()
+ .findAll { it.name() == 'junit-test-suite' || it.name() ==
'jupiter-test-suite' }
+ .each { node ->
+ String className = node.'@class-name'
+ if (!className) {
+ return
+ }
+ try {
+ Class.forName(className, false, classpathLoader)
+ } catch (Throwable t) {
+ unresolved << "${xmlFile}: <${node.name()}
class-name=\"${className}\"/> - ${t}"
+ }
+ }
+ }
+ if (!unresolved.isEmpty()) {
+ throw new GradleException('The following testdef class-name
references cannot be resolved on the '
+ + 'classpath (typo, or the class was renamed/moved/deleted
without updating the testdef XML):\n'
+ + unresolved.collect { " - ${it}" }.join('\n'))
+ }
+ }
+}
+check.dependsOn verifyTestdefClassNames
+
+// JunitJupiterTest's own javadoc documents that bare
@ExtendWith(JupiterTestExtension.class)
+// skips gradlew test's excludeTags-based exclusion - it's only caught at
runtime by
+// evaluateExecutionCondition(), reporting a skip instead of being excluded
from discovery
+// outright. That's an intentional, documented escape hatch for
JupiterTestExtension's own home
+// (JunitJupiterTest's definition and JupiterInjectionGuardsTest's fixtures,
both under
+// framework/testtools, use it deliberately to define/exercise the escape
hatch itself), but
+// nothing stops a future contributor from using the bare form on a real test
class by habit or
+// copy-paste. This cheap grep-based check catches that everywhere else.
+task verifyNoBareJupiterExtendWith(group: 'Verification') {
+ description = 'Fails the build if a bare
@ExtendWith(JupiterTestExtension.class) is used ' +
+ 'outside framework/testtools instead of @JunitJupiterTest'
+ def testInfraDir = file('framework/testtools')
+ def sourceFiles = files(
+ (getDirectoryInActiveComponentsIfExists('src/main/java')
+ + getDirectoryInActiveComponentsIfExists('src/main/groovy')
+ + getDirectoryInActiveComponentsIfExists('src/test/java')
+ + getDirectoryInActiveComponentsIfExists('src/test/groovy'))
+ .collect { dir -> fileTree(dir) { include '**/*.java',
'**/*.groovy' } }
+ ).filter { file -> !file.toPath().startsWith(testInfraDir.toPath()) }
+ inputs.files(sourceFiles)
+ doLast {
+ def bareExtendWith =
~/@ExtendWith\(\s*(org\.apache\.ofbiz\.testtools\.)?JupiterTestExtension(\.class)?\s*\)/
+ List<String> offenders = []
+ sourceFiles.each { file ->
+ file.readLines().eachWithIndex { line, idx ->
+ if (bareExtendWith.matcher(line).find()) {
+ offenders << "${file}:${idx + 1}: ${line.trim()}"
+ }
+ }
+ }
+ if (!offenders.isEmpty()) {
+ throw new GradleException('Bare
@ExtendWith(JupiterTestExtension.class) found outside '
+ + 'framework/testtools - use @JunitJupiterTest instead, so
gradlew test\'s '
+ + 'excludeTags-based exclusion actually applies (see
JunitJupiterTest\'s javadoc):\n'
+ + offenders.collect { " - ${it}" }.join('\n'))
+ }
+ }
+}
+check.dependsOn verifyNoBareJupiterExtendWith
/* ========================================================
* Tasks
diff --git a/framework/testtools/src/docs/asciidoc/unit-tests.adoc
b/framework/testtools/src/docs/asciidoc/unit-tests.adoc
index 6cc2b5653d..4838bc80e7 100644
--- a/framework/testtools/src/docs/asciidoc/unit-tests.adoc
+++ b/framework/testtools/src/docs/asciidoc/unit-tests.adoc
@@ -63,14 +63,53 @@ respectively like this:
</test-case>
----
-=== JUnit
+=== JUnit 3 (legacy)
Specific class's name which will be tested, in a class-name attribute like
this:
[source, xml]
+----
+ <test-case case-name="example-tests">
+ <junit-test-suite
class-name="org.apache.ofbiz.example.test.ExampleTests"/>
+ </test-case>
+----
+This style predates the JUnit 5 (Jupiter) migration and is being phased out -
new test classes
+should use `<jupiter-test-suite>` instead, described next.
+
+=== Jupiter
+JUnit 5 (Jupiter) test classes are wired the same way, through a
`<jupiter-test-suite>` element
+instead of `<junit-test-suite>`:
+[source, xml]
----
<test-case case-name="service-tests">
- <junit-test-suite
class-name="org.apache.ofbiz.service.test.ServiceEngineTests"/>
+ <jupiter-test-suite
class-name="org.apache.ofbiz.service.test.ServiceEngineTests"/>
</test-case>
----
+The Java/Groovy class itself needs matching Jupiter annotations -
`@JunitJupiterTest` on the
+class, `@Test` on each test method:
+[source, java]
+----
+ @JunitJupiterTest
+ public class ServiceEngineTests implements JupiterTestHelper {
+ @Test
+ @Order(1)
+ void testXxx() {
+ ...
+ }
+ }
+----
+Two things worth knowing before adding a new Jupiter test class:
+
+* *Both halves are required, and a mismatch fails silently.* The `class-name`
attribute above
+ and the class's own annotations do two separate jobs - the testdef XML
registers the class
+ with `ofbiz --test`/`testIntegration`, the annotations make it a valid
Jupiter test. A typo'd
+ or stale `class-name` (e.g. after renaming the class) is only logged as an
error, not a build
+ failure - the suite is quietly dropped from the run with no other signal.
Double-check the
+ class name after any rename.
+* *Method execution order defaults to source declaration order, not enforced
unless you say so.*
+ Add an explicit `@Order(n)` to every `@Test` method if any of them depend on
state a previous
+ method left behind - a common pattern in integration tests that share one
suite-level
+ Delegator/LocalDispatcher. Without `@Order`, a class is relying on
declaration order only
+ implicitly - reordering methods, adding one in the middle, or splitting the
file can silently
+ break it later.
=== Service
Specific service's name which will be tested in a service-name attribute like
this:
diff --git
a/framework/testtools/src/main/java/org/apache/ofbiz/testtools/JupiterTestExtension.java
b/framework/testtools/src/main/java/org/apache/ofbiz/testtools/JupiterTestExtension.java
index b881cc4987..0cce71dc60 100644
---
a/framework/testtools/src/main/java/org/apache/ofbiz/testtools/JupiterTestExtension.java
+++
b/framework/testtools/src/main/java/org/apache/ofbiz/testtools/JupiterTestExtension.java
@@ -365,6 +365,7 @@ public class JupiterTestExtension implements
ParameterResolver, TestInstancePost
@Override
public void executionFinished(TestIdentifier
testIdentifier, TestExecutionResult testExecutionResult) {
if (!testIdentifier.isTest()) {
+ reportContainerFailure(testIdentifier,
testExecutionResult, result);
return;
}
Test leaf =
leafTests.get(testIdentifier.getUniqueId());
@@ -395,6 +396,31 @@ public class JupiterTestExtension implements
ParameterResolver, TestInstancePost
}
}
+ /**
+ * Without this, a container-level failure - a static {@literal
@}BeforeAll (or any other
+ * class-level setup JUnit 5 runs before its children) throwing - is
silently discarded:
+ * executionFinished() above returns before doing anything for a
non-test identifier, so the
+ * FAILED/ABORTED result JUnit 5 reports once, on the container, never
reaches
+ * result.addError()/addFailure(). That would leave every {@literal
@}Test method in the class
+ * never individually started, results.wasSuccessful() still true, and
testIntegration reporting
+ * full success for a class whose tests never actually ran. Reported
as a synthetic leaf (mirroring
+ * JupiterLeafTest's own "reporting handle only" pattern below) since
TestResult has no native
+ * concept of a class-level failure with no associated test case.
+ */
+ private void reportContainerFailure(TestIdentifier testIdentifier,
TestExecutionResult testExecutionResult, TestResult result) {
+ TestExecutionResult.Status status =
testExecutionResult.getStatus();
+ if (status != TestExecutionResult.Status.FAILED && status !=
TestExecutionResult.Status.ABORTED) {
+ return;
+ }
+ Test leaf = new JupiterLeafTest(testClass.getSimpleName() +
".initializationError", testClass.getName());
+ Throwable throwable = testExecutionResult.getThrowable()
+ .orElseGet(() -> new AssertionError("Container '" +
testIdentifier.getDisplayName()
+ + "' reported " + status + " with no throwable"));
+ result.startTest(leaf);
+ result.addError(leaf, throwable);
+ result.endTest(leaf);
+ }
+
/**
* getLegacyReportingName() reports plain @Test methods as
"methodName(ParamType1, ParamType2)"
* and @ParameterizedTest invocations as "methodName(ParamType1,
ParamType2)[index]" - the
diff --git
a/framework/testtools/src/main/java/org/apache/ofbiz/testtools/TestRunContainer.java
b/framework/testtools/src/main/java/org/apache/ofbiz/testtools/TestRunContainer.java
index 6f097d1857..6891fff421 100644
---
a/framework/testtools/src/main/java/org/apache/ofbiz/testtools/TestRunContainer.java
+++
b/framework/testtools/src/main/java/org/apache/ofbiz/testtools/TestRunContainer.java
@@ -87,7 +87,19 @@ public class TestRunContainer implements Container {
// test
xml.startTestSuite(test);
- suite.run(results);
+ try {
+ suite.run(results);
+ } catch (Throwable t) {
+ // An individual test's exception is always caught by
TestCase.runBare()/TestResult's
+ // own protected-invocation machinery and reported as that one
test's error - it can
+ // never take down sibling suites. JupiterTestSuite.run() (the
JUnit 3 bridge for
+ // Jupiter classes) doesn't have that same guarantee: anything
escaping its
+ // launcher.execute() call - a JUnitException from a
discovery/engine-registration problem,
+ // a PreconditionViolationException, or a bug in its own
TestExecutionListener callback code
+ // - propagates straight out of suite.run() here. Without this
catch, that would abort every
+ // remaining testdef suite in this loop, not just the one that
hit the problem.
+ reportSuiteExecutionFailure(suite, results, t);
+ }
test.setCounts(results.runCount(), results.failureCount(),
results.errorCount());
modelSuite.getDelegator().rollback(); // rollback all entity
operations
xml.endTestSuite(test);
@@ -123,6 +135,30 @@ public class TestRunContainer implements Container {
}
}
+ /**
+ * Reports an exception that escaped suite.run() itself as a synthetic
suite-level error instead of
+ * letting it propagate out of start()'s for loop - see the try/catch
around suite.run() above for
+ * why that would otherwise abort every remaining testdef suite in the
run, not just this one.
+ * Reported through the same TestResult/listener pipeline (JunitListener,
the XML formatter) a normal
+ * addError() would use, so it shows up in the suite's report and
results.wasSuccessful() correctly
+ * flips to false, rather than this suite silently contributing zero tests
to the run.
+ *
+ * <p>Package-private rather than private so TestRunContainerTest can
exercise it directly without
+ * needing a full ofbiz --test container bootstrap.
+ */
+ static void reportSuiteExecutionFailure(TestSuite suite, TestResult
results, Throwable throwable) {
+ Debug.logError(throwable, "[JUNIT] Suite '" + suite.getName() + "'
failed to execute: " + throwable, MODULE);
+ Test failureMarker = new TestCase(suite.getName() +
".suiteExecutionError") {
+ @Override
+ public void run(TestResult result) {
+ throw new UnsupportedOperationException("reporting handle
only, cannot be run directly");
+ }
+ };
+ results.startTest(failureMarker);
+ results.addError(failureMarker, throwable);
+ results.endTest(failureMarker);
+ }
+
private static JunitSuiteWrapper prepareJunitSuiteWrapper(Map<String,
String> testProps) throws ContainerException {
String component = testProps.get("component");
String suiteName = testProps.get("suitename");
diff --git
a/framework/testtools/src/test/java/org/apache/ofbiz/testtools/JupiterInjectionGuardsTest.java
b/framework/testtools/src/test/java/org/apache/ofbiz/testtools/JupiterInjectionGuardsTest.java
index 4d7b013f1c..ef03033099 100644
---
a/framework/testtools/src/test/java/org/apache/ofbiz/testtools/JupiterInjectionGuardsTest.java
+++
b/framework/testtools/src/test/java/org/apache/ofbiz/testtools/JupiterInjectionGuardsTest.java
@@ -188,6 +188,31 @@ class JupiterInjectionGuardsTest {
assertThat(suite.countTestCases(), is(1));
}
+ @Test
+ void throwingBeforeAllIsReportedAsAnErrorInsteadOfSilentlyDiscarded() {
+ Delegator delegator = mock(Delegator.class);
+ LocalDispatcher dispatcher = mock(LocalDispatcher.class);
+
+ JupiterTestExtension.JupiterTestSuite suite =
+ new
JupiterTestExtension.JupiterTestSuite(ThrowingBeforeAllFixture.class);
+ suite.setDelegator(delegator);
+ suite.setDispatcher(dispatcher);
+ TestResult result = new TestResult();
+ suite.run(result);
+
+ // The @Test method itself never starts - JUnit 5 reports the failure
once, on the class
+ // container, with no [test-method:...] identifier at all (confirmed
by instrumenting the
+ // listener directly) - so this runCount()/errorCount() pair comes
entirely from the
+ // synthetic leaf reportContainerFailure() reports, not from
triggersBeforeAll() itself.
+ // Without that reporting, this would be results.wasSuccessful() ==
true for a class whose
+ // tests never actually ran - the exact false-positive "all green"
this fix prevents.
+ assertThat(result.runCount(), is(1));
+ assertThat(result.errorCount(), is(1));
+ assertThat(result.wasSuccessful(), is(false));
+ Throwable reported = result.errors().nextElement().thrownException();
+ assertThat(reported.getMessage(), containsString("boom"));
+ }
+
@Test
void beforeAllStaticMethodReceivesDelegatorViaParameterResolution() {
Delegator delegator = mock(Delegator.class);
@@ -303,4 +328,20 @@ class JupiterInjectionGuardsTest {
// No-op: exists only so the class has a @Test method for
@BeforeAll to run ahead of.
}
}
+
+ // Exercises reportContainerFailure(): a throwing @BeforeAll means JUnit 5
reports FAILED once, on
+ // the class container, and never starts triggersBeforeAll() below at all.
+ @Tag(JupiterTestExtension.INTEGRATION_TAG)
+ @ExtendWith(JupiterTestExtension.class)
+ static class ThrowingBeforeAllFixture {
+ @BeforeAll
+ static void explode() {
+ throw new IllegalStateException("boom");
+ }
+
+ @Test
+ void triggersBeforeAll() {
+ // No-op: never actually reached - exists only so the class has a
@Test method.
+ }
+ }
}
diff --git
a/framework/testtools/src/test/java/org/apache/ofbiz/testtools/TestRunContainerTest.java
b/framework/testtools/src/test/java/org/apache/ofbiz/testtools/TestRunContainerTest.java
new file mode 100644
index 0000000000..78c7e2f204
--- /dev/null
+++
b/framework/testtools/src/test/java/org/apache/ofbiz/testtools/TestRunContainerTest.java
@@ -0,0 +1,67 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+
*******************************************************************************/
+package org.apache.ofbiz.testtools;
+
+import org.junit.jupiter.api.Test;
+
+import junit.framework.TestResult;
+import junit.framework.TestSuite;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Exercises reportSuiteExecutionFailure() directly rather than through
start()'s for loop, which
+ * needs a full ofbiz --test container bootstrap (StartupCommand, a real
ModelTestSuite/Delegator,
+ * ...) to construct. The loop's own try/catch wiring around
suite.run(results) is a single,
+ * low-risk control-flow change; this test covers the part that actually has
behavior worth
+ * verifying - that an escaped exception is turned into a proper suite-level
error report instead
+ * of a silently-empty, still-"successful" TestResult.
+ */
+class TestRunContainerTest {
+
+ @Test
+ void escapedExceptionIsReportedAsASuiteLevelError() {
+ TestSuite suite = new TestSuite("myFakeSuite");
+ TestResult results = new TestResult();
+ RuntimeException thrown = new RuntimeException("boom");
+
+ TestRunContainer.reportSuiteExecutionFailure(suite, results, thrown);
+
+ // Without this reporting, an exception escaping suite.run() itself (a
JUnitException from
+ // Jupiter's launcher.execute(), for example) would leave `results`
with zero tests recorded
+ // and wasSuccessful() still true - the same false-positive "all
green" class of bug as a
+ // silently-discarded container failure.
+ assertThat(results.runCount(), is(1));
+ assertThat(results.errorCount(), is(1));
+ assertThat(results.wasSuccessful(), is(false));
+ assertThat(results.errors().nextElement().thrownException(),
is(thrown));
+ }
+
+ @Test
+ void syntheticFailureMarkerIsNamedAfterTheSuite() {
+ TestSuite suite = new TestSuite("myFakeSuite");
+ TestResult results = new TestResult();
+
+ TestRunContainer.reportSuiteExecutionFailure(suite, results, new
RuntimeException("boom"));
+
+ assertThat(results.errors().nextElement().failedTest().toString(),
containsString("myFakeSuite.suiteExecutionError"));
+ }
+}
