Fix and extend Geotiff IFD handling to allow all integer types #1341
Conversation
* Follows specs from pg. 15-16 of v6.0 TIFF: https://www.itu.int/itudoc/itu-t/com16/tiff-fx/docs/tiff6.pdf * Added roundtrip tests of the private Geotiff method via reflection * This does reveal an existing API flaw that will cause overflow of large unsigned integers
|
Was talking with a colleague about how I couldn't test private methods, which was needed to properly test out the fixes to read/writeIntValue, when they pointed me to Java reflection, which I never knew about. |
There was a problem hiding this comment.
Thanks for this PR @WeatherGod! I just had a comment about the use of reflection and some very minor style issues in the tests.
| ByteOrder byteOrder = ByteOrder.BIG_ENDIAN; | ||
| buffer.order(byteOrder); | ||
|
|
||
| Method writeMethod = |
There was a problem hiding this comment.
I personally don't like this reflection solution for testing private methods. First, in general, private methods should not be tested, though I understand that in practice some classes would need big refactors to make the public API unit testable. Second, I think it makes the test harder to read. And finally, if someone were to rename writeIntValue, the compiler would not catch an issue here, you would just get NoSuchMethodException.
Is there any reasonable way to test the public API of GeoTiff instead? I am not as familiar with the code, so I am not sure how feasible this is. If not, I propose to make the method writeIntValue package private so you can test it without reflection. Also not a perfect solution, but I think it is slightly nicer.
There was a problem hiding this comment.
Coming from python, the idea that one shouldn't test private methods is completely foreign to me. Unfortunately, with the way the GeoTiff class is designed, it would be difficult to not only construct the many geotiffs instances with the appropriate data types and ranges, but to then subsequently test that it was encoded correctly. The test would have a huge amount of unrelated setup that would make the test difficult to read and understand.
I am totally fine with changing it to be package private. We could even go a step further and make them static as they do not interact with any class members.
Co-authored-by: Tara Drwenski <[email protected]>
* This allows us to drop reflection in the TestIFDEntry tests * Simplifies the test setup and execution
Description of Changes
PR Checklist
until ready for review