[maximthomas]

InjectorHolder builds the first injector of the JVM from the module loader InjectorConfiguration holds at that moment, and never reads the loader again. The Guice test classes in openam-scripting and openam-audit-context installed their loader from @BeforeMethod or @BeforeClass, so a class that reaches the holder before them decides the injector for the whole fork. As #1187 describes, the hazard is latent: no order of today's test classes fails, but a new unhooked class that runs first breaks both modules. Same cause as #1186 for openam-core-rest.

Changes (test code only):

• openam-scripting: a TestNG execution listener, EmptyGuiceModuleLoaderListener, installs the empty loader. It is registered in src/test/resources/META-INF/services/org.testng.ITestNGListener, so TestNG runs it before it creates any test instance, in every TestNG run of the module, whichever classes the run includes and however surefire forks it.
  • It replaces the setupGuiceModules() overrides in GroovyValidatorTest, JavaScriptValidatorTest and GroovyAstTransformEscapeTest, which installed the loader before each test. GuiceTestCase.setupGuiceModules() carries its own @BeforeMethod and still registers the per-test injector from @GuiceModules.
  • GroovyAstTransformEscapeTest no longer overrides setupGuiceModules() at all: clearCanary() now runs before and after each test.
• openam-audit-context: AuditContextFilterTest installs its loader from @BeforeSuite(alwaysRun = true) instead of @BeforeClass, and resets TransactionIdConfigurationImpl.trustHeader to its default after each test.
  • Unlike the issue title says, this loader is not empty. AuditContextFilterTest is not a GuiceTestCase, so no per-test injector is registered, and AuditContextFilter reads TransactionIdConfiguration from the holder's own injector. The loader keeps returning TestGuiceModule, which binds it. With an empty loader both tests fail with No implementation for TransactionIdConfiguration was bound.
  • That binding now serves every class in the JVM, hence the reset.

Limits of the openam-audit-context hook, stated in its javadoc:

• TestNG creates an instance of every test class before it runs any @BeforeSuite method, so a constructor or field initializer that reaches InjectorHolder still comes first. No class in the module does that today.
• The loader applies to the whole JVM. A second class there that installs a different loader from its own @BeforeSuite would make the result depend on class order again.

Out of scope: RestRouterIT in openam-rest still installs an empty loader from a @BeforeMethod, but needs none, because the default loader builds the root injector on that module's test classpath (details in #1187).

Tests, offline (scratch classes and TestNG suite XMLs not committed):

• Before the change, a scratch class that touches InjectorHolder without a hook, run first: openam-scripting had 3 failures and 63 skipped (NoClassDefFoundError: Could not initialize class org.forgerock.guice.core.InjectorHolder in setupGuiceModules of all three classes), and openam-audit-context had 2 failures.
• openam-scripting at 72fdecf:
  • a scratch class whose field initializer calls InjectorHolder, run first ahead of the three Guice classes: 20 of 20 pass. Without the service file the same run cannot start (Cannot instantiate class ...AaaHolderCtorProbeTest);
  • that scratch class alone with -Dtest: 1 of 1;
  • each Guice class alone with -Dtest=<Class>: 5, 5 and 9 pass;
  • mvn -o -pl openam-scripting test in the default order, with -Dsurefire.runOrder=reversealphabetical and with -DreuseForks=false: 196 tests, 0 failures each.
• openam-audit-context at 72fdecf:
  • an unhooked scratch class that touches the holder first, then AuditContextFilterTest with shouldIgnoreTransactionIdHttpHeaderIfConfiguredToDoSo last, then a scratch class that asserts the holder's TransactionIdConfiguration trusts the header: 4 of 4. Without the reset the last one fails with Expecting value to be true but was false;
  • mvn -o -pl openam-audit/openam-audit-context test, default and with -DreuseForks=false: 2 tests, 0 failures.

Fixes #1187

[vharseko]

The hooks do fix the class-order failure in the two modules they touch. openam-scripting and openam-audit-context inherit forkCount=1 and reuseForks=true, so each module runs as one TestNG suite, and the @BeforeSuite hooks install the loader before the first test whatever the class order. Both modules pass on every CI leg (see CI below).

I'm requesting changes for points 1–3. As it stands, the PR leaves the hazard #1187 targets in openam-rest, adds the hook as three copies that a single-class run of any other class bypasses, and turns a test-local binding with mutable static state into JVM-wide state.

Needs changing

1. openam-rest still has the old pattern. RestRouterIT.setupGuiceModules() (line 184) installs the empty loader from a @BeforeMethod(dependsOnMethods = "setupMocks") override, and neither #1187 nor this PR mentions it. It passes today only because it is the only *IT in the module, so failsafe runs it alone. If a second IT reaches InjectorHolder first, RestRouterIT will either fail in setupGuiceModules with NoClassDefFoundError or silently get the default root injector instead of the empty one. I did not check which. Please harden it the same way, or note in #1187 why it is out of scope.

2. Replace the three scripting copies with one hook. An abstract base class (e.g. EmptyLoaderGuiceTestCase extends GuiceTestCase, like CTSBasedGuiceTestCase in openam-core) with a single @BeforeSuite does the same job. TestNG 6.14.3 keys @BeforeSuite methods by java.lang.reflect.Method, so an inherited hook runs once per suite, and it still runs in a -Dtest=<Class> or IDE run of any subclass. A new validator test then gets the hook automatically, whereas today one written without it fails when run alone. This is the duplication from my note 2 on #1186, which was moot there only because a single copy was left.

   A stronger variant is a TestNG IExecutionListener registered in src/test/resources/META-INF/services/org.testng.ITestNGListener. TestNG runs execution listeners before it creates the test instances, so this would close both limits the javadoc lists (constructors and field initializers, and which classes are in the suite). Check that the surefire 3.1.2 TestNG provider picks up the service file before going this way. Either variant is fine. This is not a request to pin reuseForks; #1186 settled that.

   If you keep the copies, the javadoc has to name the runs they miss. mvn -pl openam-scripting test -Dtest=SomeOtherTest runs none of the three hooks, so a class that touches the holder builds it from the default loader and fails. The #1186 javadoc names -Dtest and IDE runs explicitly, and this one only names the fork settings.

3. Reset trustHeader now that TestGuiceModule is JVM-wide. As the new javadoc says, the module now feeds the root injector for every class in the fork. Each test sets TransactionIdConfigurationImpl.trustHeader (line 141, set at lines 90 and 111) and nothing resets it. Another class that asks the holder for TransactionIdConfiguration therefore sees whatever the last AuditContextFilterTest method left, e.g. false after shouldIgnoreTransactionIdHttpHeaderIfConfiguredToDoSo. That is the kind of order dependence this PR sets out to remove. Resetting the field in an @AfterMethod, or binding a per-test instance, closes it.

Non-blocking

4. GroovyAstTransformEscapeTest still overrides setupGuiceModules() (line 66) only to clear the canary, while its two siblings drop their overrides. Putting @BeforeMethod on clearCanary() instead would stop overriding the library's lifecycle method and keep the three classes consistent.

5. Small: the four loaders are anonymous inner classes created in instance methods. Each one captures its test instance, mocks included, and keeps it reachable from the static InjectorConfiguration for the rest of the fork. A lambda (a -> Collections.emptySet(), a -> Collections.singleton(TestGuiceModule.class)) is shorter and captures nothing.

6. The root cause sits in commons, so this is for a separate issue rather than this PR. InjectorHolder builds its injector eagerly. GuiceTestCase.setupGuiceModules() calls InjectorHolder.getInjector() only to save oldInjector, and that call forces a build from whatever loader is current. A failed build then poisons the class for the rest of the JVM. If the holder built lazily, no module would need its own hook, and openam-core-rest (#1186), the two modules here and RestRouterIT would all be covered in one place.

CI

Run 39424307815 at 8d8e4d2: all five build-maven (ubuntu-latest, *) legs fail. All four macOS and Windows legs pass, and so do the CodeQL analyses.

This PR does not cause the Ubuntu failures. All five legs stop in openam-core-rest, which the diff does not touch, with the same five failures as master at this PR's base (run 39287288644 on 37cf5ab):

• IdentityResourceNotificationTest.setupMocks fails with NoClassDefFoundError: Could not initialize class org.forgerock.guice.core.InjectorHolder. The cause is No implementation for NotificationBroker was bound, raised from SessionResourceV2Test.setUp.
• OathDevicesResourceTest, PushDevicesResourceTest, TrustedDevicesResourceTest and SmsResourceProviderTest then fail with Previously setup Realm class - forgot to call teardown.

That is the ubuntu-26.04 class-order failure that #1186 fixes. Both modules this PR changes build before the reactor stops, and both pass on those legs: openam-audit-context (AuditContextFilterTest 2/2) and openam-scripting (196 tests, 0 failures). A green Ubuntu run needs #1186 merged and this branch rebased.

[maximthomas]

Pushed ebf306f and 72fdecf on top of a merge of master, which brings in #1186.

1. RestRouterIT: out of scope, because neither failure mode exists in openam-rest. Unlike in openam-scripting, the default loader builds the root injector on that module's test classpath. Measured on master 553b73f with a scratch class (not committed) that calls InjectorHolder.getInstance(ArrayList.class) with no loader installed and runs first through a suite XML: the root injector builds, and RestRouterIT then passes 8 of 8. With its loader install deleted, RestRouterIT alone also passes 8 of 8. GuiceTestCase.setupGuiceModules() only saves the root injector and then registers the test's own, and RestRouterIT reads only from that one, so a root built from the default loader does not change its result. Its empty loader is redundant rather than a latent order bug, so the test is unchanged. I noted this in Install the empty Guice module loader before the suite in openam-scripting and openam-audit-context tests #1187.

2. One listener instead of three hooks. I went with the IExecutionListener: EmptyGuiceModuleLoaderListener, registered in openam-scripting/src/test/resources/META-INF/services/org.testng.ITestNGListener, replaces the three @BeforeSuite copies. I checked it in a throwaway project on TestNG 6.14.3 and surefire 3.1.2. The service file is picked up, and onExecutionStart runs before any test constructor in a full run, with -Dtest=<one class> (also for a class that is not a Guice test), and once per JVM with -DreuseForks=false. Neither module publishes a test-jar, so the service file stays on this module's test classpath. Both limits are gone, and so is the javadoc that listed them. The same project confirms the base-class variant as you described: an inherited @BeforeSuite ran once for two subclasses, and also ran with -Dtest=<subclass>.

3. trustHeader reset. Done: @AfterMethod(alwaysRun = true) resetTrustHeader() sets it back to true. A scratch class run after AuditContextFilterTest, with shouldIgnoreTransactionIdHttpHeaderIfConfiguredToDoSo forced last, asserts that the holder's TransactionIdConfiguration trusts the header. It passes, and with the reset removed it fails with Expecting value to be true but was false.

4. GroovyAstTransformEscapeTest override. Done: the override is gone, and clearCanary() carries both @BeforeMethod and @AfterMethod.

5. Lambdas. Done: AuditContextFilterTest installs moduleAnnotation -> Collections.singleton(TestGuiceModule.class), and the listener installs moduleAnnotation -> Collections.emptySet().

6. Lazy InjectorHolder. Agreed. Filed as InjectorHolder builds its injector during class initialization, so one failed build breaks it for the rest of the JVM commons#376.

Tests at 72fdecf, offline (scratch classes and suite XMLs not committed):

• openam-scripting:
  • a scratch class whose field initializer calls InjectorHolder, run first ahead of the three Guice classes: 20 of 20 pass. Without the service file, the same run cannot start (Cannot instantiate class ...AaaHolderCtorProbeTest);
  • that scratch class alone with -Dtest: 1 of 1;
  • each Guice class alone with -Dtest=<Class>: 5, 5 and 9 pass;
  • mvn -o -pl openam-scripting test in the default order, with -Dsurefire.runOrder=reversealphabetical, and with -DreuseForks=false: 196 tests, 0 failures each.
• openam-audit-context:
  • the item 3 run, with an unhooked scratch class that touches the holder placed first: 4 of 4;
  • default and -DreuseForks=false: 2 of 2.

[vharseko]

Re-reviewed 72fdecf against the six points from the last round. All six are addressed, so I'm approving.

1. RestRouterIT. The explanation in #1187 holds. RestRouterIT asks the holder for HttpApplication and the internal router only after super.setupGuiceModules() has registered the test's own injector (lines 235 and 364). The root injector would matter only if building it threw, and your measurement shows that the default loader builds it on the openam-rest test classpath. Leaving it out of scope is fine.

2. One listener. EmptyGuiceModuleLoaderListener replaces the three copies. Neither module publishes a test-jar, so the service file stays on this module's test classpath. GuiceTestCase.setupGuiceModules() carries @BeforeMethod itself (I checked the guice test 3.1.2 jar), so dropping the overrides keeps the per-test injector.

I reproduced your check at 72fdecf. A scratch class whose field initializer calls InjectorHolder.getInstance(ArrayList.class) ran first, ahead of the three Guice classes, with -Dsurefire.runOrder=alphabetical:

• With the service file: 20 passed, 0 failed.
• Without it, the run cannot start: Cannot instantiate class org.forgerock.openam.scripting.AaaHolderProbeTest.

3. trustHeader reset. @AfterMethod(alwaysRun = true) puts it back to true, so the flag is reset after a failed test too.

4. clearCanary(). It now has both @BeforeMethod and @AfterMethod, and the order is unchanged: TestNG runs the inherited setupGuiceModules() first, then clears the canary, as the removed override did.

5. Lambdas. Done in the listener and in AuditContextFilterTest.

6. Thanks for filing OpenIdentityPlatform/commons#376.

Locally (macOS, JDK 27), mvn -o -pl openam-scripting,openam-audit/openam-audit-context test at 72fdecf gives 196 passed and 2 passed, 0 failed.

CI at 72fdecf is green on every leg, including all five ubuntu-latest ones, now that #1186 is merged in. The branch is one commit behind master (#1189, which changes only openam-ui/openam-ui-ria/package-lock.json) and merges cleanly.