This is an automated email from the ASF dual-hosted git repository.
wangdan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pegasus.git
The following commit(s) were added to refs/heads/master by this push:
new 18e8aeb74 test: enable validation logic in debug-only code to run
during unit tests (#2262)
18e8aeb74 is described below
commit 18e8aeb744228a01247fe3970e375785ddcda4db
Author: Dan Wang <[email protected]>
AuthorDate: Tue Jul 1 19:01:07 2025 +0800
test: enable validation logic in debug-only code to run during unit tests
(#2262)
In Pegasus, unit tests are not necessarily executed in debug mode (i.e.,
with the
`NDEBUG` macro undefined). For example, the GitHub CI workflow builds and
runs unit tests in release mode. As a result, certain test-related code
paths that
are only enabled in debug builds are not actually executed during CI
testing.
To address this, we now allow such debug-only validation logic to be
compiled
and executed when building test code (i.e., executing `./run.sh build
--test` where
`--test` defines the `MOCK_TEST` macro). This ensures that debug-mode test
logic is included and exercised during unit test runs, improving test
coverage in
CI and other environments.
---
src/meta/server_state.cpp | 7 ++--
src/rpc/dsn_message_parser.cpp | 79 ++++++++++++++++++++++--------------------
src/server/result_writer.cpp | 1 -
src/task/task_worker.cpp | 10 +++---
src/utils/alloc.h | 9 +++--
src/utils/blob.h | 8 +++--
src/utils/errors.h | 2 +-
src/utils/fmt_logging.h | 5 +++
src/utils/test/blob_test.cpp | 4 ---
9 files changed, 70 insertions(+), 55 deletions(-)
diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp
index 1b1257a88..4e504ff24 100644
--- a/src/meta/server_state.cpp
+++ b/src/meta/server_state.cpp
@@ -1829,9 +1829,11 @@ void server_state::update_configuration_locally(
ns = get_node_state(_nodes, node, false);
CHECK_NOTNULL(ns, "invalid node: {}", node);
}
-#ifndef NDEBUG
+
+#if defined(MOCK_TEST) || !defined(NDEBUG)
request_check(old_pc, *config_request);
#endif
+
switch (config_request->type) {
case config_type::CT_ASSIGN_PRIMARY:
case config_type::CT_UPGRADE_TO_PRIMARY:
@@ -1940,9 +1942,10 @@ void server_state::update_configuration_locally(
boost::lexical_cast<std::string>(*config_request));
}
-#ifndef NDEBUG
+#if defined(MOCK_TEST) || !defined(NDEBUG)
check_consistency(gpid);
#endif
+
if (_config_change_subscriber) {
_config_change_subscriber(_all_apps);
}
diff --git a/src/rpc/dsn_message_parser.cpp b/src/rpc/dsn_message_parser.cpp
index 178a60273..d475318b5 100644
--- a/src/rpc/dsn_message_parser.cpp
+++ b/src/rpc/dsn_message_parser.cpp
@@ -26,9 +26,10 @@
#include "dsn_message_parser.h"
-#include <stddef.h>
-#include <stdint.h>
+#include <cstddef>
+#include <cstdint>
#include <memory>
+#include <numeric>
#include <vector>
#include "rpc/rpc_message.h"
@@ -102,50 +103,52 @@ message_ex
*dsn_message_parser::get_message_on_receive(message_reader *reader,
void dsn_message_parser::prepare_on_send(message_ex *msg)
{
auto &header = msg->header;
- auto &buffers = msg->buffers;
-
-#ifndef NDEBUG
- int i_max = (int)buffers.size() - 1;
- size_t len = 0;
- for (int i = 0; i <= i_max; i++) {
- len += (size_t)buffers[i].length();
+ const auto &buffers = msg->buffers;
+
+#if defined(MOCK_TEST) || !defined(NDEBUG)
+ {
+ const size_t len =
+ std::accumulate(buffers.begin(), buffers.end(), 0UL, [](size_t
sum, const auto &buf) {
+ return sum + buf.length();
+ });
+ CHECK_EQ(len, header->body_length + sizeof(message_header));
}
- CHECK_EQ(len, header->body_length + sizeof(message_header));
#endif
- if (task_spec::get(msg->local_rpc_code)->rpc_message_crc_required) {
- // compute data crc if necessary (only once for the first time)
- if (header->body_crc32 == CRC_INVALID) {
- int i_max = (int)buffers.size() - 1;
- uint32_t crc32 = 0;
- size_t len = 0;
- for (int i = 0; i <= i_max; i++) {
- uint32_t lcrc;
- const void *ptr;
- size_t sz;
-
- if (i == 0) {
- ptr = (const void *)(buffers[i].data() +
sizeof(message_header));
- sz = (size_t)buffers[i].length() - sizeof(message_header);
- } else {
- ptr = (const void *)buffers[i].data();
- sz = (size_t)buffers[i].length();
- }
-
- lcrc = dsn::utils::crc32_calc(ptr, sz, crc32);
- crc32 = dsn::utils::crc32_concat(0, 0, crc32, len, crc32,
lcrc, sz);
-
- len += sz;
+ if (!task_spec::get(msg->local_rpc_code)->rpc_message_crc_required) {
+ return;
+ }
+
+ // compute data crc if necessary (only once for the first time)
+ if (header->body_crc32 == CRC_INVALID) {
+ uint32_t crc32 = 0;
+ size_t len = 0;
+
+ for (size_t i = 0; i < buffers.size(); ++i) {
+ const auto &buf = buffers[i];
+ const char *ptr = buf.data();
+ size_t sz = buf.length();
+
+ if (i == 0) {
+ ptr += sizeof(message_header);
+
+ // TODO(wangdan): CHECK_GE(sz, sizeof(message_header)) ?
+ sz -= sizeof(message_header);
}
- CHECK_EQ(len, header->body_length);
- header->body_crc32 = crc32;
+ const auto lcrc = dsn::utils::crc32_calc(ptr, sz, crc32);
+ crc32 = dsn::utils::crc32_concat(0, 0, crc32, len, crc32, lcrc,
sz);
+
+ len += sz;
}
- // always compute header crc
- header->hdr_crc32 = CRC_INVALID;
- header->hdr_crc32 = dsn::utils::crc32_calc(header,
sizeof(message_header), 0);
+ CHECK_EQ(len, header->body_length);
+ header->body_crc32 = crc32;
}
+
+ // always compute header crc
+ header->hdr_crc32 = CRC_INVALID;
+ header->hdr_crc32 = dsn::utils::crc32_calc(header, sizeof(message_header),
0);
}
int dsn_message_parser::get_buffers_on_send(message_ex *msg, /*out*/ send_buf
*buffers)
diff --git a/src/server/result_writer.cpp b/src/server/result_writer.cpp
index df78172e9..4d3a9160e 100644
--- a/src/server/result_writer.cpp
+++ b/src/server/result_writer.cpp
@@ -21,7 +21,6 @@
#include <pegasus/error.h>
#include <chrono>
-#include <type_traits>
#include <utility>
#include "pegasus/client.h"
diff --git a/src/task/task_worker.cpp b/src/task/task_worker.cpp
index 01fec9887..d5606d2a8 100644
--- a/src/task/task_worker.cpp
+++ b/src/task/task_worker.cpp
@@ -236,20 +236,22 @@ void task_worker::loop()
q->decrease_count(batch_size);
-#ifndef NDEBUG
+#if defined(MOCK_TEST) || !defined(NDEBUG)
int count = 0;
#endif
+
while (task != nullptr) {
next = task->next;
task->next = nullptr;
task->exec_internal();
task = next;
-#ifndef NDEBUG
- count++;
+
+#if defined(MOCK_TEST) || !defined(NDEBUG)
+ ++count;
#endif
}
-#ifndef NDEBUG
+#if defined(MOCK_TEST) || !defined(NDEBUG)
CHECK_EQ_MSG(count, batch_size, "returned task count and batch size do
not match");
#endif
diff --git a/src/utils/alloc.h b/src/utils/alloc.h
index fdb799117..fa078cd8c 100644
--- a/src/utils/alloc.h
+++ b/src/utils/alloc.h
@@ -30,8 +30,13 @@
// where CACHELINE_SIZE is defined.
#ifdef CACHELINE_SIZE
-#ifndef NDEBUG
+#if defined(MOCK_TEST) || !defined(NDEBUG)
+
+#include <cstdint>
+#include <fmt/format.h>
+
#include "utils/fmt_logging.h"
+
#endif
namespace dsn {
@@ -53,7 +58,7 @@ cacheline_aligned_ptr<T> cacheline_aligned_alloc_array(size_t
len)
T *array = new (buffer) T[len];
-#ifndef NDEBUG
+#if defined(MOCK_TEST) || !defined(NDEBUG)
if (sizeof(T) <= CACHELINE_SIZE && (sizeof(T) & (sizeof(T) - 1)) == 0) {
for (size_t i = 0; i < len; ++i) {
T *elem = &(array[i]);
diff --git a/src/utils/blob.h b/src/utils/blob.h
index c1212afb4..d07ac7df5 100644
--- a/src/utils/blob.h
+++ b/src/utils/blob.h
@@ -114,9 +114,11 @@ public:
/// NOTE: this operation is not efficient since it involves a memory copy.
[[nodiscard]] static blob create_from_bytes(const char *s, size_t len)
{
- DCHECK(s != nullptr || len == 0,
- "null source pointer with non-zero length would lead to "
- "undefined behaviour");
+#if defined(MOCK_TEST) || !defined(NDEBUG)
+ CHECK(s != nullptr || len == 0,
+ "null source pointer with non-zero length would lead to "
+ "undefined behaviour");
+#endif
std::shared_ptr<char> s_arr(new char[len],
std::default_delete<char[]>());
memcpy(s_arr.get(), s, len);
diff --git a/src/utils/errors.h b/src/utils/errors.h
index d262f2241..36b99d070 100644
--- a/src/utils/errors.h
+++ b/src/utils/errors.h
@@ -294,7 +294,7 @@ USER_DEFINED_STRUCTURE_FORMATTER(::dsn::error_s);
}
\
} while (false)
-#ifndef NDEBUG
+#if defined(MOCK_TEST) || !defined(NDEBUG)
#define DCHECK_OK CHECK_OK
#else
#define DCHECK_OK(s, ...)
diff --git a/src/utils/fmt_logging.h b/src/utils/fmt_logging.h
index 97e751a48..6360bbfb9 100644
--- a/src/utils/fmt_logging.h
+++ b/src/utils/fmt_logging.h
@@ -324,6 +324,11 @@ inline const char *null_str_printer(const char *s) {
return s == nullptr ? "(nul
}
\
} while (0)
+// TODO(wangdan): currently enable `DCHECK` for test code (via `MOCK_TEST`)
causes many unit
+// tests to fail; it will be enabled after the issues are resolved.
+//
+// After resolved, enable `DCHECK` for test code by:
+// #if defined(MOCK_TEST) || !defined(NDEBUG)
#ifndef NDEBUG
#define DCHECK CHECK
#define DCHECK_NOTNULL CHECK_NOTNULL
diff --git a/src/utils/test/blob_test.cpp b/src/utils/test/blob_test.cpp
index 586a7d68d..37a7f502d 100644
--- a/src/utils/test/blob_test.cpp
+++ b/src/utils/test/blob_test.cpp
@@ -32,8 +32,6 @@ TEST(BlobTest, CreateFromZeroLengthNullptr)
EXPECT_EQ(0, obj.size());
}
-#ifndef NDEBUG
-
TEST(BlobTest, CreateFromNonZeroLengthNullptr)
{
ASSERT_DEATH({ const auto &obj = blob::create_from_bytes(nullptr, 1); },
@@ -41,8 +39,6 @@ TEST(BlobTest, CreateFromNonZeroLengthNullptr)
"undefined behaviour");
}
-#endif
-
struct blob_base_case
{
std::string expected_str;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
