Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scijava 36.0.0 breaks Mockito Wrapper tests #341

Open
mdoube opened this issue Oct 6, 2023 · 12 comments
Open

Scijava 36.0.0 breaks Mockito Wrapper tests #341

mdoube opened this issue Oct 6, 2023 · 12 comments
Assignees

Comments

@mdoube
Copy link
Member

mdoube commented Oct 6, 2023

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
Copy link
Member Author

mdoube commented Oct 6, 2023

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
Copy link
Member Author

mdoube commented Oct 6, 2023

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
Copy link
Member Author

mdoube commented Oct 6, 2023

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
Copy link
Member Author

mdoube commented Oct 6, 2023

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

@ctrueden
Copy link
Member

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
Copy link
Member Author

mdoube commented Oct 12, 2023

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
Copy link
Member

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
Copy link
Member Author

mdoube commented Oct 13, 2023

I made a PR #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
Copy link
Member Author

mdoube commented Apr 8, 2024

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.
scijava/pom-scijava#265

@ctrueden
Copy link
Member

ctrueden commented Apr 8, 2024

@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
Copy link
Member Author

mdoube commented Apr 9, 2024

@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
Copy link
Member Author

mdoube commented Apr 9, 2024

@ctrueden there is only one offending test locally:

verify(MOCK_UI, timeout(1000)).dialogPrompt(anyString(), anyString(), any(),
any());

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants