Add base::RunLoop and update ui_test_utils to use it to reduce flakiness

content/test/test_navigation_observer.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 "content/public/test/test_navigation_observer.h"
 
 #include "base/bind.h"
 #include "base/message_loop.h"
+#include "base/run_loop.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_view_host_observer.h"
 #include "content/public/test/js_injection_ready_observer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
+namespace {
+
+// Allow some pending tasks and up to |num_deferrals| generations to complete.
+static void DeferredQuitRunLoop(
+    const base::WeakPtr<base::RunLoop>& run_loop,
+    int num_quit_deferrals) {
+  if (num_quit_deferrals <= 0) {
+    if (run_loop) run_loop->Quit();

[jar (doing other things), 2012/06/23 02:43:19]

(as commented in other file): When run_loop is null, you may as well just return
before line 24.

[jbates, 2012/06/25 20:04:57]

Done.

+  } else {
+    MessageLoop::current()->PostTask(FROM_HERE,
+        base::Bind(&DeferredQuitRunLoop, run_loop, num_quit_deferrals - 1));
+  }
+}
+
+}  // namespace
+
 // This class observes |render_view_host| and calls OnJsInjectionReady() of
 // |js_injection_ready_observer| when the time is right to inject JavaScript
 // into the page.
 class TestNavigationObserver::RVHOSendJS : public RenderViewHostObserver {
  public:
   RVHOSendJS(RenderViewHost* render_view_host,
              JsInjectionReadyObserver* js_injection_ready_observer)
       : RenderViewHostObserver(render_view_host),
         js_injection_ready_observer_(js_injection_ready_observer) {
   }
@@ skipping 50 matching lines @@
     return;
 
   EXPECT_FALSE(running_);
   running_ = true;
   done_callback_ = done_callback;
   wait_loop_callback.Run();
   EXPECT_TRUE(done_);
 }
 
 void TestNavigationObserver::Wait() {
+  base::RunLoop run_loop;
+  // Number of times to repost QuitNow task to allow pending tasks to complete:
+  const int num_quit_deferrals = 5;

[jar (doing other things), 2012/06/23 02:43:19]

Is this a hack number guess, or is this meaningful?

[jbates, 2012/06/25 20:04:57]

It's approaching being meaningful. 20 worked but may have been overkill. 2
caused an uncomfortable number of test failures. Now I'm at 5 seeing if the
trybots agree.

[jar (doing other things), 2012/06/25 21:19:43]

The larger number (20) should have no measurable impact on performance (20
minimal tasks posted and run should probably complete nicely in less than 1ms). 
The downside risk of "getting this wrong" is a flaky test, and a revisit.

IMO, you should document how you tried to pull this number form the air, mention
it can be increased if the test proves flaky, and be very conservative (choose
big... like 20?) in your selection of a constant.


On 2012/06/25 20:04:57, jbates wrote:

[jbates, 2012/06/27 02:06:37]

Docs done. Went with 10, because some perf tests might have some largish 200ms
frames, and 200ms*20 is starting to seem too long of a wait per test Wait. 10
should be enough, watching the bots...

   WaitForObservation(
-      base::Bind(&MessageLoop::Run,
-                 base::Unretained(MessageLoop::current())),
-      base::Bind(&MessageLoop::Quit,
-                 base::Unretained(MessageLoop::current())));
+      base::Bind(&base::RunLoop::Run, run_loop.AsWeakPtr()),
+      base::Bind(&DeferredQuitRunLoop, run_loop.AsWeakPtr(),
+                 num_quit_deferrals));
 }
 
 TestNavigationObserver::TestNavigationObserver(
     JsInjectionReadyObserver* js_injection_ready_observer,
     int number_of_navigations)
     : navigation_started_(false),
       navigations_completed_(0),
       number_of_navigations_(number_of_navigations),
       js_injection_ready_observer_(js_injection_ready_observer),
       done_(false),
@@ skipping 22 matching lines @@
   switch (type) {
     case NOTIFICATION_NAV_ENTRY_COMMITTED:
     case NOTIFICATION_LOAD_START:
       navigation_started_ = true;
       break;
     case NOTIFICATION_LOAD_STOP:
       if (navigation_started_ &&
           ++navigations_completed_ == number_of_navigations_) {
         navigation_started_ = false;
         done_ = true;
-        if (running_)
+        if (running_) {
+          running_ = false;
           done_callback_.Run();
+        }
       }
       break;
     case NOTIFICATION_RENDER_VIEW_HOST_CREATED:
       rvho_send_js_.reset(new RVHOSendJS(
           Source<RenderViewHost>(source).ptr(),
           js_injection_ready_observer_));
       break;
     default:
       NOTREACHED();
   }
 }
 
 }  // namespace content