This is an automated email from the ASF dual-hosted git repository. adar pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit d5fab3e808357ff7f7af77934cc5ff0b641f35ab Author: Adar Dembo <[email protected]> AuthorDate: Mon Jan 13 12:05:14 2020 -0800 maintenance_manager-test: fix cascading CHECK failure While trying to repro test flakiness, I saw that ASSERT_EVENTUALLY doesn't handle --gtest_throw_on_failure correctly. And that also highlighted an issue with the test where an ASSERT firing early could lead to a second CHECK failure, obscuring the actual test failure. Change-Id: I7a8ed5bedbcdb742bebe56327ffdbb885eef28b0 Reviewed-on: http://gerrit.cloudera.org:8080/15024 Reviewed-by: Alexey Serbin <[email protected]> Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong <[email protected]> --- src/kudu/util/maintenance_manager-test.cc | 22 +++++++++++++++------- src/kudu/util/test_util.cc | 12 +++++++----- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/kudu/util/maintenance_manager-test.cc b/src/kudu/util/maintenance_manager-test.cc index 4341351..c345d5c 100644 --- a/src/kudu/util/maintenance_manager-test.cc +++ b/src/kudu/util/maintenance_manager-test.cc @@ -40,6 +40,7 @@ #include "kudu/util/metrics.h" #include "kudu/util/monotime.h" #include "kudu/util/mutex.h" +#include "kudu/util/scoped_cleanup.h" #include "kudu/util/status.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" @@ -524,19 +525,26 @@ TEST_F(MaintenanceManagerTest, TestPriorityOpLaunch) { manager_->RegisterOp(&op5); manager_->RegisterOp(&op6); + // From this point forward if an ASSERT fires, we'll hit a CHECK failure if + // we don't unregister an op before it goes out of scope. + SCOPED_CLEANUP({ + manager_->UnregisterOp(&op1); + manager_->UnregisterOp(&op2); + manager_->UnregisterOp(&op3); + manager_->UnregisterOp(&op4); + manager_->UnregisterOp(&op5); + manager_->UnregisterOp(&op6); + }); + ASSERT_EVENTUALLY([&]() { MaintenanceManagerStatusPB status_pb; manager_->GetMaintenanceManagerStatusDump(&status_pb); ASSERT_EQ(status_pb.completed_operations_size(), 6); }); - // Wait for instances to complete. - manager_->UnregisterOp(&op1); - manager_->UnregisterOp(&op2); - manager_->UnregisterOp(&op3); - manager_->UnregisterOp(&op4); - manager_->UnregisterOp(&op5); - manager_->UnregisterOp(&op6); + // Wait for instances to complete by shutting down the maintenance manager. + // We can still call GetMaintenanceManagerStatusDump though. + StopManager(); // Check that running instances are removed from collection after completion. MaintenanceManagerStatusPB status_pb; diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc index ec9e23b..93eac65 100644 --- a/src/kudu/util/test_util.cc +++ b/src/kudu/util/test_util.cc @@ -38,7 +38,6 @@ #include <boost/optional/optional.hpp> #include <gflags/gflags.h> -#include <gflags/gflags_declare.h> #include <glog/logging.h> #include <gtest/gtest-spi.h> @@ -290,14 +289,17 @@ void AssertEventually(const std::function<void(void)>& f, AssertBackoff backoff) { const MonoTime deadline = MonoTime::Now() + timeout; { - // Disable --gtest_break_on_failure, or else the assertion failures - // inside our attempts will cause the test to SEGV even though we - // would like to retry. + // Disable gtest's "on failure" behavior, or else the assertion failures + // inside our attempts will cause the test to end even though we would + // like to retry. bool old_break_on_failure = testing::FLAGS_gtest_break_on_failure; - auto c = MakeScopedCleanup([old_break_on_failure]() { + bool old_throw_on_failure = testing::FLAGS_gtest_throw_on_failure; + auto c = MakeScopedCleanup([old_break_on_failure, old_throw_on_failure]() { testing::FLAGS_gtest_break_on_failure = old_break_on_failure; + testing::FLAGS_gtest_throw_on_failure = old_throw_on_failure; }); testing::FLAGS_gtest_break_on_failure = false; + testing::FLAGS_gtest_throw_on_failure = false; for (int attempts = 0; MonoTime::Now() < deadline; attempts++) { // Capture any assertion failures within this scope (i.e. from their function)
