Scijava 36.0.0 breaks Mockito Wrapper tests

[mdoube]

Describe the bug Updating pom.xml to use scijava 36.0.0 leads to the Mockito wrapper tests failing when running mvn test -P allTests

Expected behavior All the tests pass

Additional context A large number of these errors are produced

[ERROR]   ThicknessWrapperTest.test2DImageCancelsPlugin:69 
Wanted but not invoked:
userInterface.dialogPrompt(
    <any string>,
    <any string>,
    <any>,
    <any>
);
-> at org.bonej.wrapperPlugins.CommonWrapperTests.test2DImagePlusCancelsPlugin(CommonWrapperTests.java:222)

However, there was exactly 1 interaction with this mock:
userInterface.isVisible();
-> at org.scijava.ui.DefaultUIService.getVisibleUI(DefaultUIService.java:550)

[mdoube]

The offending methods in this example are: -> at org.bonej.wrapperPlugins.CommonWrapperTests.test2DImagePlusCancelsPlugin(CommonWrapperTests.java:222)

    static <C extends Command> void test2DImagePlusCancelsPlugin(
        final Gateway imageJ, final Class<C> commandClass) {
        // SETUP
        final UserInterface oldUI = imageJ.ui().getDefaultUI();
        final UserInterface mockUI = mockUIDialogPrompt(imageJ);
        final ImagePlus image = mock(ImagePlus.class);
        when(image.getNSlices()).thenReturn(1);

        try {
            // EXECUTE
            final CommandModule module = imageJ.command().run(commandClass, true,
                    "inputImage", image).get();

            // VERIFY
            assertTrue("2D image should have cancelled the plugin", module
                    .isCanceled());
            assertEquals("Cancel reason is incorrect", CommonMessages.NOT_3D_IMAGE,
                    module.getCancelReason());
            verify(mockUI, timeout(1000)).dialogPrompt(anyString(), anyString(), any(),
                    any());
        } catch (InterruptedException | ExecutionException e) {
            Assert.fail("Test timed out");
        } finally {
            imageJ.ui().setDefaultUI(oldUI);
        }
    }

and (line number is weird though) -> at org.scijava.ui.DefaultUIService.getVisibleUI(DefaultUIService.java:550)

    public List<UserInterface> getVisibleUIs() {
        final ArrayList<UserInterface> uis = new ArrayList<>();
        for (final UserInterface ui : uiList()) {
            if (ui.isVisible()) uis.add(ui);
        }
        return uis;
    }

[mdoube]

Looks like getVisibleUI() as per the error doesn't exist any more and is now getVisibleUIs() in the code. Digging into versions and build clashes.

[mdoube]

This bug was introduced by pom-scijava between 35.1.1 and 36.0.0. 35.1.1 is the last version that builds and tests OK.

[mdoube]

Maven test reports for pom-scijava 36.0.0. surefire-reports.zip

[ctrueden]

I just released pom-scijava 37.0.0 which does not fix this Mockito problem. But I wanted to mention that I haven't forgotten about this issue, and will try to investigate soon, and push another pom-scijava release fixing it. Thanks for the report and investigation so far, @mdoube.

[mdoube]

Thanks @ctrueden. I was thinking maybe we could try again to run these Mockito tests in the routine CI build via Github Actions. Richard pulled the slow tests out of the standard Maven build (so they have to be called specially with mvn test -P allTests) because they tended to cause time-outs, but that was back in the Travis or maybe Jenkins days. Would that have helped detect this breakage earlier?

[ctrueden]

Would that have helped detect this breakage earlier?

I think so... BoneJ2 is included in the melting pot build that runs every time pom-scijava is changed, which calls mvn clean install with updated dependency versions and such on each component being managed. So anything that runs as part of BoneJ2's standard unit test execution should get checked during this process.

[mdoube]

I made a PR https://github.com/bonej-org/BoneJ2/pull/342 to see whether I could tweak the test configuration to make the wrapper tests run. They do, but I still get the same error even on older working versions of SciJava e.g. 34.1.0, when running as an automated build test on GitHub. Locally mvn clean package completes fine.

[mdoube]

Just updated pom-scijava with the last Java 8 compatible Mockito so that wrapper tests run at all - they otherwise fail to build with an exception, on a new Eclipse installation. https://github.com/scijava/pom-scijava/pull/265

[ctrueden]

@mdoube IIUC: with Mockito 4.11.0, the tests compile and run, but... they still fail, right? Due to the UIService API change to getVisibleUI that you describe above? To fix this, could we: A) update the code in this repository to use the newer SciJava API; or better: B) fix SciJava Common to restore the previous method signature that BoneJ2 needs here? It was not intentional to break backwards API compatibility in SJC, and I'm happy to do (B) if it's feasible. What do you think?

[mdoube]

@mdoube IIUC: with Mockito 4.11.0, the tests compile and run, but... they still fail, right?

Yes, I just checked with 38.0.0-SNAPSHOT, which has Mockito 4.11.0 and the behaviour is the same.

Due to the UIService API change to getVisibleUI that you describe above?

I think so.

To fix this, could we: A) update the code in this repository to use the newer SciJava API; or better: B) fix SciJava Common to restore the previous method signature that BoneJ2 needs here? It was not intentional to break backwards API compatibility in SJC, and I'm happy to do (B) if it's feasible. What do you think?

Looks like there is now 1 failing test (locally):

-------------------------------------------------------------------------------
Test set: org.bonej.wrapperPlugins.IntertrabecularAngleWrapperTest
-------------------------------------------------------------------------------
Tests run: 14, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 3.056 s <<< FAILURE! -- in org.bonej.wrapperPlugins.IntertrabecularAngleWrapperTest
org.bonej.wrapperPlugins.IntertrabecularAngleWrapperTest.testTimeDimensionCancelsPlugin -- Time elapsed: 1.029 s <<< FAILURE!
org.mockito.exceptions.verification.TooManyActualInvocations: 

userInterface.dialogPrompt(
    <any string>,
    <any string>,
    <any>,
    <any>
);
Wanted 1 time:
-> at org.bonej.wrapperPlugins.IntertrabecularAngleWrapperTest.testTimeDimensionCancelsPlugin(IntertrabecularAngleWrapperTest.java:405)
But was 2 times:
-> at org.scijava.ui.DefaultUIService.showDialog(DefaultUIService.java:308)
-> at org.scijava.ui.DefaultUIService.showDialog(DefaultUIService.java:308)

    at org.bonej.wrapperPlugins.IntertrabecularAngleWrapperTest.testTimeDimensionCancelsPlugin(IntertrabecularAngleWrapperTest.java:405)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
    at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214)
    at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
    at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
    at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
    at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)

Plus lots of failing tests running under GitHub Actions, like this one, at https://github.com/bonej-org/BoneJ2/actions/runs/8613147702/job/23603867749?pr=342

  Error:  org.bonej.wrapperPlugins.IntertrabecularAngleWrapperTest.testNullImageCancelsPlugin -- Time elapsed: 1.021 s <<< FAILURE!
  Wanted but not invoked:
  userInterface.dialogPrompt(
      <any string>,
      <any string>,
      <any>,
      <any>
  );
  -> at org.bonej.wrapperPlugins.CommonWrapperTests.testNullImageCancelsPlugin(CommonWrapperTests.java:93)
  Actually, there were zero interactions with this mock.

    at org.bonej.wrapperPlugins.CommonWrapperTests.testNullImageCancelsPlugin(CommonWrapperTests.java:93)
    at org.bonej.wrapperPlugins.IntertrabecularAngleWrapperTest.testNullImageCancelsPlugin(IntertrabecularAngleWrapperTest.java:348)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
    at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214)
    at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
    at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
    at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
    at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)

[mdoube]

@ctrueden there is only one offending test locally:

https://github.com/bonej-org/BoneJ2/blob/10a615e0532bb04895a50e5f886a92d311079c3b/Modern/wrapperPlugins/src/test/java/org/bonej/wrapperPlugins/IntertrabecularAngleWrapperTest.java#L405-L406

Could you please advise how to make it run properly using the new API?