Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix path to write itkNiftiImageIOTest14 data #4642

Merged

Conversation

cookpa
Copy link
Contributor

@cookpa cookpa commented May 3, 2024

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

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

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

@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:IO Issues affecting the IO module labels May 3, 2024
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cookpa thanks for the follow-up 🙏

@cookpa cookpa marked this pull request as draft May 3, 2024 18:11
@cookpa
Copy link
Contributor Author

cookpa commented May 3, 2024

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
Copy link
Member

thewtex commented May 3, 2024

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

@cookpa
Copy link
Contributor Author

cookpa commented May 3, 2024

@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
Copy link
Member

dzenanz commented May 3, 2024

Yes, Windows supports forward slashes in file paths.

@cookpa
Copy link
Contributor Author

cookpa commented May 3, 2024

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
Copy link
Contributor Author

cookpa commented May 3, 2024

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.

@cookpa cookpa marked this pull request as ready for review May 3, 2024 19:18
@hjmjohnson
Copy link
Member

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.

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this so quickly @cookpa.

Left an inline comment.

@cookpa
Copy link
Contributor Author

cookpa commented May 6, 2024

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

@cookpa cookpa marked this pull request as draft May 6, 2024 15:49
@thewtex thewtex marked this pull request as ready for review May 6, 2024 17:52
Also throw exception if NiftiIOTest14 fails to read file.
@cookpa
Copy link
Contributor Author

cookpa commented May 6, 2024

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
Copy link
Member

thewtex commented May 7, 2024

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.

@thewtex thewtex merged commit e2cdab4 into InsightSoftwareConsortium:master May 7, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants