ENH: JPEG IO test another damaged file

[dzenanz]

JPEG has different scenarios for damaged files.
This increases coverage.

This is a revival of #2978.

[jhlegarreta]

👍

[dzenanz]

@issakomi can you please restore the test file to data.kitware.com?

[issakomi]

The test will not work. The test expects "no exception", but this file does trigger exception. That was the reason i modified the test,.

[dzenanz]

Well, we can create another test, one which expects the exception like the PNG one (itkPNGImageIOTestCorrupt2).

[issakomi]

Well, we can create another test, one which expects the exception

Yes, test with "expect exception" will work with the file. My suggested simple test had logic "expect no crash", with exception or without. Current test is named itkJPEGImageIODegenerateCasesTest, but is doing just "expect no exception", no difference with a good file. There are different paths for corrupted jpegs (ITK exception, ITK warning, only internal warning from libjpeg).

[dzenanz]

The first force-push adds a test which expects exception, the second one just rebases on current master.

[issakomi]

itkJPEGImageIOBrokenCasesTest

[issakomi]

2021-12-22T22:47:36.1597598Z [3872/5342] Linking CXX executable bin/ITKIOJPEGTestDriver
2021-12-22T22:47:36.1599168Z FAILED: bin/ITKIOJPEGTestDriver 
2021-12-22T22:47:36.4465746Z : && /usr/bin/c++ -mtune=generic -march=corei7 -Wall -Wcast-align -Wdisabled-optimization -Wextra -Wformat=2 -Winvalid-pch -Wno-format-nonliteral -Wpointer-arith -Wshadow -Wunused -Wwrite-strings -funit-at-a-time -Wno-strict-overflow -Wno-deprecated -Wno-invalid-offsetof -Woverloaded-virtual -Wstrict-null-sentinel  -Os -DNDEBUG -fPIE -pie Modules/IO/JPEG/test/CMakeFiles/ITKIOJPEGTestDriver.dir/ITKIOJPEGTestDriver.cxx.o Modules/IO/JPEG/test/CMakeFiles/ITKIOJPEGTestDriver.dir/itkJPEGImageIOTest.cxx.o Modules/IO/JPEG/test/CMakeFiles/ITKIOJPEGTestDriver.dir/itkJPEGImageIOTest2.cxx.o Modules/IO/JPEG/test/CMakeFiles/ITKIOJPEGTestDriver.dir/itkJPEGImageIODegenerateCasesTest.cxx.o Modules/IO/JPEG/test/CMakeFiles/ITKIOJPEGTestDriver.dir/itkJPEGImageIOBrokenCasesTest.cxx.o -o bin/ITKIOJPEGTestDriver  lib/libITKIOJPEG-5.3.a  lib/libITKTestKernel-5.3.a  lib/libITKTestKernel-5.3.a  lib/libITKIOJPEG-5.3.a  lib/libITKIOBMP-5.3.a  lib/libITKIOGDCM-5.3.a  lib/libitkgdcmMSFF-5.3.a  lib/libitkgdcmDICT-5.3.a  lib/libitkgdcmIOD-5.3.a  lib/libitkgdcmDSED-5.3.a  lib/libitkgdcmCommon-5.3.a  lib/libitkgdcmjpeg8-5.3.a  lib/libitkgdcmjpeg12-5.3.a  lib/libitkgdcmjpeg16-5.3.a  lib/libitkgdcmopenjp2-5.3.a  lib/libitkgdcmcharls-5.3.a  lib/libitkgdcmuuid-5.3.a  lib/libITKIOGIPL-5.3.a  lib/libITKIOMeshBYU-5.3.a  lib/libITKIOMeshFreeSurfer-5.3.a  lib/libITKIOMeshGifti-5.3.a  lib/libITKgiftiio-5.3.a  lib/libITKEXPAT-5.3.a  lib/libITKIOMeshOBJ-5.3.a  lib/libITKIOMeshOFF-5.3.a  lib/libITKIOMeshVTK-5.3.a  lib/libITKIOMeshBase-5.3.a  lib/libITKQuadEdgeMesh-5.3.a  lib/libITKMesh-5.3.a  lib/libITKIOMeta-5.3.a  lib/libITKMetaIO-5.3.a  lib/libITKIONIFTI-5.3.a  lib/libITKniftiio-5.3.a  lib/libITKznz-5.3.a  -lm  lib/libITKTransform-5.3.a  lib/libITKIONRRD-5.3.a  lib/libITKNrrdIO-5.3.a  lib/libITKIOPNG-5.3.a  lib/libitkpng-5.3.a  lib/libITKIOTIFF-5.3.a  lib/libitktiff-5.3.a  lib/libitkjpeg-5.3.a  lib/libitkzlib-5.3.a  lib/libITKIOVTK-5.3.a  lib/libITKIOImageBase-5.3.a  lib/libITKCommon-5.3.a  lib/libitkdouble-conversion-5.3.a  lib/libitksys-5.3.a  lib/libITKVNLInstantiation-5.3.a  lib/libitkvnl_algo-5.3.a  lib/libitkvnl-5.3.a  lib/libitkv3p_netlib-5.3.a  lib/libitkvcl-5.3.a  -lm  -lpthread  -lm  -ldl && :
2021-12-22T22:47:36.4472208Z Modules/IO/JPEG/test/CMakeFiles/ITKIOJPEGTestDriver.dir/itkJPEGImageIOBrokenCasesTest.cxx.o: In function `itkJPEGImageIODegenerateCasesTest(int, char**)':
2021-12-22T22:47:36.4473761Z itkJPEGImageIOBrokenCasesTest.cxx:(.text+0xb0): multiple definition of `itkJPEGImageIODegenerateCasesTest(int, char**)'
2021-12-22T22:47:36.4475002Z Modules/IO/JPEG/test/CMakeFiles/ITKIOJPEGTestDriver.dir/itkJPEGImageIODegenerateCasesTest.cxx.o:itkJPEGImageIODegenerateCasesTest.cxx:(.text+0xb0): first defined here
2021-12-22T22:47:36.4476624Z Modules/IO/JPEG/test/CMakeFiles/ITKIOJPEGTestDriver.dir/ITKIOJPEGTestDriver.cxx.o:(.data.rel.ro+0x38): undefined reference to `itkJPEGImageIOBrokenCasesTest(int, char**)'
2021-12-22T22:47:36.4477672Z collect2: error: ld returned 1 exit status

[issakomi]

Still don't know what was wrong with my version of itkJPEGImageIODegenerateCasesTest. @jhlegarreta What exactly was not conforming with guidelines? IMHO, name itkJPEGImageIODegenerateCasesTest for the test doing simple "try expect no exception" is misleading, it should be something like itkJPEGImageIOReadTest1.

[jhlegarreta]

@jhlegarreta What exactly was not conforming with guidelines?

@issakomi I am very sorry if my request changes review was taken as dismissal of the work, and maybe I should have made more clear the reason.

So first of all I want to make it clear that the review was by no means intended as a dismissal. All the time you spend investigating this, improving the issues with the JPEG/PNG ImageIO's (as well as issues with the DICOM IO's, among others) are highly appreciated, and are invaluable to improve the health of the toolkit.

I only requested changes because having a consistent style across test files is necessary to improve readability, and ease reviews, among others. This includes having a consistent argument checking block, defining the dimension and pixel types, among others. I grant that the latter might be controversial when we only need these at a single place, but as said, it helps making tests more consistent, eases reading the tests, and also allows for an easier extension of the test if that is needed at some point.

I did not realize that the newly added image expected an exception. Using the ITK_TRY_EXPECT_EXCEPTION would allow to print the exception, while avoiding the need to explicitly write the try/catch block.

IMHO, name itkJPEGImageIODegenerateCasesTest for the test doing simple "try expect no exception" is misleading, it should be something like itkJPEGImageIOReadTest1.

I do agree that the test name I introduced in #2933 for the case that you tried to address here might have not been the most accurate, and I do agree with a comment of yours:

"expect no crash", with exception or without (...), "expect no exception",
There are different paths for corrupted jpegs (ITK exception, ITK warning, only internal warning from libjpeg).

I do agree that all this paths should be handled clearly, and naming should be helpful in this.

BTW, making a macro for the argument checking block boilerplate code, and testing different exceptions are known areas of improvement (#1273, #983). And I do grant that not all tests present in ITK have been written with a consistent style. We try to improve them as time permits.

Sorry for the lengthy comment. Hopefully this helps in casting light on why I requested changes.

[issakomi]

@jhlegarreta Thank you for the comment.

[issakomi]

The change from

  if (argc != 2)
  {
    std::cerr << "Missing parameters." << std::endl;
    std::cerr << "Usage: " << itkNameOfTestExecutableMacro(argv);
    std::cerr << " inputFilename" << std::endl;
    return EXIT_FAILURE;
  }

to

  if (argc != 2)
  {
    std::cerr << "Missing parameters.\nUsage: " << itkNameOfTestExecutableMacro(argv) << " inputFilename\n";
    return EXIT_FAILURE;
  }

was well-meaning. I remember the discussion that unnecessary std:cerr and flushes should be better avoided ( std::cerr can flush). But it is not worth to discuss, if the first variant is the requirement, it is OK. Is the 1st version required? I am asking because i have another WIP PR with a test.

[jhlegarreta]

was well-meaning. I remember the discussion that unnecessary std:cerr and flushes should be better avoided ( std::cerr can flush).

Could be an valid argument. As I said in #2536 (comment), if this change is deemed necessary, the corresponding section in the ITK SW Guide should be changed, and a sweeping change across the whole code base should be applied (a regex or replace all may partially work, so maybe not that hard, but I grant it requires some time).

But it is not worth to discuss, if the first variant is the requirement, it is OK. Is the 1st version required? I am asking because i have another WIP PR with a test.

Following the consistency argument, I would keep them as recommended in the SW Guide unless there is agreement on the need to change and someone feels like giving it a go at the sweeping change in another PR.

Thanks.

[dzenanz]

Still don't know what was wrong with my version of itkJPEGImageIODegenerateCasesTest.

You changed an existing test, which was expecting the file to be read without an exception, into a test which ignores whether there is an exception and always completes successfully. Thus it was not really testing anything besides increasing code coverage. My solution was to keep the existing test intact, and add an additional test which expects the reading to throw an exception. In copy-pasting the test I forgot to change the test name, which I will rectify soon.

[issakomi]

You changed an existing test, which was expecting the file to be read without an exception, into a test which ignores whether there is an exception and always completes successfully. Thus it was not really testing anything besides increasing code coverage.

OK, all good. Only minor comment that it tests segfault. We had segfaults with setjmp/longjmp before. That setjmp/longjmp stuff is really crazy sensitive. The behavior changes if e.g. move some call one stack up or down. MS don't recommend to use it in C++ code at all, if possible. But we have to, JPEG is C and there is no alternative, AFAIK. With extreme caution it works.

[dzenanz]

it tests segfault

Yes, I forgot about that 😄