Fix Process unittests.

am: 992e42977b

* commit '992e42977bb39c34360e5224a85115a1a9f834dc':
  Fix Process unittests.

5 files changed, 112 insertions, 91 deletions

diff --git a/brillo/message_loops/message_loop_unittest.cc b/brillo/message_loops/message_loop_unittest.cc
index 20fb6be..7cc2a30 100644
--- a/brillo/message_loops/message_loop_unittest.cc
+++ b/brillo/message_loops/message_loop_unittest.cc
@@ -9,11 +9,6 @@
 // implementation-specific tests see the particular implementation unittests in
 // the *_unittest.cc files.
 
-#include <fcntl.h>
-#include <sys/socket.h>
-#include <sys/types.h>
-#include <unistd.h>
-
 #include <memory>
 #include <vector>
 
@@ -23,6 +18,7 @@
 #include <gtest/gtest.h>
 
 #include <brillo/bind_lambda.h>
+#include <brillo/unittest_utils.h>
 #include <brillo/message_loops/base_message_loop.h>
 #include <brillo/message_loops/glib_message_loop.h>
 #include <brillo/message_loops/message_loop_utils.h>
@@ -30,62 +26,6 @@
 using base::Bind;
 using base::TimeDelta;
 
-namespace {
-// Helper class to create and close a unidirectional pipe. Used to provide valid
-// file descriptors when testing watching for a file descriptor.
-class ScopedPipe {
- public:
-  // The internal pipe size.
-  static const int kPipeSize;
-
-  ScopedPipe() {
-    int fds[2];
-    if (pipe(fds) != 0) {
-      PLOG(FATAL) << "Creating a pipe()";
-    }
-    reader = fds[0];
-    writer = fds[1];
-    EXPECT_EQ(kPipeSize, fcntl(writer, F_SETPIPE_SZ, kPipeSize));
-  }
-  ~ScopedPipe() {
-    if (reader != -1)
-      close(reader);
-    if (writer != -1)
-      close(writer);
-  }
-
-  // The reader and writer end of the pipe.
-  int reader{-1};
-  int writer{-1};
-};
-
-const int ScopedPipe::kPipeSize = 4096;
-
-class ScopedSocketPair {
- public:
-  ScopedSocketPair() {
-    int fds[2];
-    if (socketpair(PF_LOCAL, SOCK_STREAM, 0, fds) != 0) {
-      PLOG(FATAL) << "Creating a socketpair()";
-    }
-    left = fds[0];
-    right = fds[1];
-  }
-  ~ScopedSocketPair() {
-    if (left != -1)
-      close(left);
-    if (right != -1)
-      close(right);
-  }
-
-  // The left and right sockets are bi-directional connected and
-  // indistinguishable file descriptor. We named them left/right for easier
-  // reading.
-  int left{-1};
-  int right{-1};
-};
-}  // namespace
-
 namespace brillo {
 
 using TaskId = MessageLoop::TaskId;
diff --git a/brillo/process_unittest.cc b/brillo/process_unittest.cc
index 16c1ecc..de6cd72 100644
--- a/brillo/process_unittest.cc
+++ b/brillo/process_unittest.cc
@@ -12,6 +12,7 @@
 #include <gtest/gtest.h>
 
 #include "brillo/process_mock.h"
+#include "brillo/unittest_utils.h"
 #include "brillo/test_helpers.h"
 
 using base::FilePath;
@@ -29,6 +30,7 @@ static const char kBinCp[] = SYSTEM_PREFIX "/bin/cp";
 static const char kBinEcho[] = SYSTEM_PREFIX "/bin/echo";
 static const char kBinFalse[] = SYSTEM_PREFIX "/bin/false";
 static const char kBinSleep[] = SYSTEM_PREFIX "/bin/sleep";
+static const char kBinStat[] = SYSTEM_PREFIX "/usr/bin/stat";
 static const char kBinTrue[] = SYSTEM_PREFIX "/bin/true";
 
 namespace brillo {
@@ -94,7 +96,6 @@ class ProcessTest : public ::testing::Test {
  protected:
   void CheckStderrCaptured();
   FilePath GetFdPath(int fd);
-  bool FileDescriptorExists(int pid, int fd);
 
   ProcessImpl process_;
   std::vector<const char*> args_;
@@ -168,11 +169,6 @@ FilePath ProcessTest::GetFdPath(int fd) {
   return FilePath(base::StringPrintf("/proc/self/fd/%d", fd));
 }
 
-bool ProcessTest::FileDescriptorExists(int pid, int fd) {
-  return base::PathExists(
-      FilePath(base::StringPrintf("/proc/%d/fd/%d", pid, fd)));
-}
-
 TEST_F(ProcessTest, RedirectStderrUsingPipe) {
   std::string contents;
   process_.RedirectOutput("");
@@ -341,35 +337,23 @@ TEST_F(ProcessTest, PreExecCallback) {
 }
 
 TEST_F(ProcessTest, LeakUnusedFileDescriptors) {
-  int fds[2];
-  EXPECT_EQ(0, pipe(fds));
-  process_.AddArg(kBinSleep);
-  process_.AddArg("10000");
+  ScopedPipe pipe;
+  process_.AddArg(kBinStat);
+  process_.AddArg(GetFdPath(pipe.reader).value());
+  process_.AddArg(GetFdPath(pipe.writer).value());
   process_.SetCloseUnusedFileDescriptors(false);
-  ASSERT_TRUE(process_.Start());
-  // Give child process a bit time to come up.
-  usleep(10 * 1000);
-  // Verify file descriptors are leaking to the child process.
-  EXPECT_TRUE(FileDescriptorExists(process_.pid(), fds[0]));
-  EXPECT_TRUE(FileDescriptorExists(process_.pid(), fds[1]));
-
-  EXPECT_TRUE(process_.Kill(SIGTERM, 1));
+  EXPECT_EQ(0, process_.Run());
 }
 
 TEST_F(ProcessTest, CloseUnusedFileDescriptors) {
-  int fds[2];
-  EXPECT_EQ(0, pipe(fds));
-  process_.AddArg(kBinSleep);
-  process_.AddArg("10000");
+  ScopedPipe pipe;
+  process_.AddArg(kBinStat);
+  process_.AddArg(GetFdPath(pipe.reader).value());
+  process_.AddArg(GetFdPath(pipe.writer).value());
   process_.SetCloseUnusedFileDescriptors(true);
-  ASSERT_TRUE(process_.Start());
-  // Give child process a bit time to come up.
-  usleep(10 * 1000);
-  // Verify file descriptors does not get leak to the child process.
-  EXPECT_FALSE(FileDescriptorExists(process_.pid(), fds[0]));
-  EXPECT_FALSE(FileDescriptorExists(process_.pid(), fds[1]));
-
-  EXPECT_TRUE(process_.Kill(SIGTERM, 1));
+  // Stat should fail when running on these file descriptor because the files
+  // should not be there.
+  EXPECT_EQ(1, process_.Run());
 }
 
 }  // namespace brillo
diff --git a/brillo/unittest_utils.cc b/brillo/unittest_utils.cc
new file mode 100644
index 0000000..7cace5d
--- /dev/null
+++ b/brillo/unittest_utils.cc
@@ -0,0 +1,53 @@
+// Copyright 2014 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <brillo/unittest_utils.h>
+
+#include <fcntl.h>
+#include <sys/socket.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include <base/logging.h>
+#include <gtest/gtest.h>
+
+namespace brillo {
+
+const int ScopedPipe::kPipeSize = 4096;
+
+ScopedPipe::ScopedPipe() {
+  int fds[2];
+  if (pipe(fds) != 0) {
+    PLOG(FATAL) << "Creating a pipe()";
+  }
+  reader = fds[0];
+  writer = fds[1];
+  EXPECT_EQ(kPipeSize, fcntl(writer, F_SETPIPE_SZ, kPipeSize));
+}
+
+ScopedPipe::~ScopedPipe() {
+  if (reader != -1)
+    close(reader);
+  if (writer != -1)
+    close(writer);
+}
+
+
+ScopedSocketPair::ScopedSocketPair() {
+  int fds[2];
+  if (socketpair(PF_LOCAL, SOCK_STREAM, 0, fds) != 0) {
+    PLOG(FATAL) << "Creating a socketpair()";
+  }
+  left = fds[0];
+  right = fds[1];
+}
+
+ScopedSocketPair::~ScopedSocketPair() {
+  if (left != -1)
+    close(left);
+  if (right != -1)
+    close(right);
+}
+
+}  // namespace brillo
diff --git a/brillo/unittest_utils.h b/brillo/unittest_utils.h
new file mode 100644
index 0000000..a7f92db
--- /dev/null
+++ b/brillo/unittest_utils.h
@@ -0,0 +1,43 @@
+// Copyright 2015 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Internal implementation of brillo::Any class.
+
+#ifndef LIBBRILLO_BRILLO_UNITTEST_UTILS_H_
+#define LIBBRILLO_BRILLO_UNITTEST_UTILS_H_
+
+namespace brillo {
+
+// Helper class to create and close a unidirectional pipe. The file descriptors
+// will be closed on destruction, unless set to -1.
+class ScopedPipe {
+ public:
+  // The internal pipe size.
+  static const int kPipeSize;
+
+  ScopedPipe();
+  ~ScopedPipe();
+
+  // The reader and writer end of the pipe.
+  int reader{-1};
+  int writer{-1};
+};
+
+// Helper class to create and close a bi-directional pair of sockets. The
+// sockets will be closed on destruction, unless set to -1.
+class ScopedSocketPair {
+ public:
+  ScopedSocketPair();
+  ~ScopedSocketPair();
+
+  // The left and right sockets are bi-directional connected and
+  // indistinguishable file descriptor. We named them left/right for easier
+  // reading.
+  int left{-1};
+  int right{-1};
+};
+
+}  // namespace brillo
+
+#endif  // LIBBRILLO_BRILLO_UNITTEST_UTILS_H_
diff --git a/libbrillo.gypi b/libbrillo.gypi
index d0b3504..6dec2e4 100644
--- a/libbrillo.gypi
+++ b/libbrillo.gypi
@@ -353,6 +353,7 @@
             'brillo/streams/stream_unittest.cc',
             'brillo/streams/stream_utils_unittest.cc',
             'brillo/strings/string_utils_unittest.cc',
+            'brillo/unittest_utils.cc',
             'brillo/url_utils_unittest.cc',
             'brillo/variant_dictionary_unittest.cc',
             'testrunner.cc',