[GitHub] [commons-imaging] kinow commented on a diff in pull request #271: Unit test : make tests in memory instead of writing hundred of tmp files

["kinow (via GitHub)" <gi...@apache.org>]

kinow commented on code in PR #271:
URL: https://github.com/apache/commons-imaging/pull/271#discussion_r1097740314


##########
src/test/java/org/apache/commons/imaging/formats/bmp/BmpRoundtripTest.java:
##########
@@ -140,17 +139,21 @@ private void writeAndReadImageData(final int[][] rawData) throws IOException, Im
 
         final byte[] bytes = Imaging.writeImageToBytes(srcImage, ImageFormats.BMP);
 
-        final File tempFile = Files.createTempFile("temp", ".bmp").toFile();
-        FileUtils.writeByteArrayToFile(tempFile, bytes);
-
         final BufferedImage dstImage = Imaging.getBufferedImage(bytes);
 
-        assertNotNull(dstImage);
-        assertEquals(srcImage.getWidth(), dstImage.getWidth());
-        assertEquals(srcImage.getHeight(), dstImage.getHeight());
-
-        final int[][] dstData = bufferedImageToImageData(dstImage);
-        compare(rawData, dstData);
+        try {
+            assertNotNull(dstImage);
+            assertEquals(srcImage.getWidth(), dstImage.getWidth());
+            assertEquals(srcImage.getHeight(), dstImage.getHeight());
+
+            final int[][] dstData = bufferedImageToImageData(dstImage);
+            compare(rawData, dstData);
+        } catch (final Throwable e) {
+            final Path tempFile = Files.createTempFile("temp", ".bmp");
+            Files.write(tempFile, bytes);
+            System.err.println("Failed tempFile " + tempFile);

Review Comment:
   We migrated old Syserr & Sysout calls to JUL `Logger`. See `BitImageParser` for an example use. We can use the same in the tests, so users are able to ignore this output if they want. (The examples use Syserr/Sysout as that can be helpful for users trying in their IDE's).



##########
src/test/java/org/apache/commons/imaging/roundtrip/RoundtripBase.java:
##########
@@ -39,35 +38,52 @@ public class RoundtripBase {
     protected void roundtrip(final FormatInfo formatInfo, final BufferedImage testImage,
                              final String tempPrefix, final boolean imageExact) throws IOException,
             ImageReadException, ImageWriteException {
-        final File temp1 = Files.createTempFile(tempPrefix + ".", "."
-                + formatInfo.format.getDefaultExtension()).toFile();
-        Debug.debug("tempFile: " + temp1.getName());
 
         final ImageParser imageParser = Util.getImageParser(formatInfo.format);
 
+        byte[] temp1;
         final ImagingParameters params = Util.getImageParser(formatInfo.format).getDefaultParameters();
-        try (FileOutputStream fos = new FileOutputStream(temp1)) {
-            imageParser.writeImage(testImage, fos, params);
+        try (ByteArrayOutputStream bos = new ByteArrayOutputStream(1000000)) {

Review Comment:
   Maybe leave it the default value and let it handle expanding the internal buffer? That could be helpful if we load many smaller images, I think (although I have no idea the distribution of sizes of our images & their metadata too).



##########
src/test/java/org/apache/commons/imaging/formats/bmp/BmpRoundtripTest.java:
##########
@@ -140,17 +139,21 @@ private void writeAndReadImageData(final int[][] rawData) throws IOException, Im
 
         final byte[] bytes = Imaging.writeImageToBytes(srcImage, ImageFormats.BMP);
 
-        final File tempFile = Files.createTempFile("temp", ".bmp").toFile();
-        FileUtils.writeByteArrayToFile(tempFile, bytes);
-
         final BufferedImage dstImage = Imaging.getBufferedImage(bytes);
 
-        assertNotNull(dstImage);
-        assertEquals(srcImage.getWidth(), dstImage.getWidth());
-        assertEquals(srcImage.getHeight(), dstImage.getHeight());
-
-        final int[][] dstData = bufferedImageToImageData(dstImage);
-        compare(rawData, dstData);
+        try {
+            assertNotNull(dstImage);
+            assertEquals(srcImage.getWidth(), dstImage.getWidth());
+            assertEquals(srcImage.getHeight(), dstImage.getHeight());
+
+            final int[][] dstData = bufferedImageToImageData(dstImage);
+            compare(rawData, dstData);
+        } catch (final Throwable e) {
+            final Path tempFile = Files.createTempFile("temp", ".bmp");
+            Files.write(tempFile, bytes);
+            System.err.println("Failed tempFile " + tempFile);

Review Comment:
   Also, reading again the code, I think we can rethink this fallback mechanism.
   
   Right now, in case there's any exception of any type, it writes the image to the disk. But that might end up doing that unnecessarily for some exception types.
   
   It might be simple to just throw the error and let a developer call the code to write it to the disk, I think. Otherwise we will also have to remember to use this boilerplace code whenever we write new test code that loads images into the memory.
   
   Sorry @kpouer, as I believe your initial PR didn't have this, and you added it based on our initial feedback. Right now I'm included to undo that, but give me some time to think about this one :+1: 
   



-- 
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: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org