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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
