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

Update scijava to 30.0.0 #283

Merged
merged 2 commits into from
Feb 10, 2021
Merged

Update scijava to 30.0.0 #283

merged 2 commits into from
Feb 10, 2021

Conversation

mdoube
Copy link
Member

@mdoube mdoube commented Feb 5, 2021

No description provided.

@mdoube
Copy link
Member Author

mdoube commented Feb 5, 2021

Addresses #282

Copy link
Collaborator

@rgaiacs rgaiacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because @rgaiacs is not on the BoneJ2 refactoring team that his approval is not sufficient for merging?
Somehow he should get write access (don't think I can add him as I am a member, not an owner of the BoneJ Org)?

I assume this has been tested locally with mvn test -P allTests, right? Just in case SciJava 30 breaks some API that we only use in the wrappers?

If so, happy with the changes too.

@alessandrofelder alessandrofelder self-requested a review February 5, 2021 13:17
@alessandrofelder alessandrofelder dismissed their stale review February 5, 2021 13:22

mvn clean test -P allTests fails some AnisotropyWrapper tests for me

@mdoube
Copy link
Member Author

mdoube commented Feb 5, 2021

I assume this has been tested locally with mvn test -P allTests, right? Just in case SciJava 30 breaks some API that we only use in the wrappers?

Assumuption is the mother of all eff-ups... need to fix these tests prior to merging. These failures are reminiscent of something we've dealt with before.

[ERROR] Tests run: 18, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0.978 s <<< FAILURE! - in org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest
[ERROR] testIntensityImageChannelsCancelPlugin(org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest)  Time elapsed: 0.043 s  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[The intensity image can't have a channel dimension]> but was:<[An error occurred while opening the image]>
	at org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest.testIntensityImageChannelsCancelPlugin(AnalyseSkeletonWrapperTest.java:188)

[ERROR] testIntensityImageFramesCancelPlugin(org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest)  Time elapsed: 0.024 s  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[The intensity image can't have a time dimension]> but was:<[An error occurred while opening the image]>
	at org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest.testIntensityImageFramesCancelPlugin(AnalyseSkeletonWrapperTest.java:206)

[ERROR] testIntensityImageMismatchingDimensionsCancelPlugin(org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest)  Time elapsed: 0.035 s  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[The intensity image should match the dimensionality of the input] image> but was:<[An error occurred while opening the] image>
	at org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest.testIntensityImageMismatchingDimensionsCancelPlugin(AnalyseSkeletonWrapperTest.java:226)

[ERROR] testNot8BitIntensityImageCancelsPlugin(org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest)  Time elapsed: 0.018 s  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[The intensity image needs to be 8-bit greyscal]e> but was:<[An error occurred while opening the imag]e>
	at org.bonej.wrapperPlugins.AnalyseSkeletonWrapperTest.testNot8BitIntensityImageCancelsPlugin(AnalyseSkeletonWrapperTest.java:293)

@rgaiacs rgaiacs mentioned this pull request Feb 8, 2021
@mdoube mdoube mentioned this pull request Feb 10, 2021
@mdoube
Copy link
Member Author

mdoube commented Feb 10, 2021

Failing tests in AnalyseSkeletonWrapperTests have one thing in common, which is:

// in some method above
{
    final String intensityPath ="8bit-signed&pixelType=int8&planarDims=2&lengths=5,5,3&axes=X,Y,Channel.fake";
    mockIntensityFileOpening(intensityPath);
}

private void mockIntensityFileOpening(final String path) {
	final File intensityFile = mock(File.class);
	when(intensityFile.getAbsolutePath()).thenReturn(path);
	when(MOCK_UI.chooseFile(any(File.class), anyString())).thenReturn(intensityFile);
}

So it seems like a problem with the mocking of the broken file.

The mocking appears to be broken in this version of scijava.
@mdoube
Copy link
Member Author

mdoube commented Feb 10, 2021

Also see #273

mdoube added a commit that referenced this pull request Apr 5, 2024
* Update scijava to 30.0.0

* Remove some tests that relied on mocking various file dimensions

The mocking appears to be broken in this version of scijava.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants