Fix an instability in Geotiff unit test#44
Conversation
The Geotiff writing test was failing depending on user build environment; The cause is an assertion that expects that a written image has an exact size, despite it being compressed using deflate algorithm (not deterministic). The fix consists in changing the comparison logic. We now read back the written file, and verify that the returned coverage is equivalent to the coverage that has been used as test input.
… retile given image.
| * | ||
| * @author Martin Desruisseaux (IRD, Geomatys) | ||
| * @author Alexis Manin (Geomatys) | ||
| * @version 1.5 |
There was a problem hiding this comment.
Need to be changed to 1.7.
| * @return {@code true} if the given object is equal to this sample dimension. | ||
| */ | ||
| @Override | ||
| public boolean equals(final Object object) { |
There was a problem hiding this comment.
The pattern in other classes is to rewrite this method as a final method which redirects to equals(object, ComparisonMode.STRIC). The current implementation would move to the latter method. The intent is to have only one method to override in subclasses (if any), for making sure that users do not override equals(Object) and forget to override equals(Object, ComparisonMode).
| if (object == this) { | ||
| return true; | ||
| } | ||
| if (object instanceof SampleDimension) { |
There was a problem hiding this comment.
Not an issue of this pull request, but I just realized that this line should be if (object.getClass() == getClass()) (for ComparisonMode.EXACT only).
| if (other == this) return true; | ||
| if (mode.isApproximate() && other instanceof SampleDimension) { | ||
| final var otherDim = (SampleDimension) other; | ||
| return Utilities.deepEquals(this.transferFunction, otherDim.transferFunction, mode) |
There was a problem hiding this comment.
The transfer function was not compared in equals(Object) because it was derived from categories. Therefore, comparing transferFunction was considered redundant with comparing categories. If we want the comparison to be tolerant to slight difference in transfer functions, then we need to make Category implements LenientComparable too, otherwise this method will actually behave as if a strict comparison was performed regardless the flexibility of Utilities.deepEquals(this.transferFunction, otherDim.transferFunction, mode).
| @Override | ||
| public boolean equals(Object other, ComparisonMode mode) { | ||
| if (other == this) return true; | ||
| if (mode.isApproximate() && other instanceof SampleDimension) { |
There was a problem hiding this comment.
We should only test if the mode is STRICT, in which case the test should be done as in the current implementation of equals(Object), and unconditionally uses Utilities.deepEquals in all other cases. The reason is that since we need to make Category implements LenientComparable too, Category could apply whether policy it wants. In particular, it may compare the category name in ComparisonMode.BY_CONTRACT and ignore that name in ComparisonMode.IGNORE_METADATA.
| /** | ||
| * Verify that given image tiling respects user tiling request, modulo a given restriction. | ||
| * The restriction maps Tiff standard requirement for tile size to be multiple of a given factor. | ||
| * </br> |
There was a problem hiding this comment.
It is a small detail, but the rest of SIS rather uses <p>...</p> instead of <br/> (the slash also appears to be in the wrong side in this line). This is for semantic reason (the same reason why we use <i>, <em>, <var> or <dfn> in different contexts, even if they all appear in italic in the browser): the semantic is that we want to start a new paragraph.
2 participants
I stumbled upon an error while building SIS with a specific JDK: 23.0.2 Liberica (installed using sdkman -> sdk install java 23.0.2.fx-librca).
The test was failing because it tested the size of a written Geotiff file. The problem is, as the written file uses Deflate compression, there's no guarantee that the produced file will always have the same size, depending on user build environment. The stack trace of the error is :
This PR refactors the test to stop comparing file size. Instead, we read back the written file, and check that the decoded coverage is equivalent to the original coverage that was written.
A few side notes about this PR: