kinow commented on code in PR #271:
URL: https://github.com/apache/commons-imaging/pull/271#discussion_r1114731934
##########
src/test/java/org/apache/commons/imaging/roundtrip/NullParametersRoundtripTest.java:
##########
@@ -38,13 +38,17 @@ public static Stream<FormatInfo> data() {
@MethodSource("data")
public void testNullParametersRoundtrip(final FormatInfo formatInfo)
throws Exception {
final BufferedImage testImage = TestImages.createFullColorImage(1, 1);
- final File temp1 = Files.createTempFile("nullParameters.", "." +
formatInfo.format.getDefaultExtension()).toFile();
- Imaging.writeImage(testImage, temp1, formatInfo.format);
- Imaging.getImageInfo(temp1);
- Imaging.getImageSize(temp1);
- Imaging.getMetadata(temp1);
- Imaging.getICCProfile(temp1);
- final BufferedImage imageRead = Imaging.getBufferedImage(temp1);
+ final byte[] temp1;
+ try (ByteArrayOutputStream byteArrayOutputStream = new
ByteArrayOutputStream()) {
+ Imaging.writeImage(testImage, byteArrayOutputStream,
formatInfo.format);
+ temp1 = byteArrayOutputStream.toByteArray();
+ }
+ final String filename = "nullParameters." +
formatInfo.format.getDefaultExtension();
+ Imaging.getImageInfo(new ByteArrayInputStream(temp1), filename);
+ Imaging.getImageSize(new ByteArrayInputStream(temp1), filename);
+ Imaging.getMetadata(new ByteArrayInputStream(temp1), filename);
+ Imaging.getICCProfile(new ByteArrayInputStream(temp1), filename);
Review Comment:
Hmmm, we cannot resuse the same `ByteArrayInputStream` right? :thinking:
would be nicer if we closed these streams at least. Maybe we have to split it
into separated test methods?
##########
src/test/java/org/apache/commons/imaging/formats/jpeg/exif/ExifRewriteTest.java:
##########
@@ -123,15 +119,12 @@ public void testInsert() throws Exception {
final ByteArrayOutputStream baos = new ByteArrayOutputStream();
new ExifRewriter().removeExifMetadata(byteSource, baos);
final byte[] bytes = baos.toByteArray();
- final File tempFile = Files.createTempFile("removed",
".jpg").toFile();
- Debug.debug("tempFile", tempFile);
- FileUtils.writeByteArrayToFile(tempFile, bytes);
Debug.debug("Output Segments:");
stripped = new ByteSourceArray(bytes);
new JpegUtils().dumpJFIF(stripped);
- assertFalse(hasExifData(tempFile));
+ assertFalse(hasExifData("removed.jpg", bytes));
Review Comment:
Really awkward to have the file name in the `ByteSource` constructor. I
think the original devs never imagined using it without a file
:slightly_smiling_face: Thank you for keeping the file names :+1:
##########
src/test/java/org/apache/commons/imaging/roundtrip/ImageAsserts.java:
##########
@@ -99,4 +99,18 @@ static void assertEquals(final File a, final File b) throws
IOException {
Assertions.assertEquals(aByte, bByte);
}
}
+
+ static void assertEquals(byte[] aData, byte[] bData) {
+ for (int i = 0; i < aData.length; i++) {
+ final int aByte = 0xff & aData[i];
+ final int bByte = 0xff & bData[i];
+
+ if (aByte != bByte) {
+ Debug.debug("i: " + i);
+ Debug.debug("aByte: " + aByte + " (0x" +
Integer.toHexString(aByte) + ')');
+ Debug.debug("bByte: " + bByte + " (0x" +
Integer.toHexString(bByte) + ')');
+ }
+ Assertions.assertEquals(aByte, bByte);
+ }
+ }
Review Comment:
Is there a reason for using this over using JUnit's
[`assertArrayEquals`](https://junit.org/junit5/docs/5.0.0/api/org/junit/jupiter/api/Assertions.html#assertArrayEquals-byte:A-byte:A-)
? The `Debug` is not active by default, I believe, so using this doesn't seem
to add must value.
##########
src/test/java/org/apache/commons/imaging/formats/jpeg/iptc/IptcUpdateTest.java:
##########
@@ -21,19 +21,15 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
-import java.io.BufferedOutputStream;
-import java.io.File;
-import java.io.FileOutputStream;
-import java.io.IOException;
-import java.io.OutputStream;
-import java.nio.file.Files;
+import java.io.*;
Review Comment:
I think the rest of the code in general is not using `.*` for imports, so
better keep that way for consistency (on this file, but also on the rest of the
files changed in the PR) :+1:
If your IDE did that automatically, try disabling that inspection/auto-save
behavior for this project (I know other Commons components also use complete
imports).
##########
src/test/java/org/apache/commons/imaging/roundtrip/FormatInfo.java:
##########
@@ -130,4 +130,16 @@ class FormatInfo {
this.identicalSecondWrite = identicalSecondWrite;
this.preservesResolution = preservesResolution;
}
+
+ @Override
+ public String toString() {
+ return "FormatInfo{" +
+ "format=" + format +
+ ", canRead=" + canRead +
+ ", canWrite=" + canWrite +
+ ", colorSupport=" + colorSupport +
+ ", identicalSecondWrite=" + identicalSecondWrite +
+ ", preservesResolution=" + preservesResolution +
+ '}';
Review Comment:
Looks harmless, but was there a reason for adding this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
