InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.37k stars 660 forks source link

BUG: Fix path to write itkNiftiImageIOTest14 data #4642

Closed cookpa closed 1 month ago

cookpa commented 2 months ago

Fixes a test failure introduced by #4595

@jhlegarreta, this works on my machine, but then so did the previous test. According to the error message here

https://open.cdash.org/tests/1540861560

** ERROR (nifti_image_write_engine): cannot open output file '/a_blatantly_obvious/bad_file_path/that/should/never/exist/on/the/computer/xyzt_units_output_check.nii'
Correctly caught exception for attempting to write to an invalid file.
Caught an exception: 

itk::ImageFileReaderException (0x6000037a4620)
Location: "unknown" 
File: /Users/runner/work/ITK/ITK/Modules/IO/ImageBase/include/itkImageFileReader.hxx
Line: 135
Description:  Could not create IO object for reading file xyzt_units_output_check.nii
The file doesn't exist. 
Filename = xyzt_units_output_check.nii

 /Users/runner/work/ITK/ITK/Modules/IO/ImageBase/include/itkIOTestHelper.h 54
Exception caught while reading image back in

It looks like a previous test somehow set the working directory to /a_blatantly_obvious/bad_file_path/that/should/never/exist/on/the/computer/. But I could not locate where this might have happened.

My mistake was to not include the test directory (passed to the test as ${ITK_TEST_OUTPUT_DIR}) in my output file name. I fixed the test to write to the full path.

PR Checklist

Refer to the ITK Software Guide for further development details if necessary.

cookpa commented 2 months ago

Actually I realize this might still not work

const std::string output_test_fn = std::string(test_dir) + "xyzt_units_output_check.nii";

Because it assumes a path separator is at the end of test_dir. Other tests use

itksys::SystemTools::ChangeDirectory(testdir);

I think I will do that as well to avoid the potential for other problems with the bad working directory

thewtex commented 2 months ago

If it is possible to pass in full paths to the test as args, that generally avoids issues.

cookpa commented 2 months ago

@thewtex is it OK for me to use a forward slash, like

  const std::string output_test_fn = std::string(test_dir) + "/xyzt_units_output_check.nii";
dzenanz commented 2 months ago

Yes, Windows supports forward slashes in file paths.

cookpa commented 2 months ago

One unrelated change in this last commit: while running in verbose mode to look into the path issue, I noticed the metadata wasn't being checked correctly. The metadata is correct - it just would not have failed the test if it was wrong.

I also figured out that the /a_blatantly_obvious/bad_file_path message is actually not an error, it's done by itk::IOTestHelper::WriteImage to test that an exception is thrown with a bad path.

cookpa commented 2 months ago

One more change, I added a line to throw the exception if the nifti file cannot be read in. That might shed more light on the failure.

hjmjohnson commented 2 months ago

NOTE: "/a_blatantly_obvious/bad_file_path/that/should/never/exist/on/the/computer/" is purposfully not supposed to exist. It is a path that is used to test proper behavior when a path does not exist.

cookpa commented 2 months ago

I think this is ready now, I can rebase if needed but I think admins can squash and merge directly now

cookpa commented 1 month ago

Hi @thewtex I was going to make the path change as suggested by @blowekamp but I see you've got other things going on with this PR. I think it will work as-is, so happy to leave it be if that makes things easier.

thewtex commented 1 month ago

Hi @cookpa , yes if you could please make that improvement in a follow-up PR, it will be appreciated. I will merge this now so we can get clean builds for other PRs.