TestHelper: load plugins via JUnit extension; cleanup#7580
Conversation
`TestHelper.loadPlugins()` was already idempotent — a `boolean loaded`
flag guarded the expensive `Main.createLoadPluginTask().run()` call so
only the first invocation did real work. The remaining cleanup turns
that runtime guard into a structural one:
* Replace the mutable `boolean loaded` field + `if (!loaded)` branch
with a private static initializer. Plugin loading now runs exactly
once when `TestHelper` is first class-loaded, enforced by the JVM
rather than by a flag check that future maintainers could bypass.
* Drop the two internal `loadPlugins()` calls in `makeEquipment` and
`makeAbilityFromString` — they're inside the same class that defines
the no-op, so by the time the JVM is executing those methods the
static initializer has already run.
* Keep the no-op `loadPlugins()` method and the 12 external call sites.
In most callers `loadPlugins()` is the only `TestHelper` reference, so
removing it would let unused-import cleanup strip `import TestHelper`,
which would in turn prevent class-loading and skip the static
initializer. The "redundant" calls are load-bearing: they keep
`TestHelper` on each test's class-load graph.
* Add an inline comment to silence SonarQube S1186 ("Methods should not
be empty") and to record the load-bearing rationale in-place.
- Remove unused imports (BufferedReader, FileInputStream, InputStreamReader, PersistenceLayerException) - Drop unthrown 'throws PersistenceLayerException' from parsePCClassText - Parameterize Class<?> in createSource - Replace string-based "Object".equals(clazz.getName()) walk-stop with identity compare clazz != Object.class - Inline redundant pccFilesPath local in findDataFolder - Switch three concatenated LOG.info calls to parameterized LOG.log - Fill in missing @param entries on hasWeaponProfKeyed
PCClassTest and AddClassSkillsTest each carried a private duplicate of the same parsePCClassText helper, declared with an unthrown PersistenceLayerException. Now that the canonical helper in TestHelper has had its stale throws removed, the local copies are pure dead code and the AddClassSkillsTest try/catch was already unreachable.
…loadGameModes None of the three are referenced anywhere in the repo (no callers, no reflective lookups). Removing them also lets us drop the unused abLoader and source fields plus six imports they pulled in (AbilityLoader, CampaignFileLoader, ConfigurationSettings, PCGenTask, PropertyContextFactory, SystemUtils).
TestFX's ApplicationExtension.afterEach calls cleanupParameters, which
does setAccessible(true) on the private static ParametersImpl.params
field — deep reflection that --add-exports doesn't grant. Pre-Java 25
this slipped through; on JavaFX 25 it throws InaccessibleObjectException
("module javafx.graphics does not opens com.sun.javafx.application to
unnamed module") and crashes the test JVM, which surfaces downstream as
"non-zero exit value 1" with no test failures reported.
Replace --add-exports with --add-opens for that single package; the rest
of the JVM-arg block is correct.
Also revert the temporary --stacktrace --info debug flags from the test
workflow (they were a one-shot diagnostic, not a permanent setting; they
balloon the CI log to ~100k lines).
|
Diagnosed the failure — it isn't actually caused by this PR's static-initializer change. The Full stack in run 26866284491. The What's wrong
Why this PR triggered a latent bugThe pre-existing FixPushed
Should be green now. Worth a re-run. |
|
The TestFX Looking at the diff again, this is almost certainly the original concern about the static initializer:
Just pushed |
|
The diagnostic flags exposed the cause. From the ``` Then the JVM exits — that SEVERE message is from WhyLook at the proper init order inside ```java
```java In the OLD code, step [1] just returned the data folder path — In the NEW code, step [1] triggers TestHelper.<clinit> which immediately runs RecommendationRevert the static-initializer approach. The stated goal — "plugins load once automatically without each test having to call
Option 1 is the smallest change and gets the build green immediately. Options 2 and 3 are nicer if you want to address the original ergonomic concern. Want me to push option 1 as a "revert + targeted improvements" commit on this branch, or do you want to take it from here? |
The plugin/game-mode/campaign loader sequence was duplicated verbatim in startupWithGUI() and startupWithoutGUI(). Pull it into a single package-private helper so test code can run the same sequence and the order can never drift between the two callers. Behaviour-preserving refactor; no production changes.
Loads PCGen plugins exactly once per JVM via a BeforeAllCallback, keyed on ExtensionContext.Store.GLOBAL so the work happens at most once across the whole test JVM. Auto-discovered via META-INF/services and the autodetection.enabled property, so test classes don't need any annotation to opt in. This is the proper replacement for the static initializer the previous commit introduced: BeforeAllCallback fires after JUnit's lifecycle is set up but before any user @BeforeAll runs, so it no longer perturbs the init order that DataTest/DataLoadTest require.
The static initializer ran Main.createLoadPluginTask() at class-load time — earlier than DataTest/DataLoadTest's expected init order, which caused Main.loadProperties() to find no settings file and call GracefulExit.exit(1), tearing down the test JVM with no stack trace. PCGenTestEnvironment (added in the previous commit) now owns plugin loading, runs at the correct point in the JUnit lifecycle, and is auto-discovered for every test class. Keep loadPlugins() as a deprecated no-op so existing call sites keep compiling — they can be cleaned up incrementally rather than all in one go.
The two loadGameModes() helpers each open-coded the same 4-step sequence (createLoadPluginTask + GameModeFileLoader + CampaignFileLoader, each constructed and run individually). Replace with the runBootstrapTasks() helper extracted in the earlier commit, so all three call sites (production GUI, production headless, test) share one definition of the canonical bootstrap order. Make runBootstrapTasks public — it's the same visibility the existing loadProperties() and createLoadPluginTask() helpers already have.
Auto-discovery via META-INF/services would force plugin loading on EVERY test class in the JVM, including ones that explicitly assume plugins are NOT loaded. Concrete example: SetSolverManagerTest.setUp constructs a fresh VariableContext and calls addFunction(getOther), which fails with 'Cannot load two functions of name: getOther' if PluginFunctionLibrary already has it (plugin loading populates that singleton, and VariableContext's constructor copies all entries from it). Make plugin loading opt-in: @ExtendWith(PCGenTestEnvironment.class) on classes that need it. Apply to AbstractCharacterTestCase and AbstractJunit5CharacterTestCase (lifts ~114 subclasses) plus the 10 standalone test classes that previously called loadPlugins() explicitly. Drop the META-INF/services + junit-platform.properties files.
|
Rewrote the static-initializer approach as five logical commits (572dde5..d02509c). All four test source sets (`:test`, `:itest`, `:datatest`, `:slowtest`) pass locally. Summary
Why opt-in instead of auto-discoveryI initially wired auto-discovery via `META-INF/services` so every test class would get plugins for free. That broke `SetSolverManagerTest`: ``` `VariableContext`'s constructor copies all functions from the global `PluginFunctionLibrary` singleton (`code/src/java/pcgen/rules/context/VariableContext.java:147`). `SetSolverManagerTest` builds a fresh `VariableContext` and adds `GetOtherFunction` itself — assuming the singleton was empty. The test was relying on plugins NOT being loaded. Auto-discovery breaks that assumption. The OLD code already had this property: `loadPlugins()` was opt-in (each test that wanted plugins called it from `@BeforeEach`/`@BeforeAll`). Tests that didn't call it ran without plugins, and `SetSolverManagerTest` was one of them. The `@ExtendWith` annotation preserves that boundary while removing the boilerplate. Where the annotation is applied
Verification``` (`:slowtest` took 11 min; the other three a couple minutes total.) |
No call sites remain in the tree. The previous commit kept it as a deprecated shim only to avoid touching every test class in one go; that's now done. Drop it.
Costs nothing on green builds (only fires on failure), saves a diagnostic round-trip when CI fails for a non-obvious reason. This session needed two separate diagnostic commits (one for the TestFX/JavaFX 25 InaccessibleObjectException and one for the TestHelper static-initializer-induced JVM exit) precisely because neither failure showed a stack trace by default — Gradle just said 'non-zero exit value 1' and pointed at --stacktrace as the next step. Make that the next step happen automatically.
Vest
changed the title
ExtensionContext.Store#getOrComputeIfAbsent was deprecated in JUnit 6.0 in favor of #computeIfAbsent (mirrors java.util.Map). Three other files still imported junit.framework.TestCase / org.junit.Test from JUnit 3/4 even though the rest of their bodies were already on Jupiter. PCGenTestEnvironment switches to computeIfAbsent. AbstractFormulaTestCase and TestUtilities drop junit.framework.TestCase.fail in favor of Jupiter Assertions.fail. RecursiveFileFinderTest swaps org.junit.Test for the Jupiter @test.
JUnit 6 drops junit:junit from the classpath, so the org.junit.Assert static helpers used by 86 test files no longer resolve. Each file now imports the equivalent from org.junit.jupiter.api.Assertions. Jupiter reverses the message argument convention: JUnit 4's assertEquals(message, expected, actual) becomes assertEquals(expected, actual, message). Mass-applying the import swap alone would silently match Jupiter's assertEquals(Object, Object, String) overload with `message` read as `expected` and vice versa - the calls would compile but every failure message would be wrong. The 1054 swap-able call sites (assertEquals/assertTrue/assertFalse/assertNull/ assertNotNull/assertSame/assertArrayEquals with leading String literal) are reordered to match Jupiter's convention. Nine sites where the leading message was a String concatenation expression rather than a literal were caught by the compiler and fixed by hand.
With every test file now on Jupiter, neither the junit:junit jar nor the junit-vintage-engine runner is needed. The stale comment claiming "~870 legacy tests" is removed - the actual count was zero by the time the migration finished.
2 participants
Summary
Replace the manual
TestHelper.loadPlugins()boilerplate with a proper JUnit 5 lifecycle hook. Originally tried a static initializer (early commits on this branch); that turned out to load plugins at JVM class-load time, before test setup — which broke:datatest(settings-file race) and:test(TestFX tearDown trying deep reflection without--add-opens). Rewrote using aBeforeAllCallbackextension instead. Also keeps the orthogonal cleanups from the original PR: dead-method removal andparsePCClassTextdeduplication.The PR also finishes the long-pending JUnit 4 → 6 migration (last three commits) — the
getOrComputeIfAbsentdeprecation warning that surfaced while building this branch is what kicked it off.OK to squash, but the commit history is split into logical groups if reviewers prefer to keep them.
1. JUnit 5 extension (
PCGenTestEnvironment)code/src/testcommon/pcgen/test/PCGenTestEnvironment.java— aBeforeAllCallbackthat runsMain.createLoadPluginTask().run()exactly once per JVM, keyed viaExtensionContext.Store.GLOBAL.@ExtendWith(PCGenTestEnvironment.class), NOT auto-discovered. Auto-discovery would force plugin loading on tests likeSetSolverManagerTest.setUpthat explicitly assume an emptyPluginFunctionLibrary(itsaddFunction(getOther)collides with the entry that plugin loading puts in the singleton). The originalloadPlugins()was opt-in too — each test that wanted plugins called it explicitly. The annotation preserves that boundary while removing the boilerplate.AbstractCharacterTestCaseandAbstractJunit5CharacterTestCase. The annotation propagates to ~114 subclasses automatically.TestHelper.loadPlugins().TestHelper.loadPlugins()and the static initializer that briefly replaced it are both deleted. No call sites remain.2.
Main.runBootstrapTasks()The 4-step bootstrap sequence (
createLoadPluginTask→GameModeFileLoader→CampaignFileLoader, run viaPCGenTaskExecutor) was duplicated verbatim instartupWithGUI(),startupWithoutGUI(), and open-coded twice more inDataTest.loadGameModes()/DataLoadTest.loadGameModes(). Pulled into oneMain.runBootstrapTasks(PCGenTaskListener…)helper. All four call sites now use it. Eliminates the risk of one of them drifting out of order — the bug we just hit when the static-initializer attempt loaded plugins beforeMain.loadProperties().3. TestFX/JavaFX 25 fix
Separately,
:testwas crashing withInaccessibleObjectException: module javafx.graphics does not "opens" com.sun.javafx.applicationfrom TestFX'scleanupParameters(which usessetAccessible(true)on a private static field). The existing--add-exportsflag inbuild.gradle:684wasn't enough —setAccessibleneeds--add-opens. Changed--add-exports→--add-opensforcom.sun.javafx.application. The other three--add-exportsflags in the JVM-arg block stay as-is (they're for Monocle and JavaFX logging, which only need export-level access).4.
--stacktraceon every CI gradle invocationCosts nothing on green builds (only fires on failure). This session needed two separate diagnostic commits — one for the TestFX
InaccessibleObjectException, one for the static-initializer-induced JVM exit — purely because Gradle's default output for a crashed test JVM isnon-zero exit value 1with no further detail, plus a hint to use--stacktrace. Make that the next step happen automatically.5. Original cleanup work (kept from earlier in the PR)
These were done before the static-initializer attempt and are unrelated to it. Worth keeping:
TestHelper:makeAbilityFromString,makeWeaponProf,loadGameModes(String)— zero callers anywhere in the tree, last real users removed years ago (2014, 2009, 2018 respectively). Recoverable from git viagit log -Sif anyone needs them.parsePCClassTextdedup:PCClassTestandAddClassSkillsTestcarried near-identical 17-line copies. Both now route through the canonicalTestHelper.parsePCClassText. -62/+5 lines net.throws, raw-type → parameterizedClass<?>, string-based class-walk replaced by identity compare, redundant local inlined, parameterized log calls, missing Javadoc filled in.6. JUnit 4 → 6 migration (final three commits)
The BOM was already on
org.junit:junit-bom:6.1.0, but several pieces of test scaffolding still referenced JUnit 3/4 APIs. The build comment claiming "~870 legacy tests" was stale by years — actual count of pure-JUnit-4 files at the start of this work was 1, plus 86 hybrid files usingorg.junit.Assert.*static imports.Deprecated APIs in test scaffolding (
aca6bc17a5):ExtensionContext.Store#getOrComputeIfAbsentwas deprecated in JUnit 6.0 in favor of#computeIfAbsent(mirrorsjava.util.Map) —PCGenTestEnvironmentswapped over.AbstractFormulaTestCaseandTestUtilitiesdroppedjunit.framework.TestCase.failfor JupiterAssertions.fail.RecursiveFileFinderTestswappedorg.junit.Testfor the Jupiter@Test.Bulk
Assert→Assertionsrewrite (efeb5e11a5): 86 files, 1054 message-arg position swaps. JUnit 4'sassertEquals(message, expected, actual)becomes Jupiter'sassertEquals(expected, actual, message). The trap is that mass-rewriting the imports alone would compile cleanly because Jupiter'sassertEquals(Object, Object, String)overload silently matches the old shape withmessageinterpreted asexpectedand vice versa — every failure message would be wrong, but the test class would still pass when assertions hold. The script (paren-balanced parser, top-level comma split) only swaps when the first arg is a string literal and the arity matches the JUnit-4 leading-message shape (3+ forassertEquals/assertSame/assertNotSame/assertArrayEquals, 2 for the rest). Nine cases where the leading message was a+-concatenation expression rather than a literal were caught by the compiler (theassertTrue(BooleanSupplier, ...)overload doesn't accept a String first arg) and fixed by hand inPreRaceTestandEquipmentSetFacadeImplTest.Drop the dead deps (
ac6034694c):junit:junit:4.13.2andjunit-vintage-enginecome out of bothbuild.gradlefiles. Stale comment removed.Test plan
./gradlew :test :itest :datatest :slowtest— all four green locally before the JUnit 6 commits (:slowtest~11 min, others a couple minutes total).build/test-results/{test,itest,datatest,slowtest}/— no failures, no errors, no skipped tests beyond the existing baseline.compileTestJava,compileSlowtestJava,compileItestJavaall clean;:test,:itest, and a representative:slowtestset (StatListTest,PrereqHandlerTest,PObjectTest,PreRaceTest) green.:slowtestand:datatestre-run after the JUnit 6 commits — the migration is mechanical but worth confirming end-to-end.Build PCGen with Gradleworkflow.