Skip to content

BUG: Fail tests whose expected exception is not thrown, then convert to GoogleTest - #7046

Merged
dzenanz merged 15 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:bug-imagetolistsampleadaptor-expected-exceptions
Nov 3, 2026
Merged

dzenanz merged 15 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:bug-imagetolistsampleadaptor-expected-exceptions

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Oct 31, 2026 •

Copy link
Copy Markdown
Member

Seven tests checked that a call throws, but when nothing was thrown they only printed a message and still passed. Each gets a BUG: commit that makes the check fail. Six are legacy tests, and the next commit converts each fixed test to GoogleTest (EXPECT_THROW); itkVariableSizeMatrixGTest was already a GoogleTest, so its fix is the conversion to EXPECT_THROW.

Test Checks fixed
itkVariableSizeMatrixGTest 6 (operations on mismatched sizes)
itkImageToListSampleAdaptorTest 5 (methods called before an image is set)
itkJointDomainImageToListSampleAdaptorTest 5 (same)
itkCovarianceSampleFilterTest3 1 (CreateInvalidOutput)
itkEuclideanDistanceMetricTest 1 (Evaluate before the vector size is set)
itkManhattanDistanceMetricTest 1 (same)
itkShrinkImageTest 1 (bad requested region; the test still recovers and continues)

itkJointDomainImageToListSampleAdaptorTest has one more BUG: commit: its measurement vector has 11 components, but the test filled 6 and compared only the first 5, so pixel components 2 through 7 were never checked. It now fills and compares all 11. Its conversion uses EXPECT_NEAR, because the values are small whole numbers.

Testing

macOS arm64, Apple clang, Release, Module_ITKStatistics=ON, Module_ITKImageGrid=ON. Every commit builds with 0 warnings and passes the affected tests (14). itkVariableSizeMatrixGTest passes with the fix, which confirms that all six operations throw. Each fixed legacy test passes, which confirms the call does throw; for itkImageToListSampleAdaptorTest the fixed test also fails as intended when the image is set before the checks. For the JointDomain component fix, changing the expected value of the last component makes both the legacy test and the GoogleTest fail, while the old 5-component loop missed it. pre-commit run --all-files passes.

itkJointDomainImageToListSampleAdaptorTest has a rename-only commit before its conversion so that git log --follow tracks the file.

How these were found

A review of the staged no-argument GoogleTest conversions (gtest-staging-B, gtest-staging-C) found that the conversions copied these never-failing blocks faithfully. The fix goes to each legacy test first and the conversion follows, as with #7008 and #7040.

AI assistance (Claude Code) was used to find the defects and draft the changes; they were reviewed and tested locally.

@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module area:Numerics Issues affecting the Numerics module labels Oct 31, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review October 31, 2026 12:28
@greptile-apps

This comment was marked as resolved.

@hjmjohnson
hjmjohnson force-pushed the bug-imagetolistsampleadaptor-expected-exceptions branch from d4c50a5 to 1c020d1 Compare October 31, 2026 12:45
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push (content only, same base): new BUG commit making itkJointDomainImageToListSampleAdaptorTest compare all 11 measurement components instead of 5; its GoogleTest conversion now uses EXPECT_NEAR.

@hjmjohnson
hjmjohnson force-pushed the bug-imagetolistsampleadaptor-expected-exceptions branch from 1c020d1 to d1b7eef Compare October 31, 2026 14:28
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push (content only, same base): removed the unused itkMath.h include (Greptile) and shortened the rename commit subject to 77 characters for ghostflow.

@hjmjohnson
hjmjohnson force-pushed the bug-imagetolistsampleadaptor-expected-exceptions branch from d1b7eef to 17bc24f Compare October 31, 2026 14:42
@github-actions github-actions Bot added the area:Core Issues affecting the Core module label Oct 31, 2026
@hjmjohnson

Copy link
Copy Markdown
Member Author

Force-push (content only, same base): new first commit makes itkVariableSizeMatrixGTest fail when its six mismatched-size operations do not throw (EXPECT_THROW).

@hjmjohnson

Copy link
Copy Markdown
Member Author

First force-push (17bc24f..5c4774f) was a plain rebase on main. Second (5c4774f..8145249) is a content fix only: after the rebase, #7049's return EXIT_FAILURE had been carried into the converted itkCovarianceSampleFilterGTest3.cxx, which does not compile in a void test body; it is now FAIL().

The checks that operations on mismatched sizes throw printed the
exception but did not fail the test when nothing was thrown.
The five checks that the adaptor throws before an image is set printed
a message when nothing was thrown but did not fail the test.
…is thrown

The check that an exception is thrown printed a message when nothing
was thrown but did not fail the test.
The measurement vector has ImageDimension + 8 components, but the test
filled only six and compared the first five, so errors in pixel
components 2 through 7 went undetected.
The check that an exception is thrown printed a message when nothing
was thrown but did not fail the test.
The check that an exception is thrown printed a message when nothing
was thrown but did not fail the test.
The check that an exception is thrown printed a message when nothing
was thrown but did not fail the test.
The check that an exception is thrown printed a message when nothing
was thrown but did not fail the test.
@hjmjohnson
hjmjohnson force-pushed the bug-imagetolistsampleadaptor-expected-exceptions branch from 8145249 to b89f2cf Compare November 2, 2026 21:31
@hjmjohnson

Copy link
Copy Markdown
Member Author

Rebased on main. The only conflicts were in Modules/Numerics/Statistics/test/CMakeLists.txt, where #7048 had converted neighboring Statistics tests in the same lists; the resolution keeps both sets of changes. The 14 tests this PR touches pass locally.

@dzenanz dzenanz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good on a glance.

@dzenanz
dzenanz merged commit 8249265 into InsightSoftwareConsortium:main Nov 3, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Numerics Issues affecting the Numerics module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots 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.

2 participants