iox-#1032 Refine testing with new error reporting

[elBoberido]

Pre-Review Checklist for the PR Author

1. Add a second reviewer for complex new features or larger refactorings
2. Code follows the coding style of CONTRIBUTING.md
3. Tests follow the best practice for testing
4. Changelog updated in the unreleased section including API breaking changes
5. Branch follows the naming format (iox-123-this-is-a-branch)
6. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
7. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
8. Relevant issues are linked
9. Add sensible notes for the reviewer
10. All checks have passed (except task-list-completed)
11. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
12. Assign PR to reviewer

Notes for Reviewer

This PR resets the TestingErrorHandler at each test start to simplify the usage to the error handler for the user in tests. Additionally, the violation error codes are moved to the file with the Violation class.

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Relates to ErrorHandling concept with user defined actions #1032

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (007bdb3) 80.16% compared to head (41ec0e7) 80.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2147      +/-   ##
==========================================
- Coverage   80.16%   80.15%   -0.01%     
==========================================
  Files         418      418              
  Lines       16250    16248       -2     
  Branches     2251     2251              
==========================================
- Hits        13026    13024       -2     
  Misses       2425     2425              
  Partials      799      799              

Flag	Coverage Δ
unittests	79.94% <100.00%> (-0.01%)	⬇️
unittests_timing	15.19% <0.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...porting/custom/default/error_handler_interface.hpp	100.00% <ø> (ø)
..._reporting/custom/default/error_reporting_impl.hpp	75.00% <100.00%> (-1.48%)	⬇️
...porting/include/iox/error_reporting/error_kind.hpp	100.00% <100.00%> (ø)
...fs/reporting/include/iox/error_reporting/types.hpp	100.00% <100.00%> (ø)
...eporting/include/iox/error_reporting/violation.hpp	100.00% <100.00%> (ø)

[MatthiasKillat]

I think all these handlers (onXYZ) should not be noexcept. In theory the handler code should be flexible to throw if it desires. I do not see a reason to restrict it.

Generally I am not a fan of noexcept in generic code. This includes ode that calls a (potentially polymorphic) function that is decided on runtime and could throw for all we know. In these cases the compiler can never know whether it will throw.

But most importantly the code should not restrict any framework configuring a handler to throw. We can argue that in this particular override in the test handler does not throw, but with C++17 noexcept is part of the signature. I have not checked, but specifying the derived version as noexcept would require the base interface function to be noexcept as well. This precludes any overrides that may throw, which is undesirable.

[MatthiasKillat]

@elBoberido It is still noexcept. Is there a strong reason why? Maybe I was wrong above or did not consider something. Or it was simply forgotten. I mean it apparently compiles (C++17?) so it is probably fine. I still am not a huge fan of littering everything with noexcept in generic code

The specialization here is not generic and we know that it does not throw, so it is probably fine. I cannot point to it but I think I miss something important here. Oh well, if so, we will find out later.

Please take another look whether this still applies or you can spot the issue that I currently cannot. If not, it is fine. If in doubt, it would obviously be correct to have no noexcept specifier.

[elBoberido]

Are you sure ? onPanic has no noexcept anymore and it is the only override. All the other methods do not throw exceptions and there are now hooks to inject them.

[MatthiasKillat]

@elBoberido Generally looks fine. We should reconsider the noexcept specification in the handler though unless the base interface is not restricted in such a way (I have to look whether it has to if derived is noexcept). There is not much point in proliferation of noexcept from my experience and it causes all kinds of issues at call sites and undesired termination in without much control.

The important thing is that noexcept is seen as part of the specification and hence imposes an implementation restriction (that the compiler unfortunately cannot check). We should be careful with these restrictions and leave the user as much flexibility in these generic constructs as reasonably possible without bad trade-offs.

[MatthiasKillat]

@elBoberido What about this noexcept? Is this really useful? I had some objections here before but maybe they got lost. Have to check.

I found the comment and reopened it for inspection. Feel free to close both if you conclude it is fine.

[elBoberido]

I removed them from the onPanic and other hooks. The hasPanicked method cannot throw and has no hooks and therefore noexcept communicates exactly what the method does.

[MatthiasKillat]

@elBoberido Except for my doubts about reducing everything to IOX_ASSERT thereby dropping IOX_PRECONDITION I agree with the changes. Ultimately it comes down to what we want to accomplish and the granularity of violations we want to distinguish. I am fine to merge it as is to move forward to actually use it and make adjustments based on the usage experience if needed (should be relatively easy).

I leave it to you whether the functions in the test handler should be noexcept. I believe they should not but if they really cannot be we find out soon enough. The interface they inherit from should not be noexcept if we intend to let the client code throw (which IMO we should to be least restrictive). It is no big deal for now either way.

[elBoberido]

The typo is fixed in #2151. I had to adjust the testing error handler for MinGW anyway (for some reasons the longjmp did not work in the template method)