[maximthomas]

Since ubuntu-latest moved to the ubuntu-26.04 runner image, build-maven fails in openam-core-rest on every Ubuntu leg (JDK 11, 17, 21, 25, 27). It was first seen in run 39217607024 on #1023, whose changes do not touch this module: openam-core-rest there is identical to master (d75bdb8). The same tests were green on ubuntu-24.04 three days earlier, and are green on macOS and Windows in the failing run.

Cause

InjectorHolder builds its injector once per JVM, from the module loader InjectorConfiguration holds at that moment. The modules on this test classpath cannot be built (No implementation for NotificationBroker was bound). The first test that touches InjectorHolder with the default loader therefore leaves the class unusable for the rest of the fork.

SessionResourceV2Test.setUp constructs a real AMIdentity. Its constructor asks InjectorHolder for a RepoAuditorFactory and swallows the failure (AMIdentity.java:190). IdentityResourceNotificationTest and SessionResourceTest installed an empty loader, but in @BeforeClass, which only helped when they ran before SessionResourceV2Test. Surefire runs the classes in file-system order, and on ubuntu-26.04 that order puts SessionResourceV2Test first:

• IdentityResourceNotificationTest.setupMocks:155 fails with NoClassDefFoundError: Could not initialize class org.forgerock.guice.core.InjectorHolder, through IdentityServicesImpl.<clinit>.
• That @BeforeMethod fails after RealmTestHelper.setupRealmClass(), so the @AfterMethod teardown never runs. As a result, OathDevicesResourceTest, PushDevicesResourceTest, TrustedDevicesResourceTest and SmsResourceProviderTest fail with Previously setup Realm class - forgot to call teardown.

Change

Test code only.

• IdentityResourceNotificationTest installs the empty loader from @BeforeSuite(alwaysRun = true) instead of @BeforeClass. openam-core-rest sets no forkCount, parallel or suiteXmlFiles, so surefire runs all its classes in one TestNG suite, and every @BeforeSuite method runs before the first test, whatever order the classes are in. The tests get the same empty injector they got on the green runs. Only the moment the loader is installed changes.
• SessionResourceTest and SessionResourceV2Test no longer build a real AMIdentity, whose constructor reached InjectorHolder. They return a mock with getName() and getRealm() stubbed, the only two getters the session code reads, and they no longer install a loader.
• The IdentityResourceNotificationTest javadoc states the limits of the hook: TestNG instantiates every test class before any @BeforeSuite method runs, and the hook covers the other classes only while the module runs as one TestNG suite in one JVM.

Tests

mvn -o -pl openam-core-rest test -Dsurefire.runOrder=<order>, run locally on macOS:

• reversealphabetical at master puts session.SessionResourceV2Test ahead of IdentityResourceNotificationTest, as on ubuntu-26.04. Result: IdentityResourceNotificationTest.setupMocks fails with the CI error, caused at SessionResourceV2Test.setUp:112. The default order is green at master.
• With the first version of this change (5f9ef67, a @BeforeSuite hook in all three classes), reversealphabetical, alphabetical, filesystem and random with seeds 1, 42 and 20261019 all pass: 446 passed, 0 failed, 0 skipped each, including IdentityResourceNotificationTest 8/8, SessionResourceTest 33/33 and SessionResourceV2Test 26/26.
• Each of the three classes alone (-Dtest=<class>) passes.
• At 0c41e0a (after review): reversealphabetical, the default order and -DreuseForks=false (53 classes, each in its own JVM) all pass, 446/0/0. With the IdentityResourceNotificationTest hook put back on @BeforeClass, reversealphabetical still passes (446/0), so neither session class reaches InjectorHolder. With SessionResourceV2Test building a real AMIdentity again, the same run fails as on ubuntu-26.04.

Locally, the ubuntu-26.04 order is only approximated by reversealphabetical. This PR's own CI run checks the real image.

[vharseko]

The fix is right. openam-core-rest inherits forkCount=1 and reuseForks=true, so surefire runs all of its classes in one TestNG suite, and the three @BeforeSuite hooks run before the first test whatever the class order. I also ran the module at this head with -DreuseForks=false, which gives each class its own JVM and suite. All 53 classes and 446 tests pass, so no class without the hook relies on another class's hook today.

A few non-blocking notes, for this PR or a follow-up:

1. The session tests could stop touching InjectorHolder altogether. SessionResourceTest.setUp (line 156) and SessionResourceV2Test.setUp (line 134) build a real AMIdentity only to return it from the stubbed getIdentity(). The resource code reads just getName() and getRealm() from it (SessionResourceUtil.java:135-136). A mock(AMIdentity.class) with those two stubs never runs the constructor that asks InjectorHolder for a RepoAuditorFactory. The hooks in both session classes would then not be needed.

2. The empty loader is now copied into a third class. The same anonymous GuiceModuleLoader block is in IdentityResourceNotificationTest, SessionResourceTest and SessionResourceV2Test. One shared helper would leave a single place to change, for example next to RealmTestHelper in the openam-core test-jar that this module already uses.

3. The javadoc on IdentityResourceNotificationTest.setupGuiceModuleLoader claims more than the build guarantees. It says "What every test class in the same fork gets is therefore an empty injector". That holds only while the module keeps the inherited forkCount=1 and reuseForks=true, and while one of the three hooked classes is in the run. Nothing pins either:

   • With reuseForks=false or forkCount>1, surefire's TestNG provider runs each class in a suite of its own.
   • A -Dtest=<other class> run or an IDE single-class run has no hook at all.

   It is worth saying so in the comment, or pinning the settings in openam-core-rest/pom.xml.

4. The same hazard remains in other modules. These tests install their loader before the class or the method, not before the suite:

   • IdRepoTestBase (openam-datastore) and AuditContextFilterTest (openam-audit-context) use @BeforeClass.
   • GroovyValidatorTest, JavaScriptValidatorTest and GroovyAstTransformEscapeTest (openam-scripting) use @BeforeMethod.

   They are green on ubuntu-26.04 in this PR's run, but only because of the order their classes come in, and that order is what changed under openam-core-rest. A follow-up issue would keep this from surfacing the same way.

5. A caveat dropped from the SessionResourceTest javadoc still applies. The old text ended with "this is a no-op when another class got there first". With @BeforeSuite that is still true in one case: TestNG creates an instance of every test class before it runs any @BeforeSuite method. So a constructor, field initializer or static initializer of a test class that reaches InjectorHolder still builds the injector from the default loader first. One example is a field mock(IdentityServicesImpl.class), whose static fields call InjectorHolder.getInstance. One sentence on that in the comment would save the next person a search.

[maximthomas]

Pushed 0c41e0a.

1. Done. Both session classes now stub getName() and getRealm() on mock(AMIdentity.class), and their hooks are gone. To check that neither of them reaches InjectorHolder now, I put the IdentityResourceNotificationTest hook back on @BeforeClass and ran the module in reversealphabetical order: 446 passed, 0 failed. The same run with SessionResourceV2Test building a real AMIdentity again fails at IdentityResourceNotificationTest.setupMocks with NoClassDefFoundError: Could not initialize class org.forgerock.guice.core.InjectorHolder, as on ubuntu-26.04.

2. After 1, the empty loader is left in IdentityResourceNotificationTest only, so there is nothing to share.

3. I changed the comment, not the pom. The javadoc now says the hook covers the other classes only while the module runs as one TestNG suite in one JVM. It names reuseForks=false, forkCount above 1, -Dtest and IDE runs as the cases outside that. A literal <reuseForks>true</reuseForks> in the pom would also override -DreuseForks=false on the command line, which is the isolation run you used.

4. I checked this by measurement, with the class orders fixed by a TestNG suite XML. No order of a module's own classes fails today.

   • In openam-datastore no test reaches InjectorHolder, so nothing reads the IdRepoTestBase loader.
   • In openam-scripting the three GuiceTestCase subclasses install the loader in their setupGuiceModules() override before calling super, which is the first call to reach the holder. None of the other 23 classes touches it.
   • In openam-audit-context, AuditContextFilterTest is the only test class.

   A scratch class that touches the holder without a hook and runs first makes the last two fail:

   • scripting with NoClassDefFoundError: Could not initialize class org.forgerock.guice.core.InjectorHolder, because the default loader cannot build there (NotificationBroker);
   • audit-context with No implementation for TransactionIdConfiguration was bound, because the empty default injector is built before the @BeforeClass hook runs.

   So the hazard is latent. Install the empty Guice module loader before the suite in openam-scripting and openam-audit-context tests #1187 covers hardening these two modules.

5. Added to the same javadoc, with mock(IdentityServicesImpl.class) as the example. A probe with two test classes printed both constructors before either @BeforeSuite method.

At 0c41e0a, locally: reversealphabetical, the default order and -DreuseForks=false (53 classes) all give 446 passed, 0 failed, 0 skipped.

[vharseko]

Re-reviewed 0c41e0a against the five notes from the last round. All five are addressed, and my approval stands.

1. Mocked AMIdentity. The session code reads only getName() and getRealm() from the identity (SessionResourceUtil.java:135-136, ValidateActionHandler.java:120-121, AbstractSessionPropertiesActionHandler.java:65), so the two stubs cover it. getName() returns demo, as the real object did. The getRealm() value never reaches an assertion, because both classes override convertDNToRealm. It still has to be non-null in SessionResourceTest, where the validate tests stub convertDNToRealm(anyString()), and anyString() does not match null.

2. With the loader left only in IdentityResourceNotificationTest, there is nothing to share.

3 and 5. Javadoc. It now states the one-suite, one-JVM condition, names the runs that fall outside it, and covers the instantiation order. Agreed on not pinning reuseForks in the pom, for the reason you give.

4. #1187 records the latent cases in openam-scripting and openam-audit-context, with the measurements behind them.

I reran the module at 0c41e0a locally (macOS, JDK 27) and read the class order from TEST-TestSuite.xml:

• reversealphabetical as is: 446 passed, 0 failed.
• With the IdentityResourceNotificationTest hook put back on @BeforeClass, reversealphabetical runs that class last, after all 51 others, including SessionResourceV2Test and SessionResourceTest. It passes, 446/0. So no class of the module reaches InjectorHolder with the default loader today, and the @BeforeSuite hook guards against the next one.
• The same run with SessionResourceV2Test building a real AMIdentity again fails in IdentityResourceNotificationTest.setupMocks with NoClassDefFoundError: Could not initialize class org.forgerock.guice.core.InjectorHolder, through IdentityServicesImpl.<clinit>. That is the CI failure.

CI at 0c41e0a is green on every leg, including all five ubuntu-latest ones.

Two optional wording nits. Neither blocks the merge.

• The new comment in both setUp methods says the AMIdentity constructor "leaves InjectorHolder unusable for every later test class in the JVM". That happens only when the constructor is the first call to reach the holder and no loader is installed yet. In a full-module run the @BeforeSuite hook comes first, and a real AMIdentity would just get the empty injector. The comment could keep the condition, as the removed javadoc did ("If this class is the first in the fork to touch the injector holder").
• The javadoc says "the static fields of that class ask InjectorHolder for instances". IdentityServicesImpl has one such static field, RESOURCE_MAPPING_HANDLER (line 108). Its other InjectorHolder call, idServicesErrorHandler, is an instance field, and creating a mock does not initialize it. The plural comes from my note in the last round.

[maximthomas]

Both wording nits are fixed in 31b6c89. The change touches comments only.

• The setUp comment in SessionResourceTest and SessionResourceV2Test keeps the condition again: the AMIdentity constructor leaves InjectorHolder unusable only if it is the first call to reach the holder in the JVM, before any loader is installed.
• The IdentityResourceNotificationTest javadoc now says that a static field of IdentityServicesImpl asks InjectorHolder for an instance. RESOURCE_MAPPING_HANDLER (line 108) is the only one. idServicesErrorHandler (line 105) is an instance field.

mvn -o -pl openam-core-rest test -Dsurefire.runOrder=reversealphabetical at 31b6c89: 446 passed, 0 failed, 0 skipped.

[vharseko]

Checked 31b6c89 against the two wording nits from the last round. Both are fixed, and the commit changes comment lines only.

• The setUp comment in SessionResourceTest and SessionResourceV2Test states the condition again: the AMIdentity constructor leaves InjectorHolder unusable only when it is the first call to reach the holder in the JVM, before any loader is installed.
• The IdentityResourceNotificationTest javadoc now matches the code. RESOURCE_MAPPING_HANDLER (IdentityServicesImpl.java:108) is the only static field that asks InjectorHolder for an instance. idServicesErrorHandler (line 105) is an instance field, and creating a mock does not initialize it.

CI at 31b6c89 is green on every leg, including all five ubuntu-latest ones. The branch is one commit behind master (#1176, which does not touch openam-core-rest) and merges cleanly. My approval stands.