GTEST: refactor log handlers using RAII idiom #2958
Conversation
test/gtest/common/test.cc
Outdated
| void test_base::wrap_errors() | ||
| { | ||
| ucs_log_push_handler(wrap_errors_logger); | ||
| test_base::scoped_log_handler::scoped_log_handler(scoped_log_handler_type_t type) { |
There was a problem hiding this comment.
maybe pass handler func ptr as a pointer? would not need switch/case, and avoid defining specific function like sockaddr_errs_detector in base class
There was a problem hiding this comment.
maybe also add new C++ and use lambda? For example smth like
ucs_status_t status;
{
scoped_log_handler hide_err(hide_errors_logger);
status = ucp_atomic_add64(e->ep(), 0, (uintptr_t)memheap_addr + 1, rkey);
}
EXPECT_EQ(UCS_ERR_INVALID_PARAM, status);
may be replaced with smth like
ucs_status_t status = [] (ucs_log_func_t log_func) {
scoped_log_handler hide_err(log_func);
return ucp_atomic_add64(e->ep(), 0, (uintptr_t)memheap_addr + 1, rkey);
} (hide_errors_logger);
EXPECT_EQ(UCS_ERR_INVALID_PARAM, status);
where hide_errors_logger also may be a local lambda when it's needed
There was a problem hiding this comment.
i would like that, but today we don't require c11
|
Test FAILed. |
|
Test FAILed. |
Increase scope for reqs array allocated on stack
|
Test FAILed. |
|
Test FAILed. |
|
Test PASSed. |
|
Test PASSed. |
| @@ -153,6 +153,11 @@ void ucs_log_pop_handler() | |||
| } | |||
| } | |||
|
|
|||
| unsigned ucs_log_handlers_num() | |||
There was a problem hiding this comment.
maybe ucs_log_num_handlers()?
| scoped_log_handler slh(hide_errors_logger); | ||
| ucs_status_t status = ucp_atomic_add64(e->ep(), 0, | ||
| (uintptr_t)memheap_addr + 1, rkey); | ||
| EXPECT_EQ(UCS_ERR_INVALID_PARAM, status); |
There was a problem hiding this comment.
looks like EXPECT_EQ should be outside the scope?
| scoped_log_handler slh(hide_errors_logger); | ||
| ucs_status_t status = test_ucp_atomic::ucp_atomic_post_nbi<uint64_t> | ||
| (e->ep(), OP, 0, (void *)((uintptr_t)memheap_addr + 1), rkey); | ||
| EXPECT_EQ(UCS_ERR_INVALID_PARAM, status); |
There was a problem hiding this comment.
same here, EXPECT_EQ outside the scope
| m_errors.clear(); | ||
|
|
||
| ucs_log_pop_handler(); |
There was a problem hiding this comment.
should we use RAII here too?
There was a problem hiding this comment.
I don't think that calling destructor implicitly is a RAII... more classes in inheritance to define ordering of destructors will only complicate the code. Do you know how to use it other way?
| m_errors.clear(); | ||
|
|
||
| ucs_log_pop_handler(); | ||
| if (m_num_log_handlers_before != ucs_log_handlers_num()) { | ||
| ADD_FAILURE() << "Missed log handler cleanup, " |
There was a problem hiding this comment.
unsigned num_not_removed = ucs_log_handlers_num() - m_num_log_handlers_before;
if (num_not_removed != 0) {
ADD_FAILURE() << num_not_removed << " log handlers were not removed";
}
- address CR comments to openucx#2958
What
Replace static API for log handlers set/clean up with
scoped_log_handlerclassWhy ?
Some tests potentially set up handlers and don't clean up them in case of exception, e.g.
ucs::test_skip_exception. Tests fail when they run in some magic sequence due to lack of log handlers slots and the following test can't handle expected error:How ?
Using RAII idiom