Dependency updates #274

mdoube · 2020-10-30T07:27:11Z

Update pom.xml to use scijava 29.2.1 and handle downstream effects of that change.

Hack around some downstream effects: - change in API for formatService.getFormat - Tests fail due to malformed calls These commented-out tests may need reworking in the future. Remove explicit versions of things included/referred to in scijava >= 29

These seem to be breaking due to API changes in scijava 29

Use whatever version is being declared upstream by scijava

Sometimes these tests pass and sometimes they fail, but no-one knows why the result is variable.

alessandrofelder

I am worried about commenting out tests.
~~I assume the reasoning behind this is that they have stopped passing?~~ just seen the commit message.
I will try and figure out what the problem is this week.

mdoube · 2020-11-03T09:02:16Z

I am worried about commenting out tests.

Me too. These tests seem like obscure corner cases rather than main function (e.g. if file is not an image), so I'm not too worried.

I will try and figure out what the problem is this week.

OK but balance the cost of your time against the risk that these tests are meant to be averting. The Fractal Dimension one is a known unknown - a kind of Heisentest. Sometimes passes, sometimes doesn't. The Analyse Skeleton one seems obscure, like a file format test which should be handled way upstream from the wrapper plugin, and could possibly be discarded for this reason. It's not obvious to me what it's actually testing, and whether what it is testing is relevant to this level of plugin. Same for the empty image is not binary test, because it's impossible to generate an image with 0 pixels as far as I can tell.

testSphereRidge() seems important, that one may be worth fixing.

alessandrofelder · 2020-11-03T10:57:09Z

Starting on the FindRidgePoints test - I am getting the tests to pass but mvn showing

[ERROR] Cannot create plugin: class='io.scif.convert.FileToDatasetConverter', priority=1.0, enabled=true, pluginType=Converter

Is this what you are seeing too, @mdoube ?

mdoube · 2020-11-03T12:12:13Z

Is this what you are seeing too, @mdoube ?

Yes exactly. Test passes when run directly in Eclipse, but throws this to the console:

[ERROR] Cannot create plugin: class='io.scif.convert.FileToDatasetConverter', priority=1.0, enabled=true, pluginType=Converter

@imagejan wrote on Gitter:

this is a known issue, and while it displays as an error (as the plugin can’t be created), it doesn’t affect your tests. The cause is a missing DatasetIOService at test time.
See the second paragraph here: scifio/scifio#443 (comment)

Uncommented in 47c91f9

mdoube · 2020-11-03T12:35:01Z

ElementUtilTest.testIsBinaryReturnsFalseIfIntervalEmpty() fails with:

net.imglib2.exception.InvalidDimensionsException: Expected only positive dimensions but got: [0]
	at net.imglib2.Dimensions.verifyAllPositive(Dimensions.java:166)
	at net.imglib2.Dimensions.verify(Dimensions.java:208)
	at net.imglib2.img.array.ArrayImgFactory.create(ArrayImgFactory.java:90)
	at net.imglib2.img.array.ArrayImgFactory.create(ArrayImgFactory.java:69)
	at net.imglib2.img.array.ArrayImgs.doubles(ArrayImgs.java:555)
	at org.bonej.utilities.ElementUtilTest.testIsBinaryReturnsFalseIfIntervalEmpty(ElementUtilTest.java:234)

Which suggests to me that this condition (an image with zero size) is now checked for and prevented earlier in execution.
Test deleted in 96b2e93

mdoube · 2020-11-03T13:28:48Z

AnalyseSkeletonWrapperTest.testBadFormatIntensityImageCancelsPlugin() fails with:

org.junit.ComparisonFailure: Cancel reason is incorrect expected:<[Image format is not recognized]> but was:<[An error occurred while opening the image]>
	at org.junit.Assert.assertEquals(Assert.java:125)
	at org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest.testBadFormatIntensityImageCancelsPlugin(AnalyseSkeletonWrapperTest.java:170)

It's not clear to me what error condition the test is actually checking for (and for that reason is a candidate for deletion):

	@Test
	public void testBadFormatIntensityImageCancelsPlugin() throws Exception {
		// SETUP
		final ImagePlus imagePlus = IJ.createImage("test", 3, 3, 3, 8);
		final File exceptionFile = mock(File.class);
		when(exceptionFile.getAbsolutePath()).thenReturn("file.foo");
		when(MOCK_UI.chooseFile(any(File.class), anyString())).thenReturn(
			exceptionFile);

		// EXECUTE
		final CommandModule module = command().run(
			AnalyseSkeletonWrapper.class, true, "inputImage", imagePlus,
			"pruneCycleMethod", "Lowest intensity voxel").get();

		// VERIFY
		assertTrue("Plugin should have cancelled", module.isCanceled());
		assertEquals("Cancel reason is incorrect", "Image format is not recognized",
			module.getCancelReason());
	}

Additionally, this test class (but not this particular test method) complains many times, which appears to be the same problem as scifio/scifio#443 (comment):

[ERROR] Cannot create plugin: class='org.scijava.io.http.HTTPHandle', priority=0.0, enabled=true, pluginType=DataHandle

Zero dimension images are prevented from being created upstream, so this test has no effect.

An error was being thrown that does not affect the test run or its outcome. See scifio/scifio#443

alessandrofelder · 2020-11-05T21:56:58Z

I think that AnalyseSkeletonWrapperTest.testBadFormatIntensityImageCancelsPlugin() checks that the AnalyseSkeletonWrapper throws a FormatException when the input file name extension is .foo. This is to make sure that the user gets a different, more nuanced error message when they pass an image with a dodgy file name extension than in cases where something else goes wrong when opening the image. So for that reason, I think having the test would be useful.

The underlying cause of this not passing anymore is the DefaultFormatService's getFormat() method now returns null instead of throwing a FormatException, as far as I can tell.

I hope to dig deeper tomorrow.

mdoube · 2020-11-06T00:24:24Z

The underlying cause of this not passing anymore is the DefaultFormatService's getFormat() method now returns null instead of throwing a FormatException, as far as I can tell.

Right, so the null then triggers somewhere a NullPointerException:

Addinge.toString() to AnalyseSkeletonWrapper.openIntensityImage()

		catch (final IOException | NullPointerException e) {
			cancelMacroSafe(this, "An error occurred while opening the image: "+e.toString());
                        e.printStackTrace();
			logService.trace(e);
		}

Is logged like this by the test:

An error occurred while opening the image: java.io.FileNotFoundException: file.foo (No such file or directory)

java.io.FileNotFoundException: file.foo (No such file or directory)
	at java.io.FileInputStream.open0(Native Method)
	at java.io.FileInputStream.open(FileInputStream.java:195)
	at java.io.FileInputStream.<init>(FileInputStream.java:138)
	at org.scijava.text.DefaultTextService.open(DefaultTextService.java:63)
	at org.scijava.text.DefaultTextService.asHTML(DefaultTextService.java:75)
	at org.scijava.text.io.TextIOPlugin.open(TextIOPlugin.java:70)
	at org.scijava.text.io.TextIOPlugin.open(TextIOPlugin.java:48)
	at org.scijava.io.DefaultIOService.open(DefaultIOService.java:69)
	at org.bonej.wrapperPlugins.AnalyseSkeletonWrapper.openIntensityImage(AnalyseSkeletonWrapper.java:300)
	at org.bonej.wrapperPlugins.AnalyseSkeletonWrapper.run(AnalyseSkeletonWrapper.java:198)
	at org.scijava.command.CommandModule.run(CommandModule.java:196)
	at org.scijava.module.ModuleRunner.run(ModuleRunner.java:165)
	at org.scijava.module.ModuleRunner.call(ModuleRunner.java:124)
	at org.scijava.module.ModuleRunner.call(ModuleRunner.java:63)
	at org.scijava.thread.DefaultThreadService.lambda$wrap$2(DefaultThreadService.java:225)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

alessandrofelder · 2020-11-06T19:32:49Z

Exactly.
The NPE is thrown because the File.foo doesn't actually exist (it's just being mocked by the test to check that the FormatException is thrown). So the code thinks the format is fine (no format exception) and then tries to open the file?

This set of changes to the DefaultFormatService is the key.

This is probably my lack of understanding what an empty FormatList means here, but possibly worth double-checking: @ctrueden @frauzufall, are you happy with returning null when the FormatList is empty, or should that trigger a FormatException too?
(I am basing this on the fact that in the FormatService javadoc, it says that getFormat will throw a FormatException
"if no compatible format was found, or if something goes wrong checking the source".
)
Thanks a lot for your help!

alessandrofelder · 2020-11-12T21:06:25Z

I suggest we remove the negative test from AnalyzeSkeletonWrapper for now, and I've opened an issue so we don't forget to reinstate it once we figure out how to do it. See #275. At least this way, we don't get held up.

* Update Eclipse Collections to 10.4.0 * Update scijava to 29.2.1 Hack around some downstream effects: - change in API for formatService.getFormat - Tests fail due to malformed calls These commented-out tests may need reworking in the future. Remove explicit versions of things included/referred to in scijava >= 29 * Remove com.miglayout from and LocalThickness version from pom.xml * Comment out some failing tests for later investigation These seem to be breaking due to API changes in scijava 29 * Remove the Eclipse Collections version declaration Use whatever version is being declared upstream by scijava * Reinstate fractal dimension wrapper tests. See issue #229 Sometimes these tests pass and sometimes they fail, but no-one knows why the result is variable. * Remove test that checks for an image with a single zero dimension Zero dimension images are prevented from being created upstream, so this test has no effect. * Restore commented-out test An error was being thrown that does not affect the test run or its outcome. See scifio/scifio#443 * remove this test for now Co-authored-by: alessandrofelder <[email protected]>

mdoube added 6 commits October 29, 2020 15:51

Update Eclipse Collections to 10.4.0

159ea73

Update scijava to 29.2.1

08817e3

Hack around some downstream effects: - change in API for formatService.getFormat - Tests fail due to malformed calls These commented-out tests may need reworking in the future. Remove explicit versions of things included/referred to in scijava >= 29

Remove com.miglayout from and LocalThickness version from pom.xml

21daf0a

Comment out some failing tests for later investigation

92aeea5

These seem to be breaking due to API changes in scijava 29

Remove the Eclipse Collections version declaration

a857378

Use whatever version is being declared upstream by scijava

Reinstate fractal dimension wrapper tests. See issue #229

4bc1bd5

Sometimes these tests pass and sometimes they fail, but no-one knows why the result is variable.

mdoube requested review from alessandrofelder and ctrueden October 30, 2020 07:27

alessandrofelder reviewed Nov 3, 2020

View reviewed changes

mdoube added 2 commits November 4, 2020 18:44

Remove test that checks for an image with a single zero dimension

96b2e93

Zero dimension images are prevented from being created upstream, so this test has no effect.

Restore commented-out test

47c91f9

An error was being thrown that does not affect the test run or its outcome. See scifio/scifio#443

remove this test for now

d4458c2

alessandrofelder self-requested a review November 12, 2020 21:06

alessandrofelder approved these changes Nov 12, 2020

View reviewed changes

mdoube merged commit fd1c3fd into master Nov 13, 2020

mdoube deleted the dependency-updates branch November 13, 2020 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependency updates #274

Dependency updates #274

mdoube commented Oct 30, 2020

alessandrofelder left a comment •

edited

Loading

mdoube commented Nov 3, 2020 •

edited

Loading

alessandrofelder commented Nov 3, 2020

mdoube commented Nov 3, 2020 •

edited

Loading

mdoube commented Nov 3, 2020 •

edited

Loading

mdoube commented Nov 3, 2020 •

edited

Loading

alessandrofelder commented Nov 5, 2020

mdoube commented Nov 6, 2020 •

edited

Loading

alessandrofelder commented Nov 6, 2020

alessandrofelder commented Nov 12, 2020

Dependency updates #274

Dependency updates #274

Conversation

mdoube commented Oct 30, 2020

alessandrofelder left a comment • edited Loading

Choose a reason for hiding this comment

mdoube commented Nov 3, 2020 • edited Loading

alessandrofelder commented Nov 3, 2020

mdoube commented Nov 3, 2020 • edited Loading

mdoube commented Nov 3, 2020 • edited Loading

mdoube commented Nov 3, 2020 • edited Loading

alessandrofelder commented Nov 5, 2020

mdoube commented Nov 6, 2020 • edited Loading

alessandrofelder commented Nov 6, 2020

alessandrofelder commented Nov 12, 2020

alessandrofelder left a comment •

edited

Loading

mdoube commented Nov 3, 2020 •

edited

Loading

mdoube commented Nov 3, 2020 •

edited

Loading

mdoube commented Nov 3, 2020 •

edited

Loading

mdoube commented Nov 3, 2020 •

edited

Loading

mdoube commented Nov 6, 2020 •

edited

Loading