imagej / imagej-legacy

ImageJ+ImageJ2 compatibility layer
https://imagej.net/libs/imagej-legacy
BSD 2-Clause "Simplified" License
16 stars 25 forks source link

Fix assertImageEquals to actually assert #278

Closed imagejan closed 2 years ago

imagejan commented 2 years ago

When using pairedForEach directly, assertImageEquals(RAI, RAI) would never assert anything. We need to call the three-argument signature assertImageEquals(RAI, RAI, BiPredicate) that adds the assertion.

I discovered this when using AssertImgs from my project:

        <dependency>
            <groupId>net.imagej</groupId>
            <artifactId>imagej-legacy</artifactId>
            <classifier>tests</classifier>
            <version>${imagej-legacy.version}</version>
            <scope>test</scope>
        </dependency>

... when I wanted to assert that two different images are actually failing the test, which was not the case. Using assertRealTypeImageEquals is my workaround for now.

ctrueden commented 2 years ago

Oof, good catch, @imagejan.

ctrueden commented 2 years ago

I tried to release a new imagej-legacy but ran up against a new issue #279 which should be solved before doing so.

imagejan commented 2 years ago

Thanks @ctrueden! I wondered if there's an easy way right now to actually test if two images are not equal. I thought of the @Test (expected=...SomeException) annotation, but this only works for Exceptions but not AssertionErrors, right?

Would it be useful to have something like an assertImagesNotEqual() method here as well?

ctrueden commented 2 years ago

Would it be useful to have something like an assertImagesNotEqual() method here as well?

If the images are deterministic, you could simply check the value of a known sample coordinate that was supposed to change, no? If there is randomness, you could use a fixed random seed to make them deterministic. If that is somehow impossible, then I suppose an assertImagesNotEqual() would be useful.

maarzt commented 2 years ago

It is probably best to use the assetImageEquals method in imglib2: https://github.com/imglib/imglib2/blob/33c46610ec5311bb5836beb6c54075e198f34983/src/main/java/net/imglib2/test/ImgLib2Assert.java#L67

imagejan commented 2 years ago

Thanks for pointing that out, @maarzt!

I suggest that AssertImgs here be deprecated, we add a note about using the ImgLib2 implementation, and we use ImgLib2Assert for the tests here as well. Do you agree, @ctrueden @maarzt ?

maarzt commented 2 years ago

That's fine with me. You could also be more radical and remove the AssertImgs class entirely. It will for sure only break some tests, which can be fixed easily as soon as noticed.

ctrueden commented 2 years ago

I took @maarzt's suggestion and deleted the AssertImgs class: 1fe59c62fc7657e7430b0eabeb3016f43851a71e. I also removed AssertImgs and RandomImgs from imglib2-ij: imglib/imglib2-ij@567e918526790e2cff670e74c6ad3694107b6e4c. I searched my entire local code stash, but found no other uses of AssertImgs.

@imagejan Is it OK for you to use ImgLib2Assert in your projects now?

imagejan commented 2 years ago

Thanks @ctrueden and @maarzt, that's perfect.