[test] add EXPECT_ENVOY_BUG (envoyproxy#14800)

* add EXPECT_ENVOY_BUG macro for testing

Signed-off-by: Asra Ali <[email protected]>

Author: asraa

source/common/common/assert.cc

@@ -53,7 +53,7 @@ class EnvoyBugRegistrationImpl : public ActionRegistration {
   // The implementation may also be a inline static counter per-file and line. There is no benchmark
   // to show that the performance of this mutex is any worse than atomic counters. Acquiring and
   // releasing a mutex is cheaper than a cache miss, but the mutex here is contended for every
-  // ENVOY_BUG failure rather than per individual bug. Logging ENVOY_BUGs is not a performance
+  // ENVOY_BUG failure rather than per individual bug. Hitting ENVOY_BUGs is not a performance
   // critical path, and mutex contention would indicate that there is a serious failure.
   // Currently, this choice reduces code size and has the advantage that behavior is easier to
   // understand and debug, and test behavior is predictable.
@@ -75,6 +75,13 @@ class EnvoyBugRegistrationImpl : public ActionRegistration {
     }
   }
 
+  static void resetEnvoyBugCounters() {
+    {
+      absl::MutexLock lock(&mutex_);
+      counters_.clear();
+    }
+  }
+
 private:
   // This implementation currently only handles one action being set at a time. This is currently
   // sufficient. If multiple actions are ever needed, the actions should be chained when
@@ -111,5 +118,7 @@ bool shouldLogAndInvokeEnvoyBugForEnvoyBugMacroUseOnly(absl::string_view bug_nam
   return EnvoyBugRegistrationImpl::shouldLogAndInvoke(bug_name);
 }
 
+void resetEnvoyBugCountersForTest() { EnvoyBugRegistrationImpl::resetEnvoyBugCounters(); }
+
 } // namespace Assert
 } // namespace Envoy

source/common/common/assert.h

@@ -71,6 +71,12 @@ void invokeEnvoyBugFailureRecordActionForEnvoyBugMacroUseOnly();
  */
 bool shouldLogAndInvokeEnvoyBugForEnvoyBugMacroUseOnly(absl::string_view bug_name);
 
+/**
+ * Resets all counters for EnvoyBugRegistrationImpl between tests.
+ *
+ */
+void resetEnvoyBugCountersForTest();
+
 // CONDITION_STR is needed to prevent macros in condition from being expected, which obfuscates
 // the logged failure, e.g., "EAGAIN" vs "11".
 #define _ASSERT_IMPL(CONDITION, CONDITION_STR, ACTION, DETAILS)                                    \

test/common/buffer/owned_impl_test.cc

@@ -7,6 +7,7 @@
 #include "test/mocks/api/mocks.h"
 #include "test/test_common/logging.h"
 #include "test/test_common/threadsafe_singleton_injector.h"
+#include "test/test_common/utility.h"
 
 #include "absl/strings/str_cat.h"
 #include "gmock/gmock.h"
@@ -913,16 +914,12 @@ TEST_F(OwnedImplTest, ReserveOverCommit) {
   auto reservation = buffer.reserveForRead();
   const auto reservation_length = reservation.length();
   const auto excess_length = reservation_length + 1;
-#ifdef NDEBUG
-  reservation.commit(excess_length);
-
-  // The length should be the Reservation length, not the value passed to commit.
-  EXPECT_EQ(reservation_length, buffer.length());
-#else
-  EXPECT_DEATH(
-      reservation.commit(excess_length),
-      "length <= length_. Details: commit\\(\\) length must be <= size of the Reservation");
-#endif
+  EXPECT_ENVOY_BUG(
+      {
+        reservation.commit(excess_length);
+        EXPECT_EQ(reservation_length, buffer.length());
+      },
+      "length <= length_. Details: commit() length must be <= size of the Reservation");
 }
 
 // Test behavior when the size to commit() is larger than the reservation.
@@ -931,16 +928,12 @@ TEST_F(OwnedImplTest, ReserveSingleOverCommit) {
   auto reservation = buffer.reserveSingleSlice(10);
   const auto reservation_length = reservation.length();
   const auto excess_length = reservation_length + 1;
-#ifdef NDEBUG
-  reservation.commit(excess_length);
-
-  // The length should be the Reservation length, not the value passed to commit.
-  EXPECT_EQ(reservation_length, buffer.length());
-#else
-  EXPECT_DEATH(
-      reservation.commit(excess_length),
-      "length <= slice_.len_. Details: commit\\(\\) length must be <= size of the Reservation");
-#endif
+  EXPECT_ENVOY_BUG(
+      {
+        reservation.commit(excess_length);
+        EXPECT_EQ(reservation_length, buffer.length());
+      },
+      "length <= slice_.len_. Details: commit() length must be <= size of the Reservation");
 }
 
 // Test functionality of the `freelist` (a performance optimization)

test/common/common/BUILD

@@ -26,6 +26,17 @@ envoy_cc_test(
     deps = [
         "//source/common/common:assert_lib",
         "//test/test_common:logging_lib",
+        "//test/test_common:utility_lib",
+    ],
+)
+
+envoy_cc_test(
+    name = "assert2_test",
+    srcs = ["assert2_test.cc"],
+    deps = [
+        "//source/common/common:assert_lib",
+        "//test/test_common:logging_lib",
+        "//test/test_common:utility_lib",
     ],
 )
 

test/common/common/assert2_test.cc

@@ -0,0 +1,19 @@
+#include "common/common/assert.h"
+
+#include "test/test_common/logging.h"
+#include "test/test_common/utility.h"
+
+#include "gtest/gtest.h"
+
+namespace Envoy {
+
+TEST(EnvoyBugDeathTest, TestResetCounters) {
+  // The callEnvoyBug counter has already been called in assert2_test.cc.
+  // ENVOY_BUG only log and increment stats on power of two cases. Ensure that counters are reset
+  // between tests by checking that two consecutive calls trigger the expectation.
+  for (int i = 0; i < 2; i++) {
+    EXPECT_ENVOY_BUG(TestEnvoyBug::callEnvoyBug(), "envoy bug failure: false.");
+  }
+}
+
+} // namespace Envoy

test/common/common/assert_test.cc

@@ -1,6 +1,7 @@
 #include "common/common/assert.h"
 
 #include "test/test_common/logging.h"
+#include "test/test_common/utility.h"
 
 #include "gtest/gtest.h"
 
@@ -47,16 +48,18 @@ TEST(EnvoyBugDeathTest, VariousLogs) {
   auto envoy_bug_action_registration =
       Assert::setEnvoyBugFailureRecordAction([&]() { envoy_bug_fail_count++; });
 
-#ifndef NDEBUG
-  EXPECT_DEATH({ ENVOY_BUG(false, ""); }, ".*envoy bug failure: false.*");
-  EXPECT_DEATH({ ENVOY_BUG(false, ""); }, ".*envoy bug failure: false.*");
-  EXPECT_DEATH({ ENVOY_BUG(false, "With some logs"); },
-               ".*envoy bug failure: false. Details: With some logs.*");
-  EXPECT_EQ(0, envoy_bug_fail_count);
-#else
-  // Same log lines trigger exponential back-off.
+  EXPECT_ENVOY_BUG({ ENVOY_BUG(false, ""); }, "envoy bug failure: false.");
+  EXPECT_ENVOY_BUG({ ENVOY_BUG(false, ""); }, "envoy bug failure: false.");
+  EXPECT_ENVOY_BUG({ ENVOY_BUG(false, "With some logs"); },
+                   "envoy bug failure: false. Details: With some logs");
+
+#ifdef NDEBUG
+  EXPECT_EQ(3, envoy_bug_fail_count);
+  // Reset envoy bug count to simplify testing exponential back-off below.
+  envoy_bug_fail_count = 0;
+  // In release mode, same log lines trigger exponential back-off.
   for (int i = 0; i < 4; i++) {
-    ENVOY_BUG(false, "");
+    ENVOY_BUG(false, "placeholder ENVOY_BUG");
   }
   // 3 counts because 1st, 2nd, and 4th instances are powers of 2.
   EXPECT_EQ(3, envoy_bug_fail_count);
@@ -69,4 +72,13 @@ TEST(EnvoyBugDeathTest, VariousLogs) {
 #endif
 }
 
+TEST(EnvoyBugDeathTest, TestResetCounters) {
+  // The callEnvoyBug counter has already been called in assert2_test.cc.
+  // ENVOY_BUG only log and increment stats on power of two cases. Ensure that counters are reset
+  // between tests by checking that two consecutive calls trigger the expectation.
+  for (int i = 0; i < 2; i++) {
+    EXPECT_ENVOY_BUG(TestEnvoyBug::callEnvoyBug(), "envoy bug failure: false.");
+  }
+}
+
 } // namespace Envoy

test/common/router/router_test.cc

@@ -5744,8 +5744,7 @@ TEST_F(RouterTest, InvalidUpstream) {
   Http::TestRequestHeaderMapImpl headers;
   HttpTestUtility::addDefaultHeaders(headers);
   headers.setMethod("CONNECT");
-  EXPECT_DEBUG_DEATH(router_.decodeHeaders(headers, false),
-                     "envoy bug failure: factory != nullptr.");
+  EXPECT_ENVOY_BUG(router_.decodeHeaders(headers, false), "envoy bug failure: factory != nullptr.");
 
   router_.onDestroy();
 }

test/extensions/transport_sockets/tls/utility_test.cc

@@ -164,11 +164,8 @@ TEST(UtilityTest, SslErrorDescriptionTest) {
     EXPECT_EQ(test_data.second, Utility::getErrorDescription(test_data.first));
   }
 
-#if defined(NDEBUG)
-  EXPECT_EQ(Utility::getErrorDescription(19), "UNKNOWN_ERROR");
-#else
-  EXPECT_DEATH(Utility::getErrorDescription(19), "Unknown BoringSSL error had occurred");
-#endif
+  EXPECT_ENVOY_BUG(EXPECT_EQ(Utility::getErrorDescription(19), "UNKNOWN_ERROR"),
+                   "Unknown BoringSSL error had occurred");
 }
 
 } // namespace

test/integration/protocol_integration_test.cc

@@ -302,24 +302,16 @@ TEST_P(ProtocolIntegrationTest, ContinueAfterLocalReply) {
 
   // Send a headers only request.
   IntegrationStreamDecoderPtr response;
-  const std::string error = "envoy bug failure: !state_.local_complete_ || status == "
-                            "FilterHeadersStatus::StopIteration. Details: Filters should return "
-                            "FilterHeadersStatus::StopIteration after sending a local reply.";
-#ifdef NDEBUG
-  EXPECT_LOG_CONTAINS("error", error, {
-    response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
-    response->waitForEndStream();
-  });
-  EXPECT_TRUE(response->complete());
-  EXPECT_EQ("200", response->headers().getStatusValue());
-#else
-  EXPECT_DEATH(
+  EXPECT_ENVOY_BUG(
       {
         response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
         response->waitForEndStream();
+        EXPECT_TRUE(response->complete());
+        EXPECT_EQ("200", response->headers().getStatusValue());
       },
-      HasSubstr(error));
-#endif
+      "envoy bug failure: !state_.local_complete_ || status == "
+      "FilterHeadersStatus::StopIteration. Details: Filters should return "
+      "FilterHeadersStatus::StopIteration after sending a local reply.");
 }
 
 TEST_P(ProtocolIntegrationTest, AddEncodedTrailers) {

test/test_common/BUILD

@@ -110,6 +110,7 @@ envoy_cc_test_library(
     ],
     deps = [
         ":file_system_for_test_lib",
+        ":logging_lib",
         ":printers_lib",
         ":resources_lib",
         ":test_time_lib",

test/test_common/utility.h

@@ -28,6 +28,7 @@
 #include "common/stats/symbol_table_impl.h"
 
 #include "test/test_common/file_system_for_test.h"
+#include "test/test_common/logging.h"
 #include "test/test_common/printers.h"
 #include "test/test_common/test_time_system.h"
 #include "test/test_common/thread_factory_for_test.h"
@@ -80,6 +81,16 @@ namespace Envoy {
   EXPECT_THAT_THROWS_MESSAGE(statement, expected_exception,                                        \
                              ::testing::Not(::testing::ContainsRegex(regex_str)))
 
+// Expect that the statement hits an ENVOY_BUG containing the specified message.
+#ifdef NDEBUG
+// ENVOY_BUGs in release mode log error.
+#define EXPECT_ENVOY_BUG(statement, message) EXPECT_LOG_CONTAINS("error", message, statement)
+#else
+// ENVOY_BUGs in debug mode is fatal.
+#define EXPECT_ENVOY_BUG(statement, message)                                                       \
+  EXPECT_DEBUG_DEATH(statement, ::testing::HasSubstr(message))
+#endif
+
 #define VERBOSE_EXPECT_NO_THROW(statement)                                                         \
   try {                                                                                            \
     statement;                                                                                     \
@@ -108,6 +119,11 @@ namespace Envoy {
 #define DEPRECATED_FEATURE_TEST(X) DISABLED_##X
 #endif
 
+class TestEnvoyBug {
+public:
+  static void callEnvoyBug() { ENVOY_BUG(false, ""); }
+};
+
 // Random number generator which logs its seed to stderr. To repeat a test run with a non-zero seed
 // one can run the test with --test_arg=--gtest_random_seed=[seed]
 class TestRandomGenerator {

test/test_runner.cc

@@ -153,6 +153,9 @@ int TestRunner::RunTests(int argc, char** argv) {
         TestEnvironment::getOptions().logPath(), access_log_manager, Logger::Registry::getSink());
   }
 
+  // Reset all ENVOY_BUG counters.
+  Envoy::Assert::resetEnvoyBugCountersForTest();
+
 #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
   // Fuzz tests may run Envoy tests in fuzzing mode to generate corpora. In this case, we do not
   // want to fail building the fuzz test because of a failed test run, which can happen when testing