[mgaffigan]

Several tests would fail if run in a shared JVM depending on what tests ran before them. Traced to setting mocks without cleaning them up.

As it happens, those were the only uses of Guice in the system. Removed. Revised model matches the JavaScriptBuilder.setControllersForTesting precedent.

Found during #449

[github-actions]

Test Results

133 files  + 1  133 suites  +1   3m 56s ⏱️ +52s
817 tests +11  817 ✅ +11  0 💤 ±0  0 ❌ ±0 
943 runs  +11  937 ✅ +11  6 💤 ±0  0 ❌ ±0 

Results for commit be46261. ± Comparison against base commit 056475c.

♻️ This comment has been updated with latest results.

[NicoPiel]

Clean ups:

• Wrong copy-pasted comments. All four TODOs in DatabaseConnectionPoolTests say “leaks channelController”, but they are on the DonkeyConnectionPools, Donkey, ControllerFactory and SqlConfig overrides.
• Stale Guice references left behind:
  • server/docs/thirdparty/THIRD-PARTY-README.txt:29 still lists “Guice 4.1”. The library no longer ships, so this line should go.
  • gradle/vendored-layout.json still has com.google.inject:guice (lines 132 and 186). It doesn’t break the build, because the check only fails on missing entries, but it’s dead config now.
  • gradle/verification-metadata.xml still has entries for guice, guice-parent and javax.inject:1. These are harmless but stale, and can be regenerated.

[mgaffigan]

Oops. I was missing the push to remove the stale references.

[gibson9583]

Looks safe to merge.

[tonygermano]

Thanks for tracking these leaks down. Restoring the singletons after each test is the right fix, and dropping Guice is a nice bonus. My concern is how the overrides are installed.

This PR adds a public static set*ForTesting method to five production classes: Donkey, DonkeyConnectionPools, ChannelController, ControllerFactory and SqlConfig. @VisibleForTesting is only documentation. Those methods ship in the server jars, and any plugin or user code can call ControllerFactory.setFactoryForTesting(...) on a live server to replace a core singleton. Public methods marked @VisibleForTesting are an anti-pattern, and I'd rather not grow the API surface we'll have to deprecate later. When JavaScriptBuilder.setControllersForTesting went in, I let it through as a one-off to avoid a larger refactor. I don't want it to become the pattern.

Guice was never doing real dependency injection here. requestStaticInjection used reflection to write a value into a private static field. The closest replacement is to do that same reflection from test code, so production classes don't change at all beyond losing @Inject.

Proposal: a test-only StaticOverride helper

I pushed what I'm proposing to your branch as an amend! commit on top of yours, so it's easier to review in this PR: ebdab3a

It removes the five set*ForTesting methods, so the only production change left is dropping @Inject. It adds StaticOverride, a single helper at donkey/src/testFixtures/java/com/mirth/connect/donkey/testing/StaticOverride.java, in a test-fixtures source set, so nothing from it ships. Tests then use it like this:

private static StaticOverride factoryOverride;

@BeforeClass
public static void setup() {
    ...
    factoryOverride = StaticOverride.install(ControllerFactory.class, "factory", controllerFactory);
}

@AfterClass
public static void restoreControllerFactory() {
    if (factoryOverride != null) {
        factoryOverride.close();
    }
}

What this gets us:

• Production classes get no new methods, and the five near-identical swap-and-restore lambdas go away.
• The helper locks on the class object, as the methods in this PR do, so it doesn't race with the synchronized (X.class) blocks in the getInstance() methods.
• close() is safe to call twice. The null check in teardown keeps an earlier setup failure from being hidden by a NullPointerException.
• Neither install nor close throws a checked exception, because a wrong field name is a programming error. Setup and teardown methods don't need throws Exception.

The trade-offs are the same as with Guice. The field name is a string, so renaming a field breaks the tests at runtime with an IllegalArgumentException that names the missing field, rather than at compile time. setAccessible would need --add-opens if the server is ever modularized.

This commit has to be autosquashed into yours before the PR is merged. We rebase-merge, so otherwise main would get the five setters added in one commit and removed in the next, plus a commit titled amend! .... Once the review is settled, update your local branch and squash it:

git fetch origin
git reset --hard origin/feat/singleton-test-hooks
git rebase -i --autosquash HEAD~2
git push --force-with-lease

(use whichever remote points at your fork in place of origin), and accept the todo list as is. Git keeps you as the author and replaces your commit message with one that describes the whole PR. The new message keeps your Signed-off-by and adds mine, so please read it and edit it however you like before you push. amend! commits need Git 2.32 or newer.

Happy to push the squashed result to your branch instead if you'd prefer. Just say so. The new message keeps your Signed-off-by, so I'd rather you agree to that first.

JavaScriptBuilder.setControllersForTesting can go in the same PR, but that commit doesn't touch it. For that class, the better fix is to stop copying the controllers into static fields at class-init time (lines 46–47) and look them up through ControllerFactory.getFactory() when they're needed. Then overriding ControllerFactory covers it, and the mocks it copied no longer leak into later tests.

Helper source

package com.mirth.connect.donkey.testing;

/**
 * Temporarily replaces the value of a static field, such as a singleton instance, for the duration
 * of a test.
 */
public final class StaticOverride implements AutoCloseable {
    private final Class<?> owner;
    private final Field field;
    private final Object value;
    private final Object previous;
    private final AtomicBoolean closed = new AtomicBoolean();

    private StaticOverride(Class<?> owner, Field field, Object value, Object previous) {
        this.owner = owner;
        this.field = field;
        this.value = value;
        this.previous = previous;
    }

    /**
     * Sets a static field to {@code value} until the returned handle is closed, then puts the
     * previous value back. Closing more than once does nothing.
     *
     * @throws IllegalArgumentException
     *             if the field does not exist, is not a non-final static field, or cannot hold
     *             {@code value}
     */
    public static StaticOverride install(Class<?> owner, String fieldName, Object value) {
        Field field;
        try {
            field = owner.getDeclaredField(fieldName);
        } catch (NoSuchFieldException e) {
            throw new IllegalArgumentException(owner.getName() + " has no field named " + fieldName, e);
        }

        int modifiers = field.getModifiers();
        if (!Modifier.isStatic(modifiers) || Modifier.isFinal(modifiers)) {
            throw new IllegalArgumentException(owner.getName() + "." + fieldName + " is not a non-final static field");
        }
        field.setAccessible(true);

        synchronized (owner) {
            Object previous = get(field);
            set(field, value);
            return new StaticOverride(owner, field, value, previous);
        }
    }

    /**
     * Puts back the value the field had before this override was installed.
     *
     * @throws IllegalStateException
     *             if something else replaced the field's value after this override was installed
     */
    @Override
    public void close() {
        if (!closed.compareAndSet(false, true)) {
            return;
        }

        synchronized (owner) {
            Object current = get(field);
            if (current != value) {
                throw new IllegalStateException(owner.getName() + "." + field.getName() + " was replaced by " + current + " while " + value + " was installed");
            }
            set(field, previous);
        }
    }

    private static Object get(Field field) {
        try {
            return field.get(null);
        } catch (IllegalAccessException e) {
            throw new IllegalStateException(e);
        }
    }

    private static void set(Field field, Object value) {
        try {
            field.set(null, value);
        } catch (IllegalAccessException e) {
            throw new IllegalStateException(e);
        }
    }
}

Gradle wiring

In donkey/build.gradle:

apply plugin: 'java-test-fixtures'

// The test-fixtures plugin gives the test source set the main classes through
// a dependency on this project, which the non-transitive test classpaths
// drop, so add them back directly. Remove this when transitive resolution
// is enabled.
sourceSets.test {
    compileClasspath += sourceSets.main.output
    runtimeClasspath += sourceSets.main.output
}

In server/build.gradle:

testImplementation testFixtures(project(':donkey'))

Without the sourceSets.test lines, donkey's own tests stop compiling. The root build sets the test classpaths to transitive = false, which drops the route the plugin uses to put donkey's main classes on the test classpath. On the server side, non-transitive resolution works in our favor: it pulls in only the fixtures jar, not a second copy of donkey.

The full build passes with the commit, and nothing from the helper ends up in the distribution.

What Claude checked

Besides the helper and the Gradle wiring, the commit:

• switches every call site in the tests to StaticOverride.install: ChannelTest, DatabaseConnectionPoolTests, and 14 other server test classes
• changes the override fields to StaticOverride, null-checks them in teardown, and drops throws Exception from the restore methods
• adds a unit test for the helper at donkey/src/test/java/com/mirth/connect/donkey/testing/StaticOverrideTest.java
• keeps the six handles DatabaseConnectionPoolTests installs and closes them, newest first, in an @After method, which also removes its TODO: this leaks ... comments

Results at that commit:

Check	Result
StaticOverrideTest (11 cases)	Pass
ChannelTest	Pass (1)
Converted server *Test classes	All 13 pass (78 tests)
./gradlew clean build	Pass, with no failures: donkey 34 tests, server 675, client 18, command 6
Jars containing StaticOverride	Only donkey-4.7.0-test-fixtures.jar. None of the jars the build produces for the distribution contain it.
DatabaseConnectionPoolTests (run directly with JUnit, since the build skips it)	All 10 still fail with Could not find resource SqlMapConfig.xml, as on this PR's head.
DatabaseReceiverInvalidColumnNameTests (run directly, after DatabaseConnectionPoolTests)	Gets through @BeforeClass now. Its test fails only because there's no Postgres at localhost:5432, the same as when it runs alone on this PR's head.

The 11 StaticOverrideTest cases cover:

• restoring a previous value, and restoring null
• a volatile field
• nested overrides closed in order, and failing when closed out of order
• a second close() doing nothing
• failing when something else replaced the value while the override was installed
• rejecting final, non-static, missing and wrong-typed fields

Longer term, I'd like to move toward constructor injection, one class at a time: a no-arg constructor that delegates to the factory, plus a constructor that takes the dependencies so tests can pass mocks. Each conversion would remove a use of this helper. That's out of scope here, though.

Other items

• The test task only includes **/*Test.class, so DatabaseConnectionPoolTests and DatabaseReceiverInvalidColumnNameTests never run in the build. Run directly with JUnit on this PR's head:

  • All 10 DatabaseConnectionPoolTests cases fail with Could not find resource SqlMapConfig.xml.
  • DatabaseReceiverInvalidColumnNameTests needs a live Postgres at localhost:5432. Run alone, it fails connecting to it. Run after DatabaseConnectionPoolTests, it fails earlier, in @BeforeClass, with a Mockito UnfinishedStubbingException, because DatabaseConnectionPoolTests never restored its overrides. The @AfterClass then throws a NullPointerException on factoryOverride.close(), which hides that error. The commit above fixes both: DatabaseConnectionPoolTests now restores its overrides, and the teardown null-checks the handle.

  Even so, the build can't check the changes to these two classes. Either fix DatabaseConnectionPoolTests and rename both so they run (the receiver test would need a database, or a mock in place of one), or leave them out of this PR.

• Removing Guice from the donkey-main and server-main bundles also takes guice-4.1.0.jar out of the shipped server lib. I'm fine with dropping it, but a plugin that relied on getting Guice from the server classpath would break. Please call this out in the PR description so it makes the release notes.

---

🤖 Drafted with Claude Code

[mgaffigan]

@tonygermano, whatever gets it merged is fine by me. I do not share your opposition to test infrastructure living in shipped artifacts, and thus do not like reflection for this use case - but the risk seems limited here.

Also: Given the significant departure in methodology (this was not a refinement or correction of my PR but an alternative design), I believe it would have made more sense to open an alternate PR. I take no offense in such things. Collaboration can cross PR #'s. Whichever does not reach quorum can close.

TL;DR: ship it.

[tonygermano]

rebased and included an update due to #439 merging and introducing new tests using Guice with the same pattern this PR addresses. They were in ControllerFactory.factory and SqlConfig.instance.