Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

iox-#1099 Separate test part from ErrorHandler and call errorHandler() in iceoryx_hoofs only once #1100

Conversation

mossmaurice
Copy link
Contributor

@mossmaurice mossmaurice commented Feb 12, 2022

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-#123-this-is-a-branch)
  5. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. Assign PR to reviewer

Notes for Reviewer

  • Separate test specific code from the ErrorHandler and templatize setTemporaryErrorHandler(..)
  • Call errorHandler(..) only once in iceoryx_hoofs in Expects / Ensures

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
    • Each unit test case has a unique UUID
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@mossmaurice mossmaurice added refactoring Refactor code without adding features technical debt unclean code and design flaws labels Feb 12, 2022
@mossmaurice mossmaurice self-assigned this Feb 12, 2022
@codecov
Copy link

codecov bot commented Feb 12, 2022

Codecov Report

Merging #1100 (28d6a5d) into master (418b483) will increase coverage by 0.01%.
The diff coverage is 73.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1100      +/-   ##
==========================================
+ Coverage   78.93%   78.95%   +0.01%     
==========================================
  Files         370      371       +1     
  Lines       14705    14683      -22     
  Branches     2059     2051       -8     
==========================================
- Hits        11608    11593      -15     
+ Misses       2417     2415       -2     
+ Partials      680      675       -5     
Flag Coverage Δ
unittests 78.17% <61.53%> (+<0.01%) ⬆️
unittests_timing 15.46% <15.38%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...clude/iceoryx_hoofs/internal/cxx/variant_queue.inl 78.31% <ø> (ø)
...iceoryx_hoofs/internal/file_reader/file_reader.hpp 100.00% <ø> (ø)
...eoryx_posh/internal/runtime/ipc_interface_base.hpp 100.00% <ø> (ø)
...oryx_posh/include/iceoryx_posh/roudi/roudi_app.hpp 40.00% <ø> (ø)
iceoryx_hoofs/source/file_reader/file_reader.cpp 71.42% <50.00%> (-6.84%) ⬇️
iceoryx_hoofs/source/posix_wrapper/timer.cpp 65.53% <80.00%> (+2.46%) ⬆️
...x_hoofs/internal/error_handling/error_handling.inl 100.00% <100.00%> (ø)
...ryx_hoofs/source/error_handling/error_handling.cpp 60.00% <100.00%> (-7.57%) ⬇️
iceoryx_hoofs/source/concurrent/loffli.cpp 80.00% <0.00%> (-11.43%) ⬇️
... and 1 more

Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

Please squash or reorder and squash some commits and revert some changes by dropping some commits like:

  1. Update copyright header, this really clutters our git history.
  2. The last two one liner commits.

Please create one commit here instead of 4 and add the copyright year findings to it.

I suggest to squash and drop the commits as a first step. After you done this we can go to the next review stage where you try to avoid rebasing/push force so that all the reviewers see only the changes. But lets start with a more or less clean git history and when it clutters up in the later review process then just let it clutter up.

CONTRIBUTING.md Outdated
@@ -110,8 +110,8 @@ codebase follows these rules, things are work in progress.
our code may contain additions which are not compatible with the STL (e.g. `iox::cxx::vector::emplace_back()`
does return a bool)
7) **Always use `iox::log::Logger`**, instead of `printf()`
8) **Always use `iox::ErrorHandler()`**, when an error occurs that cannot or shall not be propagated via an
`iox::cxx::expected`, the `iox::ErrorHandler()` shall be used; exceptions are not allowed
8) **Always use `iox::ErrorHandler()` or `cxx::Expects`/`cxx::Ensures`**, when an error occurs that cannot or shall not be propagated via an
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion this will be very soon outdated when the low level error handler becomes also available for iceoryx hoofs. Then it would make sense to never use cxx::Expects / cxx::Ensures (even when they use the error handler behind the scenes).
I would state here right from the beginning: always use the iox::ErrorHandler since we can emit an error code here which can be important for later bug reports or debugging.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think having indivual error codes in iceoryx hoofs (e.g. when a vector capacity is exceeded) is a good idea. In my opinion this does not belong into a generic library. We should use a generic hoofs error category (or categories, exception like).

Regardless, this is for another time and we should state how the handling is currently intended, even if it will change in the (near) future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it makes sense to use errorHandler() in iceoryx_hoofs building blocks. IMHO it's more idiomatic and closer to a library philosophy to use Expects / Ensures and allow the users to call the errorHandler() inside Expects / Ensures (or throw or std::terminate).

However, we should not discuss this here, that's something for #1032

The goal for v2.0 is to decouple iceoryx_posh and iceoryx_binding_c from iceoryx_hoofs as an intermediate step. Hence, the description above is still valid.

cc @MatthiasKillat

@@ -17,6 +17,8 @@
#ifndef IOX_DDS_GATEWAY_TEST_TEST_HPP
#define IOX_DDS_GATEWAY_TEST_TEST_HPP

#include "iceoryx_hoofs/testing/mocks/error_handler_mock.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we actually require an error handler mock? We can always set a temporary error handler and check if it was called with the correct arguments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I had the same observation in the previous PR. I do not think we require a mock or it is actually a mock, we just want to change to a different ErrorHandler for the tests (which is one reason to have a configurable ErrorHandler) . It somehow looks like a design flaw or I do not understand it correctly.

Should at least be considered in another refactoring pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ErrorHandlerMock contains call code which is only needed in test code. Now, ErrorHandler on the other hand, contains the code needed during runtime of an iceoryx system and is not cluttered with test-only code anymore. The templatization of setTemporaryErrorHandler is necessary to provide a user friendly way to use a different error enums for each module (posh, C binding).

iceoryx_hoofs/source/error_handling/error_handling.cpp Outdated Show resolved Hide resolved
std::cerr << "\033[5;31m"
<< "Could not open file '" << m_file << "'. Exiting!"
<< "\033[0m" << std::endl;
cxx::Ensures(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we will most likely use the error handler in the complete code base, why not use it here with a newly created enum to spare us some future work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not do this in the scope of this PR since the concept is still not fully clear. Just cause no regression compared to previous code.

Reason: Global error handling cannot be applied if the user is supposed to somehow configure the error handling of FileReader individually (i.e. decoupled from the global error handling). And if it is just the terminate case Ensures is sufficient for now.

Copy link
Contributor Author

@mossmaurice mossmaurice Feb 15, 2022

Choose a reason for hiding this comment

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

Sorry, I disagree with using the errorHandler() directly in iceoryx_hoofs. See this comment for the background. That's something to be discussed in #1032. For v2.0 I would like to implement our current error handling strategy as described in doc/design/error-handling.md to have a consistent code base.

errorHandler(Error::kPOSIX_TIMER__INCONSISTENT_STATE);
return;
}
cxx::Expects((handle.m_timer != nullptr) && "Timer in inconsistent state");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the error handler here and in all remaining files where you added an cxx::Expects / cxx::Ensures or where you removed the errorHandler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Disagree. The entire point AFAIK is not to use the error handler in hoofs directly (and I agree with this). Expects may call or may not call it, this is implementation specific.

On another note Expects is just a runtime assert in its most basic form and I do not really see a reason to not name it Assert as we cannot use it as a true precondition check (enforced as a contract!) as the standard says https://doc.bccnsoft.com/docs/cppreference2018/en/cpp/language/attributes/contract.html (same with Ensures). To do so we need compiler support which we do not have (i.e. we cannot implement this in with C++17 alone).

Whenever something like this is checked in the function body it is an assert (as per the standard) and I would not deviate from this (as it has no point and just causes confusion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rejected, quote from doc/design/error-handling.md:

The error handler cannot be used in hoofs.

See this comment for the background an my opinion. For v2.0 I would like to implement our current error handling strategy.

I'm happy to discuss changes to the error handling strategy in #1032 for v3.0.

std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT.toMilliseconds() * 10));
TIMING_TEST_EXPECT_FALSE(hasTerminated);
// EXPECT_NO_DEATH
EXPECT_TRUE(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the changes (in whole file). cxx::Expects / cxx::Ensures are calling the error handler and terminate the program. When this test fails the whole test is terminated and the user is wondering what happened.

If possible, a unit test should never die! We always want a list of all the failing unit tests with the condition/code line which failed - this test setup makes this impossible since we do not know if the test failed or some segfault occurred.

Please do this by dropping the commit otherwise we clutter or history with changes which actually never happened.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to exchange the error handling to some specific test handling where we do not terminate but indicate that an error occurred instead ... If this is not possible right now (why not?) it needs to be possible in the future at least (and a todo added).

Furthermore if the test success is determined at some point we should use SUCCEED().

But I thought we will get rid of the POSIX timer which would make all this a non-issue.

Copy link
Contributor Author

@mossmaurice mossmaurice Feb 15, 2022

Choose a reason for hiding this comment

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

You're right, DEATH_TESTS should be avoided. However, this is an intermediate step as the posix::Timer and its tests will be removed before v2.0 e.g. by finishing off #754 or re-implementing it from scratch.

@@ -0,0 +1,81 @@
// Copyright (c) 2022 by Apex.AI Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need it and cannot use the setTemporaryErrorHandler?

Please drop this change from the history as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, the ErrorHandler is free from any test code, see more in my previous comment.

@elBoberido
Copy link
Member

@mossmaurice can't promise to look at this anytime soon since I'm quite busy with the request-response feature

@mossmaurice mossmaurice force-pushed the iox-#1099-separate-test-part-and-call-error-handler-in-hoofs-once branch from 068001c to 258bbcd Compare February 15, 2022 15:48
@mossmaurice mossmaurice force-pushed the iox-#1099-separate-test-part-and-call-error-handler-in-hoofs-once branch from 258bbcd to 907ad93 Compare February 15, 2022 17:53
@mossmaurice mossmaurice force-pushed the iox-#1099-separate-test-part-and-call-error-handler-in-hoofs-once branch 2 times, most recently from bc69ced to 0f40aea Compare February 26, 2022 21:26
Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Looks good in general. I'm just not sure about the new death tests, since the make problems on macOS

Comment on lines +28 to +33
class ErrorHandlerMock : protected ErrorHandler
{
public:
template <typename Error>
static cxx::GenericRAII setTemporaryErrorHandler(const TypedHandlerFunction<Error>& newHandler) noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having to call a function, why not make this the ctor?

Copy link
Contributor Author

@mossmaurice mossmaurice Feb 28, 2022

Choose a reason for hiding this comment

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

That would be possible. However, using setTemporaryErrorHandler() in tests is more meaningful for me. I use a mock and call a method to set a certain behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

For me it is the oposite. It's like with the std::lock. One creates the object and let it's dtor do everything.

iceoryx_posh/test/moduletests/test_popo_sample.cpp Outdated Show resolved Hide resolved
@mossmaurice mossmaurice force-pushed the iox-#1099-separate-test-part-and-call-error-handler-in-hoofs-once branch from 1ae5fd0 to 386f5c0 Compare February 28, 2022 08:07
@mossmaurice
Copy link
Contributor Author

mossmaurice commented Feb 28, 2022

I'm just not sure about the new death tests, since the make problems on macOS

Let's delete the posix::Timer and its tests for good finally! This should be done right after v2.0

@mossmaurice mossmaurice force-pushed the iox-#1099-separate-test-part-and-call-error-handler-in-hoofs-once branch 2 times, most recently from d0d769e to c7a6cb4 Compare February 28, 2022 14:48
@mossmaurice mossmaurice force-pushed the iox-#1099-separate-test-part-and-call-error-handler-in-hoofs-once branch from c7a6cb4 to 6f8560e Compare March 14, 2022 22:55
Copy link
Contributor

@MatthiasKillat MatthiasKillat left a comment

Choose a reason for hiding this comment

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

Ok in general for the purpose of separating the error handling but we need to rethink some of the error handling in another issue. I also think ErrorHandlerMock is the wrong name, it is the TestErrorHandler or something like this.

What happened to typed_unique_id.hpp?

CONTRIBUTING.md Outdated
@@ -110,8 +110,8 @@ codebase follows these rules, things are work in progress.
our code may contain additions which are not compatible with the STL (e.g. `iox::cxx::vector::emplace_back()`
does return a bool)
7) **Always use `iox::log::Logger`**, instead of `printf()`
8) **Always use `iox::ErrorHandler()`**, when an error occurs that cannot or shall not be propagated via an
`iox::cxx::expected`, the `iox::ErrorHandler()` shall be used; exceptions are not allowed
8) **Always use `iox::ErrorHandler()` or `cxx::Expects`/`cxx::Ensures`**, when an error occurs that cannot or shall not be propagated via an
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think having indivual error codes in iceoryx hoofs (e.g. when a vector capacity is exceeded) is a good idea. In my opinion this does not belong into a generic library. We should use a generic hoofs error category (or categories, exception like).

Regardless, this is for another time and we should state how the handling is currently intended, even if it will change in the (near) future.

@@ -17,6 +17,8 @@
#ifndef IOX_DDS_GATEWAY_TEST_TEST_HPP
#define IOX_DDS_GATEWAY_TEST_TEST_HPP

#include "iceoryx_hoofs/testing/mocks/error_handler_mock.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I had the same observation in the previous PR. I do not think we require a mock or it is actually a mock, we just want to change to a different ErrorHandler for the tests (which is one reason to have a configurable ErrorHandler) . It somehow looks like a design flaw or I do not understand it correctly.

Should at least be considered in another refactoring pass.

// We undo the type erasure
auto typedError = static_cast<ErrorEnumType>(error);
typedHandler<iox::Error>.and_then(
[&](TypedHandlerFunction<Error> storedHandler) { storedHandler(typedError, level); });
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a storedHandler though? At this point it is merely a passed argument, nor will it be stored (so maybe only handler). Furthermore we pass by value here, is this a good idea/necessary since we call the function right away and are done. Could we encounter a dangling reference? If so, is it due to bad design or unavoidable or a misuse?

Copy link
Contributor Author

@mossmaurice mossmaurice Mar 15, 2022

Choose a reason for hiding this comment

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

Yes, the handler is stored inside typedHandler. If the cxx::optional contains a value, this handler is invoked. It is not possible to encounter a dangling reference, as the resetting is done via a cxx::GenericRAII. Misuse is therefore very low. Semantically there is no change to before.

Copy link
Contributor

@MatthiasKillat MatthiasKillat Mar 15, 2022

Choose a reason for hiding this comment

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

OK, but I mean in the context of the lambda it is meaningless whether it is/was stored or not so I would have dropped this from the name (it does not help clarity, it just adds confusion IMO).

But it is a small scope so I do not care too much.


namespace iox
{
std::mutex ErrorHandlerMock::handler_mutex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this snake case break with our (local) conventions or is there more of a purpose?

I prefer snake case for certain library abstractions but not variables like this one, unless all variables in the module follow this convention.

But more importantly, it is a global variable in the iox namespace but is actually something specific to error handling. We should try to avoid polluting the iox namespace with this. If we cannot avoid the global mutex (can we confine it to some class as a static?) maybe it should live in something like iox::error_handling::detail.

The convention being that the any detail namespace is certainly not to be accessed by the user (even if it can be accessed to write a global variable like this one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the snake case naming. We can think about wrapping parts of the error_handling in objects in an extra namespace. For me this not an important topic, as the class is already separate in a testing package. Hence, not a big deal for me.

iceoryx_posh/test/moduletests/test_popo_sample.cpp Outdated Show resolved Hide resolved
@mossmaurice
Copy link
Contributor Author

Ok in general for the purpose of separating the error handling but we need to rethink some of the error handling in another issue. I also think ErrorHandlerMock is the wrong name, it is the TestErrorHandler or something like this.

Agree, that will happen as part of #1032.

What happened to typed_unique_id.hpp?

typed_unique_id.hpp was an empty left over file, which was forgotten in a previous PR.

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Can you please remove the empty typed_unique_id.hpp

Comment on lines +28 to +33
class ErrorHandlerMock : protected ErrorHandler
{
public:
template <typename Error>
static cxx::GenericRAII setTemporaryErrorHandler(const TypedHandlerFunction<Error>& newHandler) noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

For me it is the oposite. It's like with the std::lock. One creates the object and let it's dtor do everything.

inline void errorHandlerForTest(const uint32_t error, const char* errorName, const ErrorLevel level) noexcept
{
uint32_t errorEnumType = error >> 16;
uint32_t expectedErrorEnumType =
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this just be 0 with a comment why it currently 0 and what will change?

@mossmaurice
Copy link
Contributor Author

mossmaurice commented Mar 16, 2022

Can you please remove the empty typed_unique_id.hpp

@elBoberido It's already removed, maybe just harder to catch in GitHub web GUI due to being empty.

@mossmaurice mossmaurice dismissed elfenpiff’s stale review March 16, 2022 16:26

Findings addressed

@mossmaurice mossmaurice merged commit 86deed9 into eclipse-iceoryx:master Mar 16, 2022
@mossmaurice mossmaurice deleted the iox-#1099-separate-test-part-and-call-error-handler-in-hoofs-once branch March 16, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactor code without adding features technical debt unclean code and design flaws
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate module specific errors from iceoryx_hoofs
4 participants