Skip to content
New issue

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

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

Already on GitHub? # to your account

TEST: Fix unexercised tests for large image read/write #5151

Conversation

jadh4v
Copy link
Member

@jadh4v jadh4v commented Jan 23, 2025

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.

Closes: #3703

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:IO Issues affecting the IO module labels Jan 23, 2025
@jadh4v jadh4v changed the title TEST: Fix uncalled tests for large image read/write TEST: Fix unexercised tests for large image read/write Jan 23, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for contributing a pull request! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
More support and guidance on the contribution process can be found in our contributing guide. 📖

This is an automatic message. Allow for time for the ITK community to be able to read the pull request and comment
on it.

@dzenanz
Copy link
Member

dzenanz commented Jan 23, 2025

ITK doesn't recognize TEST: prefix, so you should use ENH: in the commit message. Otherwise looks good.

@jadh4v jadh4v force-pushed the TESTS-fix-uncalled-large-image-read-write-tests branch from 7d057cb to 83bfcfe Compare January 23, 2025 18:39
@jadh4v
Copy link
Member Author

jadh4v commented Jan 23, 2025

ITK doesn't recognize TEST: prefix, so you should use ENH: in the commit message. Otherwise looks good.

Updated the commit msg to use ENH

@jadh4v jadh4v force-pushed the TESTS-fix-uncalled-large-image-read-write-tests branch from 83bfcfe to 21e038c Compare January 23, 2025 21:32
@dzenanz
Copy link
Member

dzenanz commented Jan 24, 2025

CI runs into: Error: No space left on device. We should maybe raise the memory available limit so that these tests do not run in the CI.

@dzenanz
Copy link
Member

dzenanz commented Jan 24, 2025

Maybe a better solution would be to remove temporary files (at least for tests creating large temporaries) immediately after the test is successfully finished. But this is a lot of additional work, and could be its own issue.

Also added a minimum memory guard so that the tests
don't run on CI or other systems which cannot
support high memory usage.
@jadh4v jadh4v force-pushed the TESTS-fix-uncalled-large-image-read-write-tests branch from 21e038c to 259bd4d Compare January 27, 2025 16:46
@jadh4v
Copy link
Member Author

jadh4v commented Jan 27, 2025

No space left on device

I have increased the memory limit to 16GB. I think this should suffice for now? Removing the temp files, as you said can be a separate issue to track and could be applied across all tests.

@hjmjohnson hjmjohnson merged commit 5df209f into InsightSoftwareConsortium:master Jan 27, 2025
15 checks passed
@jhlegarreta
Copy link
Member

Maybe a better solution would be to remove temporary files (at least for tests creating large temporaries) immediately after the test is successfully finished. But this is a lot of additional work, and could be its own issue.

It ctest supported this option, it would avoid the additional work.

@dzenanz
Copy link
Member

dzenanz commented Jan 28, 2025

@bradking Does CTest support this? I suspect this would be interesting to other projects besides ITK.

@bradking
Copy link
Member

CTest does not model the inputs or outputs of a test on disk. One would have to bake the cleanup into the tests themselves via wrapper scripts or similar.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:IO Issues affecting the IO module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IO ImageBase tests not exercised
6 participants