----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28415/#review63016 -----------------------------------------------------------
3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105191> { // Scope for socket. 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105193> When using gtest/gmock always put the expected value first and the actual value second since that is how it prints out when tests fail. Also, feel free to use less temporaries when you don't need them: ASSERT_GT(0, write(s, data, 5)); 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105190> EXPECT_SOME_TRUE(os::isNonblock(socket.get())); 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105195> EXPECT_SOME_TRUE(os::isCloexec(socket.get())); 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105264> This doesn't look used. 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105263> AWAIT_ASSERT_FAILED(socket.accept()); 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105262> We try not to use out parameters in the codebase. And for this case we should instead turn this into test set up in a test fixture anyway: class ServerSocketTest : public ::testing::Test { protected: virtual void SetUp() { ASSERT_SOME(socket.bind(node)); node = bind.get(); ASSERT_SOME(socket.listen(5)); } Node node; Socket socket; }; Then the 'connect' test below can just be: TEST_F(ServerSocketTest, connect) { Socket clientSocket; AWAIT_EXPECT_READY(clientSocket.connect(node)); } Also, using 'node' as the default bind is weird, we should really have a Node::LOCALHOST constant that we can use instead! Although honestly for both the 'connect' and 'connectFail' tests we could just do this inline, it's not that much code: TEST(Socket, connect) { Socket serverSocket; Try<Node> bind = serverSocket.bind(Node::LOCALHOST); ASSERT_SOME(bind); ASSERT_SOME(serverSocket.listen(5)); Socket clientSocket; AWAIT_EXPECT_READY(clientSocket.connect(bind.get())); } One suggestion until we get something like Node::LOCALHOST is to make a test fixture that includes that. class SocketTest : public ::testing::Test { protected: SocketTest() : LOCALHOST(...) {} const Node LOCALHOST; }; Then in tests: TEST_F(SocketTest, connect) { ... LOCALHOST ... } 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105265> Another great candidate for a test fixture: class ServerClientSocketTest : public ::testing::Test { protected: virtual void SetUp() { ... see comment above ... } Node serverNode; Node clientNode; Socket serverSocket; Socket clientSocket; }; Then in the tests below you can just do: TEST_F(ServerClientSocketTest, send) { const string data = "Hi!"; AWAIT_ASSERT_EQ(3, clientSocket.send(data.data(), data.size())); // ... and you could even combine the 'read' in this test as well! } Also, from looking at this code now it becomes really obvious that we should have a 'Option<Node> node()' method on Socket that returns either the local node that we've bound to if we did a 'bind' or the remote node that we've connected to if we did a 'connect' or none if we've neither done a 'bind' or a 'connect'. The other thing that jumps out is that we should really s/read/recv/ in Socket to keep consistency with using 'send' instead of 'write'. 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105266> To write this test without relying on scoping it becomes really clear that we need a Socket::close!!!! 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105236> { // Scope for life of client socket. 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105235> AWAIT_EXPECT_EQ(0, socket.read(buf, 4)); 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105231> { on newline please. 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105233> AWAIT_ASSERT_EQ(data.size(), sendLength); 3rdparty/libprocess/src/tests/socket_tests.cpp <https://reviews.apache.org/r/28415/#comment105232> AWAIT_ASSERT_EQ(data.size(), readLength); - Benjamin Hindman On Nov. 24, 2014, 10:15 p.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28415/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2014, 10:15 p.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Bugs: MESOS-2119 > https://issues.apache.org/jira/browse/MESOS-2119 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am aebd281da41325a0246f5c57780928afc22a1baa > 3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/28415/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Joris Van Remoortere > >
