Dive communication protocol: Unit tests for client and unix domain server by elviscapiaq · Pull Request #357 · google/dive

elviscapiaq · 2025-09-08T19:16:41Z

No description provided.

elviscapiaq · 2025-09-08T19:28:55Z

Please take a look of this CL. I want your opinion to decide if we add unit tests in this way or maybe using something else like Google Mock?

elviscapiaq · 2025-09-10T20:27:40Z

@wangra-google , @footballhead, I fixed the issue for Linux, now you can take a look of this commit.

footballhead · 2025-09-11T17:53:42Z

At a high-level, I think this is a good approach since we should strive to write tests using the real implementation. (Google SWE Handbook, chapter 18.e.i) The main arguments against mocking from that chapter are:

Using the real implementation provides us with more confidence that the code we are shipping to users is reliable.
When the real implementation changes, so too can the mocks. This means mocks add additional maintenance overhead. This can also make refactoring harder.

Chapter 18: Test Doubles goes into options that you can consider if testing the real implementation is not feasible. In general, they encourage looking at other options first instead of mocks, e.g. fakes. TestTcpServer is a good example of a fake.

I'd encourage reading Chapters 16-19 of the SWE handbook

network/tcp_client_test.cc

…rver

elviscapiaq · 2025-11-07T20:25:16Z

At a high-level, I think this is a good approach since we should strive to write tests using the real implementation. (Google SWE Handbook, chapter 18.e.i) The main arguments against mocking from that chapter are:

Using the real implementation provides us with more confidence that the code we are shipping to users is reliable.

When the real implementation changes, so too can the mocks. This means mocks add additional maintenance overhead. This can also make refactoring harder.

Chapter 18: Test Doubles goes into options that you can consider if testing the real implementation is not feasible. In general, they encourage looking at other options first instead of mocks, e.g. fakes. TestTcpServer is a good example of a fake.

I'd encourage reading Chapters 16-19 of the SWE handbook

Thanks for poiting it out. I have read all these good practices and made a commit based on that.
Now we are faking the TestTcpServer as this is the recommended approach for this case.

elviscapiaq · 2025-11-07T20:26:37Z

I am restarting this commit applying all the recommendations and after reading the tips about how to use a fake test. I found it very interesting and useful

elviscapiaq · 2025-11-11T16:10:41Z

I fixed the issue as the server and client tests are only for Linux OS.
Also, move the PR from draft to for review.

network/CMakeLists.txt

network/unix_domain_server_test.cc

network/socket_connection.h

elviscapiaq · 2025-11-26T19:00:48Z

I addressed the feedback, ready for review.

network/socket_connection.cc

network/tcp_client_test.cc

elviscapiaq · 2025-12-10T17:48:28Z

Ready for review!

network/tcp_client_test.cc

network/fake_socket_connection.h

footballhead · 2025-12-19T22:09:18Z

network/unix_domain_server_test.cc

+    /// Connects to an Unix (Local) Domain with abstract namespace.
+    sockaddr_un addr = {};
+    addr.sun_family = AF_UNIX;
+    // first char is '\0'


Let the code speak for itself. Comments should focus on the why, not the what. The what should be obvious from reading the code. If it's not then that's a good sign that it should be refactored

Suggested change

// first char is '\0'

footballhead · 2025-12-19T22:29:11Z

network/unix_domain_server_test.cc

+    addr.sun_path[0] = '\0';
+    strncpy(addr.sun_path + 1, kTestServerAddress, std::size(kTestServerAddress));
+    if (connect(client_socket,
+                (sockaddr*)&addr,


Prefer C++ casts since they are more explicit https://google.github.io/styleguide/cppguide.html#Casting

footballhead · 2025-12-19T22:38:06Z

network/unix_domain_server_test.cc

+    ASSERT_TRUE(server->Start(kTestServerAddress).ok());
+
+    absl::StatusOr<std::unique_ptr<SocketConnection>> client_conn = ConnectClient();
+    ASSERT_TRUE(client_conn.ok());


Prefer using the absl_testing::IsOk() matcher since it will print more information if something fails

Suggested change

ASSERT_TRUE(client_conn.ok());

#include "absl/status/status_matchers.h"

using ::absl_testing::IsOk;

// ...

ASSERT_THAT(client_conn.ok(), IsOk());

footballhead · 2025-12-19T22:38:30Z

network/unix_domain_server_test.cc

+TEST(UnixDomainServerTest, HandShakeSuccess)
+{
+    auto server = std::make_unique<UnixDomainServer>();
+    ASSERT_TRUE(server->Start(kTestServerAddress).ok());


Prefer using the absl_testing::IsOk() matcher since it will print more information if something fails

footballhead · 2025-12-19T22:38:35Z

network/unix_domain_server_test.cc

+    ASSERT_TRUE(server->Start(kTestServerAddress).ok());
+
+    absl::StatusOr<std::unique_ptr<SocketConnection>> client_conn = ConnectClient();
+    ASSERT_TRUE(client_conn.ok());


Prefer using the absl_testing::IsOk() matcher since it will print more information if something fails

footballhead · 2025-12-19T22:38:44Z

network/unix_domain_server_test.cc

+    HandshakeRequest request;
+    request.SetMajorVersion(5);
+    request.SetMinorVersion(2);
+    ASSERT_TRUE(SendSocketMessage((*client_conn).get(), request).ok());


Prefer using the absl_testing::IsOk() matcher since it will print more information if something fails

footballhead · 2025-12-19T22:38:52Z

network/unix_domain_server_test.cc

+    // Client receives response.
+    absl::StatusOr<std::unique_ptr<ISerializable>> response_msg = ReceiveSocketMessage(
+    (*client_conn).get());
+    ASSERT_TRUE(response_msg.ok());


Prefer using the absl_testing::IsOk() matcher since it will print more information if something fails

footballhead · 2025-12-19T22:40:09Z

network/unix_domain_server_test.cc

+    ASSERT_EQ((*response_msg)->GetMessageType(), MessageType::HANDSHAKE_RESPONSE);
+    auto* response = dynamic_cast<HandshakeResponse*>((*response_msg).get());
+    ASSERT_NE(response, nullptr);
+    EXPECT_EQ(response->GetMajorVersion(), request.GetMajorVersion());


Prefer a custom matcher here

elviscapiaq requested review from footballhead and wangra-google September 8, 2025 19:27

elviscapiaq force-pushed the unit_tests branch 3 times, most recently from 6df7e26 to 010ed76 Compare September 10, 2025 19:24

footballhead reviewed Sep 12, 2025

View reviewed changes

network/tcp_client_test.cc Outdated Show resolved Hide resolved

elviscapiaq added 2 commits November 7, 2025 18:34

Dive communication protocol: Unit tests for client and unix domain se…

2e4310a

…rver

Create a Fake TestTcpServer

136e942

elviscapiaq force-pushed the unit_tests branch from 010ed76 to 136e942 Compare November 7, 2025 20:23

elviscapiaq requested review from angela28chen and footballhead November 7, 2025 20:25

Make server and client tests only for Linux

1f7c88b

elviscapiaq marked this pull request as ready for review November 11, 2025 16:10

footballhead reviewed Nov 11, 2025

View reviewed changes

network/CMakeLists.txt Show resolved Hide resolved

footballhead reviewed Nov 12, 2025

View reviewed changes

network/unix_domain_server_test.cc Outdated Show resolved Hide resolved

elviscapiaq and others added 3 commits November 13, 2025 18:57

Fix test fistures + client/server OS targets

9a151e8

Fix FakeSocketConnection

bba3b48

Merge branch 'main' into unit_tests

a6617f7

footballhead self-requested a review November 18, 2025 17:37

footballhead reviewed Nov 18, 2025

View reviewed changes

network/unix_domain_server_test.cc Outdated Show resolved Hide resolved

network/unix_domain_server_test.cc Outdated Show resolved Hide resolved

footballhead reviewed Nov 19, 2025

View reviewed changes

network/unix_domain_server_test.cc Outdated Show resolved Hide resolved

network/unix_domain_server_test.cc Outdated Show resolved Hide resolved

network/unix_domain_server_test.cc Outdated Show resolved Hide resolved

footballhead reviewed Nov 20, 2025

View reviewed changes

network/socket_connection.h Outdated Show resolved Hide resolved

network/socket_connection.h Outdated Show resolved Hide resolved

Create an interface for SocketCOnnection + fix unit tests

c84e3a2

footballhead reviewed Nov 26, 2025

View reviewed changes

elviscapiaq force-pushed the unit_tests branch 3 times, most recently from 72e8e1b to 8e433ae Compare December 10, 2025 17:40

elviscapiaq requested a review from footballhead December 10, 2025 17:48

Fix race conditions + logical error

dd86539

elviscapiaq force-pushed the unit_tests branch from 8e433ae to dd86539 Compare December 10, 2025 17:50

wangra-google previously approved these changes Dec 12, 2025

View reviewed changes

footballhead reviewed Dec 12, 2025

View reviewed changes

network/tcp_client_test.cc Outdated Show resolved Hide resolved

network/fake_socket_connection.h Outdated Show resolved Hide resolved

Adds IgnoreError to unused absl::Status

07b8821

elviscapiaq dismissed wangra-google’s stale review via 07b8821 December 12, 2025 19:08

elviscapiaq requested review from footballhead and wangra-google December 12, 2025 19:10

Merge branch 'main' into unit_tests

14084d7

wangra-google previously approved these changes Dec 12, 2025

View reviewed changes

elviscapiaq dismissed wangra-google’s stale review via fc3a2c3 December 12, 2025 20:11

elviscapiaq force-pushed the unit_tests branch from fc3a2c3 to 2e44e6a Compare December 12, 2025 20:48

wangra-google previously approved these changes Dec 12, 2025

View reviewed changes

Fix dive_status dependencies + unixdomainserver tests

6859a0d

elviscapiaq dismissed wangra-google’s stale review via 6859a0d December 12, 2025 20:58

elviscapiaq force-pushed the unit_tests branch from 2e44e6a to 6859a0d Compare December 12, 2025 20:58

elviscapiaq requested a review from wangra-google December 12, 2025 21:28

wangra-google approved these changes Dec 12, 2025

View reviewed changes

Merge branch 'main' into unit_tests

e8b0959

footballhead reviewed Dec 19, 2025

View reviewed changes

-    ASSERT_TRUE(client_conn.ok());
+#include "absl/status/status_matchers.h"
+using ::absl_testing::IsOk;
+// ...
+    ASSERT_THAT(client_conn.ok(), IsOk());

Conversation

elviscapiaq commented Sep 8, 2025

Uh oh!

elviscapiaq commented Sep 8, 2025

Uh oh!

elviscapiaq commented Sep 10, 2025

Uh oh!

footballhead commented Sep 11, 2025

Uh oh!

Uh oh!

elviscapiaq commented Nov 7, 2025

Uh oh!

elviscapiaq commented Nov 7, 2025

Uh oh!

elviscapiaq commented Nov 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elviscapiaq commented Nov 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elviscapiaq commented Dec 10, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants