Copilot commented on code in PR #3626:
URL: https://github.com/apache/brpc/pull/3626#discussion_r4314190321
##########
test/brpc_urma_unittest.cpp:
##########
@@ -41,6 +43,91 @@ extern butil::atomic<bool> g_urma_available;
} // namespace urma
} // namespace brpc
+class UrmaSendCompletionTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+ SocketOptions options;
+ SocketId id;
+ ASSERT_EQ(0, Socket::Create(options, &id));
+ ASSERT_EQ(0, Socket::Address(id, &_socket));
+ }
+
+ void TearDown() override {
+ if (_socket) {
+ _socket->SetFailed();
+ }
+ }
+
+ static urma_cr_t SendCompletion() {
+ urma_cr_t cr{};
+ cr.status = URMA_CR_SUCCESS;
+ cr.user_ctx = 1;
+ return cr;
+ }
+
+ SocketUniquePtr _socket;
+};
+
+TEST_F(UrmaSendCompletionTest, reclaim_buffer_before_publishing_credit) {
+ char payload = 'x';
+ bool released = false;
+ urma::UrmaEndpoint endpoint(_socket.get());
+ endpoint._sbuf.resize(endpoint._sq_size -
+ urma::UrmaEndpoint::RESERVED_WR_NUM);
+ endpoint._local_window_capacity = endpoint._sbuf.size();
+ endpoint._sq_window_size.store(0, butil::memory_order_relaxed);
+ ASSERT_EQ(0, endpoint._sbuf[0].append_user_data(
+ &payload, sizeof(payload), [&](void*) {
+ released = true;
+ // Observe the credit at the exact point the old buffer is freed.
+ // Publishing first lets the writer reuse a slot still being
cleared.
+ EXPECT_EQ(0, endpoint._sq_window_size.load(
+ butil::memory_order_relaxed));
+ }));
Review Comment:
These new tests reach deeply into `UrmaEndpoint` internals (`_sbuf`,
`_sq_size`, `_sq_sent`, `_sq_window_size`, `_local_window_capacity`). This
makes the tests brittle to refactors (even when behavior remains correct) and
can force production encapsulation changes. Prefer granting access via
`FRIEND_TEST(...)` declarations (or a small test-only accessor API) so the
class can keep members private while still supporting deterministic validation
of reclamation order/cursor behavior.
##########
test/brpc_urma_unittest.cpp:
##########
@@ -41,6 +43,91 @@ extern butil::atomic<bool> g_urma_available;
} // namespace urma
} // namespace brpc
+class UrmaSendCompletionTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+ SocketOptions options;
+ SocketId id;
+ ASSERT_EQ(0, Socket::Create(options, &id));
+ ASSERT_EQ(0, Socket::Address(id, &_socket));
+ }
+
+ void TearDown() override {
+ if (_socket) {
+ _socket->SetFailed();
+ }
+ }
+
+ static urma_cr_t SendCompletion() {
+ urma_cr_t cr{};
+ cr.status = URMA_CR_SUCCESS;
+ cr.user_ctx = 1;
+ return cr;
+ }
+
+ SocketUniquePtr _socket;
+};
+
+TEST_F(UrmaSendCompletionTest, reclaim_buffer_before_publishing_credit) {
+ char payload = 'x';
+ bool released = false;
+ urma::UrmaEndpoint endpoint(_socket.get());
+ endpoint._sbuf.resize(endpoint._sq_size -
+ urma::UrmaEndpoint::RESERVED_WR_NUM);
+ endpoint._local_window_capacity = endpoint._sbuf.size();
+ endpoint._sq_window_size.store(0, butil::memory_order_relaxed);
+ ASSERT_EQ(0, endpoint._sbuf[0].append_user_data(
+ &payload, sizeof(payload), [&](void*) {
+ released = true;
+ // Observe the credit at the exact point the old buffer is freed.
+ // Publishing first lets the writer reuse a slot still being
cleared.
+ EXPECT_EQ(0, endpoint._sq_window_size.load(
+ butil::memory_order_relaxed));
+ }));
+
+ ASSERT_EQ(0, endpoint.HandleCompletion(SendCompletion()));
+ EXPECT_TRUE(released);
+ EXPECT_TRUE(endpoint._sbuf[0].empty());
+ EXPECT_EQ(1, endpoint._sq_sent);
Review Comment:
These new tests reach deeply into `UrmaEndpoint` internals (`_sbuf`,
`_sq_size`, `_sq_sent`, `_sq_window_size`, `_local_window_capacity`). This
makes the tests brittle to refactors (even when behavior remains correct) and
can force production encapsulation changes. Prefer granting access via
`FRIEND_TEST(...)` declarations (or a small test-only accessor API) so the
class can keep members private while still supporting deterministic validation
of reclamation order/cursor behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
