[kudu] 01/02: maintenance_manager-test: fix cascading CHECK failure

[ad...@apache.org]

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 <ad...@cloudera.com>
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 <as...@cloudera.com>
    Tested-by: Kudu Jenkins
    Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
 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)