Make ITK exceptions and their tests specific

[jhlegarreta]

Description

ITK exceptions are largely non-specific, i.e. a general itkExceptionMacro can loosely used to throw virtually any type of exception. Making exceptions be more specific, including the messages, would make the toolkit more consistent.

At the same time, the TRY_EXPECT_EXCEPTION macro is non-specific, meaning that it expects the statement to throw an exception, whichever the exception is. When writing such a statement in a test, the programmer generally expects a specific exception to be caught, i.e. an exception with a known message to be thrown. However, if the code is not designed correctly or hides a bug, the statement may well catch an exception, but one that has been thrown elsewhere/is different from the one expected. The test or macro would work correctly, because it does not check the exception type (or better, the source of the exception, or both).

PR #511 uncovered such a situation.

Impact analysis

Having non-specific exceptions provides a flexible framework, but may hide bugs. Also, if there is no way to check the exception type and/or the class that threw it, when expecting an exception, any other exception thrown in the call stack may be considered as a valid exception

Expected behavior

Creating a specific set of exceptions and enhancing the TRY_EXPECT_EXCEPTION to accept the class and exception type would help in making the code more consistent, and robust.

e.g.

TRY_EXPECT_EXCEPTION( filter->Update(), FilterClass, ExceptionType );

Actual behavior

Current exception checking:

TRY_EXPECT_EXCEPTION( filter->Update() );

Versions

master.

Additional Information

None.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jhlegarreta]

This is still relevant and can help uncovering hidden bugs.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[N-Dekker]

FYI, GoogleTest does have an EXPECT_THROW(statement, expected_exception) macro: https://github.com/InsightSoftwareConsortium/ITK/blob/v5.3rc02/Modules/ThirdParty/GoogleTest/src/itkgoogletest/googletest/include/gtest/gtest.h#L1952

Being used in various ITK GTest unit tests, for example at: https://github.com/InsightSoftwareConsortium/ITK/blob/v5.3rc02/Modules/Core/Transform/test/itkTranslationTransformGTest.cxx#L41

[jhlegarreta]

Slightly related: for methods that only display warnings, we may need to add macros to itkTestingMacros.h in order to:

• capture the standard output, and
• check that the message emitted by a method matches the one displayed

[jhlegarreta]

FYI, GoogleTest does have an EXPECT_THROW(statement, expected_exception) macro: https://github.com/InsightSoftwareConsortium/ITK/blob/v5.3rc02/Modules/ThirdParty/GoogleTest/src/itkgoogletest/googletest/include/gtest/gtest.h#L1952

Being used in various ITK GTest unit tests, for example at: https://github.com/InsightSoftwareConsortium/ITK/blob/v5.3rc02/Modules/Core/Transform/test/itkTranslationTransformGTest.cxx#L41

Those checks are still not exception-specific. This issue was raised by a situation where the same exception type was being raised, but the exception came from a class that was not the expected one, or the message did not match the expected one. EXPECT_THROW is still unable to solve this. Maybe EXPECT_THAT would.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[jhlegarreta]

This is still relevant stale bot.