Fix failed apk unit test case MessagePumpLibeventTest.* on Android.

base/message_pump_libevent_unittest.cc

 // Copyright (c) 2012 The Chromium 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 "base/message_pump_libevent.h"
 
 #include <unistd.h>
 
+#include "base/eintr_wrapper.h"
 #include "base/message_loop.h"
 #include "base/threading/thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(USE_SYSTEM_LIBEVENT)
 #include <event.h>
 #else
 #include "third_party/libevent/event.h"
 #endif
 
@@ skipping 37 matching lines @@
 
   // base:MessagePumpLibevent::Watcher interface
   virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE {}
   virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {}
 };
 
 #if GTEST_HAS_DEATH_TEST && !defined(NDEBUG)
 
 // Test to make sure that we catch calling WatchFileDescriptor off of the
 // wrong thread.
 TEST_F(MessagePumpLibeventTest, TestWatchingFromBadThread) {

[jar (doing other things), 2012/07/09 21:12:23]

Where is TestWatchingFromBadThread defined?

[Shouqun Liu, 2012/07/10 05:42:41]

This is a death test, I think we don't need to define TestWatchingFromBadThread
On 2012/07/09 21:12:23, jar wrote:

[jar (doing other things), 2012/07/10 17:02:10]

<doh>  For some reason I was confusing this test name with the class name...
sorry.  I don't know what I was thinking. <sigh>

Nevermind. 

On 2012/07/10 05:42:41, Shouqun Liu wrote:

   MessagePumpLibevent::FileDescriptorWatcher watcher;
   StupidWatcher delegate;
 
   ASSERT_DEATH(io_loop()->WatchFileDescriptor(
       STDOUT_FILENO, false, MessageLoopForIO::WATCH_READ, &watcher, &delegate),
       "Check failed: "
       "watch_file_descriptor_caller_checker_.CalledOnValidThread()");
 }
 
 #endif  // GTEST_HAS_DEATH_TEST && !defined(NDEBUG)
@@ skipping 17 matching lines @@
     DCHECK(controller_);
     delete controller_;
     controller_ = NULL;
   }
 
  private:
   MessagePumpLibevent::FileDescriptorWatcher* controller_;
 };
 
 TEST_F(MessagePumpLibeventTest, DeleteWatcher) {
+  int pipefds[2];
+  int err = pipe(pipefds);
+  ASSERT_EQ(0, err);
+  int fd = pipefds[1];
   scoped_refptr<MessagePumpLibevent> pump(new MessagePumpLibevent);
   MessagePumpLibevent::FileDescriptorWatcher* watcher =
       new MessagePumpLibevent::FileDescriptorWatcher;
   DeleteWatcher delegate(watcher);
   pump->WatchFileDescriptor(
-      0, false, MessagePumpLibevent::WATCH_READ_WRITE, watcher, &delegate);
+      fd, false, MessagePumpLibevent::WATCH_READ_WRITE, watcher, &delegate);
 
   // Spoof a libevent notification.
   OnLibeventNotification(pump, watcher);
+  if (HANDLE_EINTR(close(pipefds[0])) < 0)
+    PLOG(ERROR) << "close";
+  if (HANDLE_EINTR(close(pipefds[1])) < 0)
+    PLOG(ERROR) << "close";
 }
 
 class StopWatcher : public MessagePumpLibevent::Watcher {
  public:
   explicit StopWatcher(
       MessagePumpLibevent::FileDescriptorWatcher* controller)
       : controller_(controller) {
     DCHECK(controller_);
   }
   virtual ~StopWatcher() {}
 
   // base:MessagePumpLibevent::Watcher interface
   virtual void OnFileCanReadWithoutBlocking(int /* fd */) OVERRIDE {
     NOTREACHED();
   }
   virtual void OnFileCanWriteWithoutBlocking(int /* fd */) OVERRIDE {
     controller_->StopWatchingFileDescriptor();
   }
 
  private:
   MessagePumpLibevent::FileDescriptorWatcher* const controller_;
 };
 
 TEST_F(MessagePumpLibeventTest, StopWatcher) {
+  int pipefds[2];
+  int err = pipe(pipefds);
+  ASSERT_EQ(0, err);
+  int fd = pipefds[1];

[jar (doing other things), 2012/07/09 21:12:23]

I hate seeing growth of replicated code... 

WDYT of:

Put this creation/closing into the constructor and destructor of the test class.
 This way it is harder to "not clean up" in the test.

Given that DeleteWatcher and StopWatcher are almost identical (and grow larger
and yet more identical with this constructor/destructor addition), combine them
into one Watcher class, with an enum arg to the constructor indicating if it
should DELETE or just STOP.

You can then have two separately named derived clases StopWatcher and
DeleteWatcher that pass in the two choice DELETE vs STOP, and use them in the
test (as they are now).

Code gets bloated more and more as folks make these changes... and it would be
nice to clean as you fix.

WDYT?

[Shouqun Liu, 2012/07/10 05:42:41]

Yes, agree with you.  I have made the following changes:
1, move the creation/deletion of pipe into the SetUp/TearDown of test case class
MessagePumpLibeventTest to reducing replicated code.
2, Use a base class BaseWatcher to merge StopWatcher/DeleteWatcher, and
StopWatcher/DeleteWatcher derived from BaseWatcher, TYPE_STOP/TYPE_DELETE in
constructor to control which one should be used.
On 2012/07/09 21:12:23, jar wrote:

   scoped_refptr<MessagePumpLibevent> pump(new MessagePumpLibevent);
   MessagePumpLibevent::FileDescriptorWatcher watcher;
   StopWatcher delegate(&watcher);
   pump->WatchFileDescriptor(
-      0, false, MessagePumpLibevent::WATCH_READ_WRITE, &watcher, &delegate);
+      fd, false, MessagePumpLibevent::WATCH_READ_WRITE, &watcher, &delegate);
 
   // Spoof a libevent notification.
   OnLibeventNotification(pump, &watcher);
+  if (HANDLE_EINTR(close(pipefds[0])) < 0)
+    PLOG(ERROR) << "close";
+  if (HANDLE_EINTR(close(pipefds[1])) < 0)
+    PLOG(ERROR) << "close";
 }
 
 }  // namespace
 
 }  // namespace base