This is an automated email from the ASF dual-hosted git repository.
ashishvijaywargiya pushed a commit to branch release24.09
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/release24.09 by this push:
new ccdee9ee3f Fix webtools Run Suite/Run Case: missing prerequisites and
shared ambient transaction (#1655) (#1656)
ccdee9ee3f is described below
commit ccdee9ee3f51793c9549d2be5a59a995fcb0d921
Author: Ashish Vijaywargiya <[email protected]>
AuthorDate: Sat Aug 15 13:59:48 2026 +0530
Fix webtools Run Suite/Run Case: missing prerequisites and shared ambient
transaction (#1655) (#1656)
Backport of #1655 (trunk commit b9dfe95aa1) to release24.09.
## Problem
Webtools' "Run Suite" / "Run Case" (`TestSuiteInfo` ->
`RunTest/{compName}/{suiteName}[/{caseName}]`) had two bugs that didn't
show up via the CLI's `ofbiz --test`:
1. Requesting a single case (`case=`) ran only that exact case, silently
skipping sibling setup cases it depends on (e.g. `party-tests` needs the
data `party-tests-data-load` creates), so a filtered run could fail for
missing data that the full suite run never exposed.
2. `entity`/`service`/`ecommerce` full-suite runs failed or misbehaved:
`RunTestEvents` -> `TestRunContainer` runs a whole testdef suite's worth
of otherwise-independent test-cases inside one
`JavaEventHandler`-invoked event call, and `JavaEventHandler`
unconditionally wraps that whole call in one ambient transaction. A
`require-new-transaction` service invoked by one test-case couldn't see
data a prior test-case's own entity-xml load had "committed" (that
commit was a no-op participant in the still-open ambient transaction),
and one test-case marking that shared transaction rollback-only poisoned
every test-case after it in the same run. Neither symptom is possible
via `ofbiz --test`, which runs `TestRunContainer` as a bare `Container`
with no event/HTTP layer and no ambient transaction at all.
## Fix
**`ModelTestSuite`**: `case=` filtering now auto-includes only genuine
data-load prerequisites (`entity-xml`/`entity-xml-assert` with
`action="load"`) declared before the requested case - not every
preceding case. An earlier, broader version of this fix (running
everything declared before the target) over-included independent sibling
test classes, real `service-test` cases, and `entity-xml
action="assert"` checks - `entitytests.xml` bundles five unrelated
`jupiter-test-suite` classes in one file, and requesting one used to
also run the others, including a deliberately slow one.
**`JavaEventHandler`**: honors the existing `global-transaction="false"`
`<event>` attribute (previously only read by `ServiceMultiEventHandler`,
unused anywhere in the codebase until this change).
**`controller.xml`**: declares `global-transaction="false"` on both
`RunTest` request-maps in webtools, opting only that one event out of
the default whole-invocation ambient transaction.
### Backport-specific notes
- Omits trunk's new `ModelTestSuiteTest.java`: this branch's `test`
Gradle task is JUnit4-only (`useJUnit()`, no `useJUnitPlatform()`), so a
pure-Jupiter unit test can't be auto-executed here the way it is on
trunk.
- Also includes a one-line fix to `RunTestEvents.java`, unrelated to and
predating #1655: an inverted null-check (`if (caseName == null)` instead
of `if (caseName != null)`) silently dropped the `case=` filter on every
webtools case-filtered run, so
`RunTest/{compName}/{suiteName}/{caseName}` always ran the entire suite
regardless of `{caseName}` - defeating half of this fix's benefit via
the actual UI. Trunk already has the correct condition; found and fixed
while verifying this backport live through webtools.
## Testing
- `./gradlew compileJava compileTestJava checkstyleMain checkstyleTest
codenarcMain codenarcTest` - all clean.
- `./gradlew cleanAll loadAll testIntegration` (full CLI run, every
testdef suite in the repo): 36 suites, 1089 tests, 0 failures, 0 errors
- including the previously-known pre-existing
`facilitytests`/`InventoryTests.testGetInventoryAvailableByFacility`
failure noted on trunk, which did not reproduce here.
- Manually exercised `party` (case-filtered), `entity` (full suite +
case-filtered `entity-crypto-tests`), `service` (full suite),
`ecommerce` (full suite) through the actual webtools UI (`RunTest/...`
URLs) - all passing, and `entity-crypto-tests` case-filtering confirmed
scoped to just its own 4 tests (0.56s, was ~32s/59 tests before the
`RunTestEvents.java` fix).
(cherry picked from commit b9dfe95aa10b2857110d6d7c0db2a689d0d1fb65)
---
build.gradle | 23 ++++-
.../org/apache/ofbiz/testtools/ModelTestSuite.java | 109 +++++++++++++++++++--
.../ofbiz/webapp/event/JavaEventHandler.java | 18 +++-
.../ofbiz/webtools/artifactinfo/RunTestEvents.java | 2 +-
.../webapp/webtools/WEB-INF/controller.xml | 18 +++-
5 files changed, 152 insertions(+), 18 deletions(-)
diff --git a/build.gradle b/build.gradle
index 9d2dd0823e..60dc5e0450 100644
--- a/build.gradle
+++ b/build.gradle
@@ -1004,11 +1004,28 @@ tasks.addRule('Pattern: ofbizBackground <Commands>:
Execute OFBiz startup comman
def createOfbizCommandTask(taskName, arguments) {
task(type: JavaExec, dependsOn: classes, taskName) {
jvmArgs(application.applicationDefaultJvmArgs)
+ // Every ofbiz-launched dev server needs test classes on its
classpath, not just one
+ // started with --test/-t: webtools' "Run Test" screen (RunTestEvents
-> TestRunContainer)
+ // loads jupiter-test-suite/junit-test-suite classes by name inside
whatever JVM is already
+ // running, regardless of how that JVM was started. OFBIZ-13402 moved
those classes out of
+ // src/main/* (and so out of sourceSets.main) to keep them out of
release artifacts; this
+ // task is a local dev-run launcher, never a release-packaging task,
so widening its
+ // classpath here doesn't reintroduce test code into anything that
gets distributed.
+ classpath = sourceSets.main.runtimeClasspath +
sourceSets.test.runtimeClasspath
if (taskName ==~ /^ofbiz.*(--test|-t).*/) {
- classpath = sourceSets.main.runtimeClasspath +
sourceSets.test.runtimeClasspath
+ // TestRunContainer.java writes one XML per suite as
"<suite.getName()>.xml" and only
+ // ever overwrites the suites a run actually re-executes - nothing
here deletes the
+ // rest first. Left alone, a narrower run (suitename=... filtering
to one component) or
+ // a renamed/removed suite leaves other suites' XML sitting in
this directory from
+ // whenever they last ran, and createTestReports below just globs
whatever *.xml
+ // files exist at generation time - so the report would silently
blend fresh results
+ // with stale ones from an earlier run into what looks like one
coherent run. Clearing
+ // the directory before the run starts guarantees every report
reflects exactly this
+ // run's suites, nothing left over from before.
+ doFirst {
+ delete fileTree('./runtime/logs/test-results') { include
'*.xml' }
+ }
finalizedBy(createTestReports)
- } else {
- classpath = sourceSets.main.runtimeClasspath
}
mainClass = application.mainClass
args arguments
diff --git
a/framework/testtools/src/main/java/org/apache/ofbiz/testtools/ModelTestSuite.java
b/framework/testtools/src/main/java/org/apache/ofbiz/testtools/ModelTestSuite.java
index 547f9cc1ae..c941f6fff1 100644
---
a/framework/testtools/src/main/java/org/apache/ofbiz/testtools/ModelTestSuite.java
+++
b/framework/testtools/src/main/java/org/apache/ofbiz/testtools/ModelTestSuite.java
@@ -65,23 +65,112 @@ public class ModelTestSuite {
this.delegator =
DelegatorFactory.getDelegator(DELEGATOR_NAME).makeTestDelegator(DELEGATOR_NAME
+ uniqueSuffix);
this.dispatcher = ServiceContainer.getLocalDispatcher(DISPATCHER_NAME
+ uniqueSuffix, delegator);
- for (Element testCaseElement : UtilXml.childElementList(mainElement,
UtilMisc.toSet("test-case", "test-group"))) {
+ List<Element> testCaseElements =
List.copyOf(UtilXml.childElementList(mainElement, UtilMisc.toSet("test-case",
"test-group")));
+ for (Element testCaseElement :
selectTestCaseElements(testCaseElements, testCase)) {
String caseName = testCaseElement.getAttribute("case-name");
String nodeName = testCaseElement.getNodeName();
- if (testCase == null || caseName.equals(testCase)) {
- if ("test-case".equals(nodeName)) {
- parseTestElement(caseName,
UtilXml.firstChildElement(testCaseElement));
- } else if ("test-group".equals(nodeName)) {
- int i = 0;
- for (Element childElement:
UtilXml.childElementList(testCaseElement)) {
- parseTestElement(caseName + '-' + i, childElement);
- i++;
- }
+ if ("test-case".equals(nodeName)) {
+ parseTestElement(caseName,
UtilXml.firstChildElement(testCaseElement));
+ } else if ("test-group".equals(nodeName)) {
+ int i = 0;
+ for (Element childElement:
UtilXml.childElementList(testCaseElement)) {
+ parseTestElement(caseName + '-' + i, childElement);
+ i++;
}
}
}
}
+ /**
+ * Selects which of a suite's ordered test-case/test-group elements should
run for a given
+ * {@code case=} filter.
+ *
+ * <p>A single case is rarely self-contained: every testdef file in this
repo follows the
+ * convention of a data-load case (an {@code entity-xml action="load"},
typically named
+ * {@code *-data-load}/{@code load*TestData}) declared immediately before
the case(s) that consume
+ * it - see e.g. applications/party/testdef/PartyTests.xml's {@code
party-tests-data-load} feeding
+ * {@code party-tests}. Filtering down to only the exact requested
case-name, the way this method
+ * used to work, silently drops that prerequisite and leaves the requested
case failing for lack of
+ * data it never asked to be responsible for loading itself.
+ *
+ * <p>Only that one shape - an {@code entity-xml}/{@code
entity-xml-assert} case whose
+ * {@code action} is {@code "load"} - is auto-included as a prerequisite;
every other preceding
+ * case is skipped unless it is itself the requested case. That is
deliberately narrower than
+ * "run everything declared before the requested case": a {@code load} is
additive and
+ * side-effect-free relative to every other case in the suite, so running
it unconditionally is
+ * safe, but nothing else in a testdef file has that property -
+ * <ul>
+ * <li>A {@code jupiter-test-suite}/{@code junit-test-suite} case, or a
{@code test-group} (a
+ * repo-wide scan found 14 of 15 {@code test-group} children are themselves
+ * {@code jupiter-test-suite}), is a whole independent test class in its
own right. Several
+ * testdef files (e.g. framework/entity/testdef/entitytests.xml) bundle
multiple such classes into
+ * one {@code <test-suite>} purely for organization, with no data
relationship between them -
+ * requesting entity-crypto-tests (declared third) used to also run the
unrelated, much larger and
+ * deliberately slow entity-tests/entity-util-tests classes ahead of it,
turning a single-case
+ * request into most of the suite's runtime.</li>
+ * <li>A {@code service-test} is always a real functional test in every
testdef file in this repo,
+ * never data loading (see framework/service/testdef/servicetests.xml's
+ * service-dead-lock-retry-test and friends).</li>
+ * <li>An {@code entity-xml} whose action is {@code "assert"} (the default
when unset) is a check,
+ * not a load - in servicetests.xml,
service-eca-global-event-exec-assert-data verifies the side
+ * effects of the service-test case declared immediately before it.
Auto-including an assert case
+ * without the real test whose effects it checks would assert against data
that was never
+ * created - trading one bug (a missing prerequisite) for another (a
spurious failure) rather than
+ * fixing it.</li>
+ * </ul>
+ *
+ * <p>Nothing declared after the requested case runs, so this is not
simply "ignore the filter":
+ * a case still can't see data set up by a case that only runs after it in
the suite's own
+ * declared order.
+ *
+ * <p>When {@code testCase} is {@code null} (no filter - run the whole
suite), every element is
+ * selected unchanged. When {@code testCase} names a case-name that isn't
present in this
+ * particular document at all, nothing is selected - unchanged from the
old exact-match
+ * behavior, and still what lets JunitSuiteWrapper's suite-name filtering
keep a testdef file
+ * that simply doesn't define the requested case from contributing
anything.
+ * @param testCaseElements the suite's test-case/test-group elements, in
declared order
+ * @param testCase the requested case-name, or {@code null} to select
every element
+ * @return the elements to run, in the same order
+ */
+ static List<Element> selectTestCaseElements(List<Element>
testCaseElements, String testCase) {
+ if (testCase == null) {
+ return testCaseElements;
+ }
+ List<Element> selected = new ArrayList<>();
+ for (Element element : testCaseElements) {
+ boolean isTarget =
testCase.equals(element.getAttribute("case-name"));
+ if (isTarget) {
+ selected.add(element);
+ return selected;
+ }
+ if (isDataLoadCase(element)) {
+ selected.add(element);
+ }
+ }
+ return List.of();
+ }
+
+ /**
+ * True for a {@code <test-case>} whose sole child is an {@code
entity-xml}/{@code entity-xml-assert}
+ * element with {@code action="load"}: the one shape safe to auto-include
as a prerequisite ahead
+ * of a requested case. See selectTestCaseElements()'s javadoc for why
every other shape - including
+ * the same elements with a different action - is excluded instead.
+ * @param testCaseElement one of a suite's test-case/test-group elements
+ * @return true if this element is a genuine data-load case
+ */
+ private static boolean isDataLoadCase(Element testCaseElement) {
+ if (!"test-case".equals(testCaseElement.getNodeName())) {
+ return false;
+ }
+ Element child = UtilXml.firstChildElement(testCaseElement);
+ if (child == null) {
+ return false;
+ }
+ String nodeName = child.getNodeName();
+ boolean isEntityXml = "entity-xml".equals(nodeName) ||
"entity-xml-assert".equals(nodeName);
+ return isEntityXml && "load".equals(child.getAttribute("action"));
+ }
+
private void parseTestElement(String caseName, Element testElement) {
String nodeName = testElement.getNodeName();
if ("junit-test-suite".equals(nodeName)) {
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
index f11f3fbb2c..03ba636131 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/JavaEventHandler.java
@@ -76,10 +76,24 @@ public class JavaEventHandler implements EventHandler {
if (Debug.verboseOn()) {
Debug.logVerbose("[Processing]: Java Event", MODULE);
}
+ // A type="java" event defaults to running inside one transaction
spanning the whole
+ // invoke() call - the same global-transaction="false" opt-out
ServiceMultiEventHandler
+ // already honors for type="service-multi" events. Without it, an
event whose own method
+ // deliberately manages several independent units of work (e.g.
RunTestEvents running a
+ // whole testdef suite's worth of otherwise-independent test-cases
through TestRunContainer)
+ // has every one of those units silently folded into this one ambient
transaction instead:
+ // an entity-xml load's own begin()/commit() inside that event becomes
a no-op participant
+ // rather than a real commit, so a require-new-transaction service
called by a later,
+ // unrelated test-case in the same suite can't see data an earlier one
just "committed",
+ // and one test-case marking the transaction rollback-only poisons
every test-case after it
+ // in the same event invocation - see the 2026-08-15
ecommerce/entity/service RunTest
+ // investigation this comment was added for.
boolean began = false;
try {
- int timeout = Integer.max(event.getTransactionTimeout(), 0);
- began = TransactionUtil.begin(timeout);
+ if (event.isGlobalTransaction()) {
+ int timeout = Integer.max(event.getTransactionTimeout(), 0);
+ began = TransactionUtil.begin(timeout);
+ }
Method m = k.getMethod(event.getInvoke(), HttpServletRequest.class,
HttpServletResponse.class);
String ret = (String) m.invoke(null, request, response);
diff --git
a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/artifactinfo/RunTestEvents.java
b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/artifactinfo/RunTestEvents.java
index 7ec49474a0..741e17dbd2 100644
---
a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/artifactinfo/RunTestEvents.java
+++
b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/artifactinfo/RunTestEvents.java
@@ -43,7 +43,7 @@ public class RunTestEvents {
List<StartupCommand> ofbizCommands = new ArrayList<>();
Map<String, String> cmdArgs = UtilMisc.toMap("component", component,
"suitename", suiteName);
- if (caseName == null) {
+ if (caseName != null) {
cmdArgs.put("case", caseName);
}
ofbizCommands.add(new
StartupCommand.Builder("test").properties(cmdArgs).build());
diff --git a/framework/webtools/webapp/webtools/WEB-INF/controller.xml
b/framework/webtools/webapp/webtools/WEB-INF/controller.xml
index 783acb468a..f60a9de123 100644
--- a/framework/webtools/webapp/webtools/WEB-INF/controller.xml
+++ b/framework/webtools/webapp/webtools/WEB-INF/controller.xml
@@ -426,13 +426,27 @@ under the License.
<request-map uri="RunTest/{compName}/{suiteName}">
<security https="true" auth="true"/>
- <event type="java"
path="org.apache.ofbiz.webtools.artifactinfo.RunTestEvents" invoke="runTest"/>
+ <!-- TestRunContainer runs a whole testdef suite's worth of
otherwise-independent
+ test-cases; each manages its own transaction the same way it does
when run via
+ the ofbiz command line's own test option, which never runs inside
an event's
+ ambient transaction to begin with. Without
global-transaction="false" here,
+ JavaEventHandler's default whole-invoke() transaction folds every
one of those
+ test-cases into a single ambient transaction instead, breaking
both data
+ visibility between test-cases and per-suite rollback-only
isolation. -->
+ <event type="java"
path="org.apache.ofbiz.webtools.artifactinfo.RunTestEvents" invoke="runTest"
global-transaction="false"/>
<response name="success" type="request" value="TestSuiteInfo"/>
<response name="error" type="request" value="TestSuiteInfo"/>
</request-map>
<request-map uri="RunTest/{compName}/{suiteName}/{caseName}">
<security https="true" auth="true"/>
- <event type="java"
path="org.apache.ofbiz.webtools.artifactinfo.RunTestEvents" invoke="runTest"/>
+ <!-- TestRunContainer runs a whole testdef suite's worth of
otherwise-independent
+ test-cases; each manages its own transaction the same way it does
when run via
+ the ofbiz command line's own test option, which never runs inside
an event's
+ ambient transaction to begin with. Without
global-transaction="false" here,
+ JavaEventHandler's default whole-invoke() transaction folds every
one of those
+ test-cases into a single ambient transaction instead, breaking
both data
+ visibility between test-cases and per-suite rollback-only
isolation. -->
+ <event type="java"
path="org.apache.ofbiz.webtools.artifactinfo.RunTestEvents" invoke="runTest"
global-transaction="false"/>
<response name="success" type="request" value="TestSuiteInfo"/>
<response name="error" type="request" value="TestSuiteInfo"/>
</request-map>
