REST API Support for Running and Monitoring OFBiz Test Cases - #1690
Merged
ashishvijaywargiya merged 29 commits intoAug 20, 2026
Conversation
Add a new 5-arg constructor to JupiterClassRunner that accepts a testParams parameter map, with the 4-arg constructor delegating to it with Map.of(). Arm and clear CURRENT_TEST_PARAMS in the run() method's try/finally block. Add a corresponding 5-arg overload to TestRunContainer.runSuiteEntries() that accepts testParams, with the 4-arg version delegating to it. Include tests verifying that CURRENT_TEST_PARAMS is armed during execution and cleared after, and that it defaults to an empty Map when omitted.
…suite resolution - executeRun: wrap SuiteXmlReportWriter usage per suite so endSuite() (the only place that flushes/writes/closes the underlying FileOutputStream) always runs via finally, even if runSuiteEntries()/rollback() throws. Prevents a file-descriptor leak and a 0-byte XML being left on disk. - executeRun: widen the outer catch from Exception to Throwable, matching TestRunContainer.start()'s established precedent, so an Error escaping a test class (NoClassDefFoundError, StackOverflowError, OOM) still reaches TRACKER.markError(...) instead of leaving the run stuck at RUNNING forever. - runTestSuite: wrap JunitSuiteWrapper construction and the empty-list check in a try/catch returning ServiceUtil.returnError(...), so malformed testdef XML during suite resolution no longer propagates as a raw 500 to the REST caller.
… limitations runTestSuite reuses ModelTestSuite's constructor (the same construction the ofbiz --test CLI already uses once per process), which permanently registers a new test Delegator/LocalDispatcher in ServiceContainer's static dispatcher cache and re-runs startup services. Harmless for the one-shot CLI, but a real resource leak for this feature's long-lived server process handling many API-triggered runs with no cleanup. This is an accepted POC limitation, not an oversight - a proper fix would touch shared static server-engine internals (ServiceContainer deregistration) that can't be safely validated without a live running instance. Also documents that API-triggered runs execute against the live server's database with only best-effort rollback (modelSuite.getDelegator().rollback()), not a true isolated sandbox - previously undocumented anywhere in the shipped code and a material fact for anyone deciding whether to enable test.api.enabled. Adds a class-level javadoc paragraph on TestRunServices and a matching note in testtools.properties' test.api.enabled comment block.
register() stored the caller's paramsUsed map by reference; that same map instance later gets armed into JupiterTestExtension.CURRENT_TEST_PARAMS and is directly readable/mutable inside the running Jupiter test class (as testParameters). A test mutating testParameters could corrupt both the tracker's stored record and, eventually, the archived manifest.json's paramsUsed. Wraps the incoming map with Map.copyOf(...) before storing it - the existing call site (TestRunServices.runTestSuite) always passes a non-null map, so no null handling is needed. Adds a test proving that mutating the original map after calling register() does not affect what tracker.get(runId).paramsUsed() returns.
- Remove the dead 'Delegator delegator = null;' local that existed only to be passed as null; pass null directly to readStringProperty(...) instead. - Replace the inline fully-qualified org.apache.ofbiz.base.util.UtilProperties call with a proper import and unqualified call, matching the rest of the file's style. - Document, at the point a reader would notice it, that test.history is read file-only here (delegator is null on this background thread) unlike test.api.enabled, which is delegator-aware - so a SystemProperty override of test.history will NOT apply to API-triggered runs. Intentional (see the existing 'Note for the implementer' comment) but previously undocumented at this specific point.
M1: Move defensive parameterization to runTestSuite before passing to
executor/test/archiver, ensuring all downstream consumers receive an
immutable, null-safe copy from that point on.
M2: Replace Map.copyOf with LinkedHashMap+unmodifiableMap pattern in both
runTestSuite and TestRunTracker.register to tolerate null values,
fixing the NullPointerException when testParams contained null values.
Add test verifying null values are handled without NPE.
Update comments to reflect the layered defense-in-depth approach.
Deregister each ModelTestSuite's test dispatcher after its suite finishes, via ServiceContainer.deregister(name). This removes the per-run entry from ServiceContainer's static dispatcher cache and closes its JMS listeners, so those no longer accumulate forever on a long-lived server. Best-effort: failures are logged, not thrown, so cleanup can never turn a passing run into a reported failure. Still open, and documented as such in the class javadoc and in testtools.properties: the underlying ServiceDispatcher instance itself stays in ServiceDispatcher's own separate static cache indefinitely, since no public API removes an entry from that map (would require changes to ServiceDispatcher.java, out of scope here). Startup services also still re-run on every call, since each run's delegator gets a unique name and so never hits the ServiceDispatcher-level cache regardless of deregistration.
…eanup The prior leak-mitigation commit called ServiceContainer.deregister(name) after each API-triggered test suite. That call shuts down the backing ServiceDispatcher once its localContext empties out - always true immediately for a dispatcher used by exactly one test run - and shutdown closes JMS listeners through JmsListenerFactory, a process-wide static singleton shared by every dispatcher in the JVM, including the live server's real one. Every API-triggered test run would have permanently killed the server's actual JMS listeners in any deployment with JMS listening enabled. Switched to ServiceContainer.removeFromCache(name), which only removes the ServiceContainer dispatcher-cache entry and touches nothing else. Corrected the javadoc/properties-comment claims that described the removed call as closing JMS listeners. Also, related to the same cleanup path: - JunitSuiteWrapper now tracks ModelTestSuite instances it discards for having no matching tests (previously silently dropped, each still holding a live dispatcher/delegator pair from its constructor). TestRunServices deregisters these before returning its "no tests found" error, closing a leak that previously had no mitigation at all. - Moved the dispatcher-name lookup inside the try block in deregisterTestDispatcher and widened its catch from Exception to Throwable, so a cleanup-path Error can no longer escape into the outer handler and turn a passing run into a reported error. - Reworded the class javadoc so it no longer implies the test Delegator is mitigated by this fix - it stays pinned by the ServiceDispatcher instance for as long as that instance remains cached, same as before. - Widened the per-suite try in executeRun to cover report-stream creation, and added a fallback in the outer finally to deregister any suites the loop didn't reach, so one suite's exception can no longer abort the loop and leave every later suite's dispatcher registered. Verified via ./gradlew test --tests "org.apache.ofbiz.testtools.*" (all passing). ServiceDispatcher.java and ModelTestSuite.java are untouched.
…ponent-scoped wrappers
…adopters away from the raw services - runScopedTestSuite/getScopedTestRunStatus now reject an empty/null fixed/expected componentName up front with a clean ServiceUtil error, instead of one failing open (ComponentConfig treats a null cname as match-all) and the other throwing a raw NPE. - Added services.xml description text and TestRunServices class javadoc pointing a reader from the raw runTestSuite/getTestRunStatus services to the scoped-wrapper pattern, so a future component doesn't reproduce the cross-component-reach bug by exposing them directly in its own *.rest.xml. - Added tests covering both new guards and the null-componentName masking branch.
Matches the wire-level runTestSuite service attribute name (testParams) instead of a second name for the same concept. Groovy test classes now read the bare testParams property instead of testParameters - see the companion commit in plugins/ (ExampleJupiterTests.groovy).
ashishvijaywargiya
added a commit
to apache/ofbiz-plugins
that referenced
this pull request
Aug 20, 2026
…component sample code) (#370) Pushing the sample code from plugins->example component, this will help others to see the patterns like how we can enable rest support for other component's test cases. ofbiz-framework PR: apache/ofbiz-framework#1690
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Today, running OFBiz's tests means logging into the server and typing a command by hand. This feature lets tests be started and checked over a simple web request instead, so tools like CI pipelines can run them automatically, and no one needs direct server access just to run a test.
I took care of the following work items in this pass:
Adds TestRunServices.runTestSuite/getTestRunStatus, a REST/service-triggered way to kick off an existing testdef test-suite (optionally one case-name within it) asynchronously and poll it via a runId, reusing the exact same JunitSuiteWrapper/TestRunContainer engine that ofbiz --test and gradlew testIntegration already use.
Gates that API behind two independent fail-closed checks enforced inside the service itself, not just at the REST layer: the test.api.enabled config flag (off by default) and a new TESTEXEC_ADMIN permission.
Tracks each run's lifecycle (QUEUED -> RUNNING -> PASSED/FAILED/ERROR) in an in-memory TestRunTracker so getTestRunStatus can be polled while the run executes on a dedicated single-threaded executor, and archives finished API-triggered runs into the same manifest.json test-history store gradlew test/testIntegration already write to, tagged trigger="api".
Adds runScopedTestSuite/getScopedTestRunStatus, which force-lock componentName server-side so a component-branded REST endpoint can never trigger or poll another component's tests regardless of what a caller supplies; plugins/example's ExampleTestRunServices (a Groovy service script) is the reference implementation any other component can copy.
Adds a testParams map that lets a caller override field values inside the Jupiter test methods for that one run, delivered through a ThreadLocal bridge and JupiterTestHelper.getTestParams(), later extended to be namespaced per test method so two different methods in the same run can each receive their own value for the same field name.
Adds ofbiz --test method= on the CLI and the equivalent testMethodName on runTestSuite/runExampleTestSuite, both scoping a resolved case's class down to one @test or @ParameterizedTest method instead of running the whole class, sharing the identical fail-closed validators and reflection-based method discovery (needed because JUnit Platform's plain selectMethod(Class,String) cannot match a method that takes parameters).
Documents and partially mitigates a POC-scope dispatcher/delegator resource leak from running tests inside a long-lived server process, deliberately using ServiceContainer.removeFromCache rather than deregister after a fix for a critical regression where deregister was found to shut down the live server's real JMS listeners.
Adds substantial new unit-test coverage across both repos (TestRunServicesTest, TestRunContainerTest, JupiterClassRunnerTest, TestRunTrackerTest, and more) and grows plugins/example's ExampleJupiterTests with testParams-driven and parameterized test methods used to exercise every feature above end-to-end.
A final round of live testing against a running dev server -- unit tests, checkstyle, and real REST/CLI calls covering valid runs, parameterized methods, every validation-failure path, permission/auth checks, and componentName-tampering attempts -- found zero regressions across both the test-run-triggering feature and the method=/testMethodName feature.