Skip to content

Commit

Permalink
Merge pull request #2147 from elBoberido/iox-1032-refine-testing-with…
Browse files Browse the repository at this point in the history
…-new-error-reporting

iox-#1032 Refine testing with new error reporting
  • Loading branch information
elBoberido committed Jan 11, 2024
2 parents 007bdb3 + 41ec0e7 commit 8a5e083
Show file tree
Hide file tree
Showing 31 changed files with 308 additions and 381 deletions.
20 changes: 10 additions & 10 deletions .cirrus.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ ubuntu_22_04_aarch64_build_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: true
fingerprint_key: $CIRRUS_OS_ubuntu_22_04_aarch64_test_binaries_cache
fingerprint_key: $CIRRUS_OS_ubuntu_22_04_aarch64_test_binaries_cache_$CIRRUS_BRANCH
build_script:
<<: *IOX_POSIX_CLEAN_BUILD_STRICT_WITH_ADDITIONAL_USER
populate_test_binary_folder_script:
Expand All @@ -140,7 +140,7 @@ ubuntu_22_04_aarch64_test_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: false
fingerprint_key: $CIRRUS_OS_ubuntu_22_04_aarch64_test_binaries_cache
fingerprint_key: $CIRRUS_OS_ubuntu_22_04_aarch64_test_binaries_cache_$CIRRUS_BRANCH
test_script:
<<: *IOX_RUN_TESTS

Expand All @@ -157,7 +157,7 @@ arch_linux_x64_gcc_8_3_aka_qnx_canary_build_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: true
fingerprint_key: $CIRRUS_OS_archlinux_x64_gcc_8_3_aka_qnx_canary_test_binaries_cache
fingerprint_key: $CIRRUS_OS_archlinux_x64_gcc_8_3_aka_qnx_canary_test_binaries_cache_$CIRRUS_BRANCH
env:
# use GCC 8.3 which corresponds to QCC 8.3 on QNX 7.1
CC: gcc-8
Expand All @@ -176,7 +176,7 @@ arch_linux_x64_gcc_8_3_aka_qnx_canary_test_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: false
fingerprint_key: $CIRRUS_OS_archlinux_x64_gcc_8_3_aka_qnx_canary_test_binaries_cache
fingerprint_key: $CIRRUS_OS_archlinux_x64_gcc_8_3_aka_qnx_canary_test_binaries_cache_$CIRRUS_BRANCH
test_script:
<<: *IOX_RUN_TESTS

Expand All @@ -194,7 +194,7 @@ arch_linux_x64_build_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: true
fingerprint_key: $CIRRUS_OS_archlinux_x64_test_binaries_cache
fingerprint_key: $CIRRUS_OS_archlinux_x64_test_binaries_cache_$CIRRUS_BRANCH
build_script:
<<: *IOX_POSIX_CLEAN_BUILD_STRICT_WITH_ADDITIONAL_USER
populate_test_binary_folder_script:
Expand All @@ -210,7 +210,7 @@ arch_linux_x64_test_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: false
fingerprint_key: $CIRRUS_OS_archlinux_x64_test_binaries_cache
fingerprint_key: $CIRRUS_OS_archlinux_x64_test_binaries_cache_$CIRRUS_BRANCH
test_script:
<<: *IOX_RUN_TESTS

Expand All @@ -227,7 +227,7 @@ freebsd_x64_build_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: true
fingerprint_key: $CIRRUS_OS_freebsd_x64_test_binaries_cache
fingerprint_key: $CIRRUS_OS_freebsd_x64_test_binaries_cache_$CIRRUS_BRANCH
setup_script:
- pkg install -y cmake git ncurses bash wget
- ln -s /usr/local/bin/bash /bin/bash
Expand All @@ -245,7 +245,7 @@ freebsd_x64_test_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: false
fingerprint_key: $CIRRUS_OS_freebsd_x64_test_binaries_cache
fingerprint_key: $CIRRUS_OS_freebsd_x64_test_binaries_cache_$CIRRUS_BRANCH
test_script:
<<: *IOX_RUN_TESTS

Expand All @@ -263,7 +263,7 @@ macos_aarch64_build_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: true
fingerprint_key: $CIRRUS_OS_macOS_aarch64_test_binaries_cache
fingerprint_key: $CIRRUS_OS_macOS_aarch64_test_binaries_cache_$CIRRUS_BRANCH
setup_script:
- brew install ncurses
build_script:
Expand All @@ -281,7 +281,7 @@ macos_aarch64_test_task:
test_binaries_cache:
folder: iox-tests-bin
reupload_on_changes: false
fingerprint_key: $CIRRUS_OS_macOS_aarch64_test_binaries_cache
fingerprint_key: $CIRRUS_OS_macOS_aarch64_test_binaries_cache_$CIRRUS_BRANCH
env:
# No timing tests on macOS
GTEST_FILTER: "-*TimingTest*"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,19 @@
//
// SPDX-License-Identifier: Apache-2.0

#include <gmock/gmock.h>
#include <gtest/gtest.h>

using namespace ::testing;
using ::testing::_;
#include "iceoryx_hoofs/testing/error_reporting/testing_error_handler.hpp"
#include "iceoryx_hoofs/testing/testing_logger.hpp"

#include <gmock/gmock.h>
#include <gtest/gtest.h>

int main(int argc, char* argv[])
{
::testing::InitGoogleTest(&argc, argv);

iox::testing::TestingLogger::init();
iox::testing::TestingErrorHandler::init();

return RUN_ALL_TESTS();
}
65 changes: 22 additions & 43 deletions doc/design/error_reporting.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,84 +157,63 @@ Fatal errors can be conditionally reported in a similar way.
IOX_REPORT_FATAL_IF(x<0, Code::OutOfBounds);
```
### Require That a Condition Holds
### Enforce a Condition
Similarly we can conditionally check whether a condition does hold and report a fatal error in
Similarly we can conditionally enforce whether a condition does hold and report a fatal error in
the case that it does not hold
```cpp
int x;
// ...
IOX_REQUIRE(x>=0, "required condition violation message");
IOX_ENFORCE(x>=0, "enforce violation message");
```

The condition is required to hold and this requirement is always checked.
If the condition does not hold, panic is invoked and the execution stops.

This should be used for conditions that may not hold on the correct path, e.g. for error cases.
It should not be used for assumptions that have to be true in correct code
(use `IOX_ASSUME` or `IOX_PRECONDITION` for this).
(use `IOX_ASSERT` for this).

Note that no condition can generally be enforced in the sense that it must be true and no checking is required.

## Using the API to Check Contracts and Assumptions
### Assert a Condition

The following checks can be disabled and are intended to increase safety during incorrect
The following check is disabled for release builds and is intended to increase safety during incorrect
use, specifically detect incorrect use at runtime.

This means they should only be used for checks that are not needed in correct code (i.e. defensive
This means it should only be used for checks that are not needed in correct code (i.e. defensive
programming).

If these checks are disabled, there is no overhead in the code, i.e. no checking or reporting takes place.

### Checking Preconditions

A precondition check
When check is disabled, there is no overhead in the code, i.e. no checking or reporting takes place.

```cpp
int f(int x)
{
IOX_PRECONDITION(x>=0, "precondition violation message");
IOX_ASSERT(x>=0, "precondition violation message");

// ...
// some computation
int y = g(x);

IOX_ASSERT(y>=0, "assumption violation message");
// proceed assuming that y>=0 holds
}
```
is used to verify assumptions **BEFORE** any logic in the function body is executed. Technically copy
Is can be used to verify preconditions before any logic in the function body is executed. Technically copy
constructors may run before any condition can be checked, and there is also the possibility of
reordering if the following code does not depend on the condition at all.
This is not a problem since any reordering is not allowed to affect the observable result.
Specifically it cannot affect the value of the precondition itself as this would change the
observable behaviour.
When used in the middle of a function it serves as documentation of assumptions that should hold at this
point in the code before the next statement and can be used e.g. to check for out-of-bounds accesses. It can
also be used to check postconditions.
In case of violation, the violation and a (potentially empty) message are forwarded to the backend,
panic is invoked and execution stops.
The verification can be optionally disabled, and hence this also documents assumptions of the
function itself.
### Checking Assumptions
Checking assumptions is similar to checking preconditions, but can happen anywhere in the code.
```cpp
int f(int x)
{
// some computation
int y = g(x);
IOX_ASSUME(y>=0, "assumption violation message");
// proceed assuming that y>=0 holds
}
```

These serve as documentation of assumptions that should hold at this point in the code before the
next statement and can be used e.g. to check for out-of-bounds accesses. It can also be used to
check postconditions.

It should not be used at the start of a function body and instead replaced with a precondition check
in this case.

## Marking Unreachable Code
It is also possible to explicitly state that code is supposed to be unreachable.
Expand Down Expand Up @@ -394,7 +373,7 @@ Alternatively the shorthand version can be used
int algorithm(int x)
{
// require that the condition holds or raise a fatal error
IOX_REQUIRE(!errorCondition(x), SomeError);
IOX_REPORT_FATAL_IF(errorCondition(x), SomeError);
return 42;
}
```
Expand Down Expand Up @@ -425,7 +404,7 @@ These are
8. `errors.hpp` : supported error types and related free functions

Additionally there is the `assertions.hpp` in `iceoryx_hoofs/reporting` which contains the `IOX_PANIC`,
`IOX_UNREACHABLE`, `IOX_REQUIRE`, `IOX_PRECONDITION` and `IOX_ASSUME` macros
`IOX_UNREACHABLE`, `IOX_ASSERT` and `IOX_ENFORCE` macros.

All the files focus on singular aspects to allow fine-grained inclusion.
All definitions have to reside in `iox::er`, which is considered a private (detail) namespace
Expand Down Expand Up @@ -463,7 +442,7 @@ The default implementation does not depend on any code that uses the error repor
### Testing

All testing related definitions are located in `iceoryx_hoofs/testing/error_reporting`.
These are the definition of `TestErrorHandler` in `testing_error_handler.hpp` and auxiliary
These are the definition of `TestingErrorHandler` in `testing_error_handler.hpp` and auxiliary
functions in `testing_support.hpp` to be used in tests to verify errors.
The latter can be extended as required.

Expand Down
5 changes: 2 additions & 3 deletions iceoryx_binding_c/test/moduletests/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//
// SPDX-License-Identifier: Apache-2.0

#include "iceoryx_hoofs/testing/error_reporting/testing_error_handler.hpp"
#include "iceoryx_hoofs/testing/testing_logger.hpp"

#include "test.hpp"
Expand All @@ -24,9 +25,6 @@

#include "test_types_storage_size.hpp"

using namespace ::testing;
using ::testing::_;

// in case this isn't executed before the other tests, just call
// 'checkIceoryxBindingCStorageSizes' directly in main
TEST(SanityCheck, CheckStorageSizeAndAlingment)
Expand All @@ -41,6 +39,7 @@ int main(int argc, char* argv[])
::testing::InitGoogleTest(&argc, argv);

iox::testing::TestingLogger::init();
iox::testing::TestingErrorHandler::init();

return RUN_ALL_TESTS();
}
41 changes: 13 additions & 28 deletions iceoryx_hoofs/reporting/include/iox/assertions.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,50 +51,35 @@
//* Instead a special internal error type is used.
//************************************************************************************************

/// @brief report fatal required condition violation if expr evaluates to false
/// @note for conditions that may actually happen during correct use
/// @brief only for debug builds: report fatal assert violation if expr evaluates to false
/// @note for conditions that should not happen with correct use
/// @param expr boolean expression that must hold
/// @param message message to be forwarded in case of violation
#define IOX_REQUIRE(expr, message) \
#define IOX_ASSERT(expr, message) \
do \
{ \
if (!(expr)) \
if (iox::er::Configuration::CHECK_ASSERT && !(expr)) \
{ \
iox::er::forwardFatalError(iox::er::Violation::createRequiredConditionViolation(), \
iox::er::REQUIRED_CONDITION_VIOLATION, \
CURRENT_SOURCE_LOCATION, \
message); /* @todo iox-#1032 add strigified 'expr' as '#expr' */ \
} \
} while (false)

/// @brief if enabled: report fatal precondition violation if expr evaluates to false
/// @param expr boolean expression that must hold upon entry of the function it appears in
/// @param message message to be forwarded in case of violation
#define IOX_PRECONDITION(expr, message) \
do \
{ \
if (iox::er::Configuration::CHECK_PRECONDITIONS && !(expr)) \
{ \
iox::er::forwardFatalError(iox::er::Violation::createPreconditionViolation(), \
iox::er::PRECONDITION_VIOLATION, \
iox::er::forwardFatalError(iox::er::Violation::createAssertViolation(), \
iox::er::ASSERT_VIOLATION, \
CURRENT_SOURCE_LOCATION, \
message); \
} \
} while (false)

/// @brief if enabled: report fatal assumption violation if expr evaluates to false
/// @note for conditions that should not happen with correct use
/// @brief report fatal enforce violation if expr evaluates to false
/// @note for conditions that may actually happen during correct use
/// @param expr boolean expression that must hold
/// @param message message to be forwarded in case of violation
#define IOX_ASSUME(expr, message) \
#define IOX_ENFORCE(expr, message) \
do \
{ \
if (iox::er::Configuration::CHECK_ASSUMPTIONS && !(expr)) \
if (!(expr)) \
{ \
iox::er::forwardFatalError(iox::er::Violation::createAssumptionViolation(), \
iox::er::ASSUMPTION_VIOLATION, \
iox::er::forwardFatalError(iox::er::Violation::createEnforceViolation(), \
iox::er::ENFORCE_VIOLATION, \
CURRENT_SOURCE_LOCATION, \
message); \
message); /* @todo iox-#1032 add strigified 'expr' as '#expr' */ \
} \
} while (false)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ struct ConfigurationParameters
{
static_assert(std::is_same<T, ConfigurationTag>::value, "Incorrect configuration tag type");

static constexpr bool CHECK_PRECONDITIONS{true};
static constexpr bool CHECK_ASSUMPTIONS{true};
static constexpr bool CHECK_ASSERT{true}; /// @todo iox-#1032 deactive for release builds
};

// used by the API to obtain the compile time parameters
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ namespace er
template <>
struct ConfigurationParameters<ConfigurationTag>
{
static constexpr bool CHECK_PRECONDITIONS{true};
static constexpr bool CHECK_ASSUMPTIONS{true};
static constexpr bool CHECK_ASSERT{true}; /// @todo iox-#1032 deactive for release builds
};

} // namespace er
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
#ifndef IOX_HOOFS_REPORTING_ERROR_REPORTING_CUSTOM_ERROR_HANDLER_INTERFACE_HPP
#define IOX_HOOFS_REPORTING_ERROR_REPORTING_CUSTOM_ERROR_HANDLER_INTERFACE_HPP

#include "iox/error_reporting/errors.hpp"
#include "iox/error_reporting/source_location.hpp"
#include "iox/error_reporting/types.hpp"
#include "iox/error_reporting/violation.hpp"

namespace iox
{
Expand Down
Loading

0 comments on commit 8a5e083

Please sign in to comment.