Fix timeout of DistillablePageUtilsBrowserTest on DrMemory

chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc

Index: chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc
diff --git a/chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc b/chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc
index 37b899096e2fceb73ff0bc928ad7ce10304c884a..d4343086eeff35290bed73b24462cd35eb1ccdd5 100644
--- a/chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc
+++ b/chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc
@@ -53,6 +53,39 @@ class MockDelegate : public Holder {
   }
 };
 
+// Wait a bit to make sure there are no extra calls after the last expected
+// call. All the expected calls happen within ~1ms on linux release build,
+// so 100ms should be pretty safe to catch extra calls.
+// If there are no extra calls, changing this doesn't change the test result.
+const unsigned kWaitAfterLastCall = 100;

[Lei Zhang, 2016/07/21 22:54:26]

s/unsigned/int/ ? As it's being used in quitAfter() as an int. Also put Ms in
the name so readers can tell what the unit is.

[wychen, 2016/07/22 22:39:53]

Converted all to unsigned. Added Ms.

[Lei Zhang, 2016/07/26 20:56:35]

Any reason to favor unsigned? TimeDelta::FromMilliseconds() ultimately takes an
int64_t. At least match the signed-ness?

[wychen, 2016/07/27 01:41:12]

Done

+
+// If there are no expected calls, we wait for a while to make sure there are

[Lei Zhang, 2016/07/21 22:54:26]

s/we/the test/

Sometimes "we" can be unclear as to whom "we" refers to.

[wychen, 2016/07/22 22:39:53]

Done.

+// no calls in this period of time. When there are expected calls, they happen
+// within 100ms after content::WaitForLoadStop() on linux release build, and
+// 10X safety margin is used.
+// If there are no extra calls, changing this doesn't change the test result.
+const unsigned kWaitNoExpectedCall = 1000;
+
+// QuitWhenIdleClosure() would become nop if it is called before

[Lei Zhang, 2016/07/21 22:54:26]

s/nop/no-op/

[wychen, 2016/07/22 22:39:53]

Done.

+// content::RunMessageLoop(). This timeout should be long enough to make sure
+// at least one QuitWhenIdleClosure() is called after RunMessageLoop().
+// All tests are limited by kWaitAfterLastCall or kWaitNoExpectedCall, so

[Lei Zhang, 2016/07/21 22:54:26]

Refer to variables as |variable_name|

[wychen, 2016/07/22 22:39:53]

Done.

+// making this longer doesn't actually make tests run for longer, unless
+// kWaitAfterLastCall or kWaitNoExpectedCall are so small or the test is so
+// slow, for example, on Dr. Memory or Android, that QuitWhenIdleClosure()
+// is called prematurely. 100X safety margin is used.
+const unsigned kDefaultTimeout = 10000;
+
+void quitAfter(int time_ms) {

[Lei Zhang, 2016/07/21 22:54:26]

C++ methods have CapitalNames(). Have you been writing too much Java lately? ;)

[wychen, 2016/07/22 22:39:53]

Oops.

+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, base::MessageLoop::QuitWhenIdleClosure(),
+      base::TimeDelta::FromMilliseconds(time_ms));
+}
+
+void quitSoon() {
+  quitAfter(kWaitAfterLastCall);
+}
+
 }  // namespace
 
 template<const char Option[]>
@@ -72,7 +105,7 @@ class DistillablePageUtilsBrowserTestOption : public InProcessBrowserTest {
     setDelegate(web_contents_, holder_.GetDelegate());
   }
 
-  void NavigateAndWait(const char* url) {
+  void NavigateAndWait(const char* url, unsigned timeout_ms = 0) {

[Lei Zhang, 2016/07/21 22:54:26]

int here as well.

[Lei Zhang, 2016/07/21 22:54:26]

There aren't that many callers. Just fix the callsites to pass in 0, rather than
having a default argument.

[wychen, 2016/07/22 22:39:53]

This is one extra timeout. When not passed, it's not there.
I think it's more readable as a default argument.
Would it be better if using overloading instead?

[wychen, 2016/07/22 22:39:53]

Done.

[Lei Zhang, 2016/07/23 00:51:24]

I still stand by my original statement since there's so few callers. I also
admit to being biased against default arguments.

The pattern I sometimes see in code that turns me off from default arguments is:

FuncWithManyCallers(bool do_foo = false, bool do_bar = false, ...) {
  // lots of complicated code, calls to other functions assuming |do_foo| /
|do_bar| can be true.
}

and then after checking the many callers, find out |do_foo| is always false. :-[

[wychen, 2016/07/24 23:50:31]

Make sense. Removed the default parameter.

     GURL article_url(url);
     if (base::StartsWith(url, "/", base::CompareCase::SENSITIVE)) {
       article_url = embedded_test_server()->GetURL(url);
@@ -82,10 +115,11 @@ class DistillablePageUtilsBrowserTestOption : public InProcessBrowserTest {
     ui_test_utils::NavigateToURL(browser(), article_url);
     content::WaitForLoadStop(web_contents_);
 
-    // Wait a bit for the message.
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, base::MessageLoop::QuitWhenIdleClosure(),
-        base::TimeDelta::FromMilliseconds(100));
+    quitAfter(kDefaultTimeout);
+    if (timeout_ms) {
+      // Local time-out for the tests that don't expect callbacks.
+      quitAfter(timeout_ms);
+    }
     content::RunMessageLoop();
   }
 
@@ -99,22 +133,17 @@ using DistillablePageUtilsBrowserTestAlways =
 
 IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAlways,
                        TestDelegate) {
-  // Run twice to make sure the delegate object is still alive.
-  for (int i = 0; i < 2; ++i) {
-    testing::InSequence dummy;
-    EXPECT_CALL(holder_, OnResult(true, true)).Times(1);
-    NavigateAndWait(kSimpleArticlePath);
-  }
   for (unsigned i = 0; i < sizeof(kAllPaths) / sizeof(kAllPaths[0]); ++i) {
     testing::InSequence dummy;
-    EXPECT_CALL(holder_, OnResult(true, true)).Times(1);
+    EXPECT_CALL(holder_, OnResult(true, true))
+        .WillOnce(testing::InvokeWithoutArgs(quitSoon));
     NavigateAndWait(kAllPaths[i]);
   }
   // Test pages that we don't care about its distillability.
   {
     testing::InSequence dummy;
     EXPECT_CALL(holder_, OnResult(_, _)).Times(0);
-    NavigateAndWait("about:blank");
+    NavigateAndWait("about:blank", kWaitNoExpectedCall);
   }
 }
 
@@ -125,7 +154,7 @@ using DistillablePageUtilsBrowserTestNone =
 IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestNone,
                        TestDelegate) {
   EXPECT_CALL(holder_, OnResult(_, _)).Times(0);
-  NavigateAndWait(kSimpleArticlePath);
+  NavigateAndWait(kSimpleArticlePath, kWaitNoExpectedCall);
 }
 
 
@@ -136,12 +165,14 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestOG,
                        TestDelegate) {
   {
     testing::InSequence dummy;
-    EXPECT_CALL(holder_, OnResult(true, true)).Times(1);
+    EXPECT_CALL(holder_, OnResult(true, true))
+        .WillOnce(testing::InvokeWithoutArgs(quitSoon));
     NavigateAndWait(kArticlePath);
   }
   {
     testing::InSequence dummy;
-    EXPECT_CALL(holder_, OnResult(false, true)).Times(1);
+    EXPECT_CALL(holder_, OnResult(false, true))
+        .WillOnce(testing::InvokeWithoutArgs(quitSoon));
     NavigateAndWait(kNonArticlePath);
   }
 }
@@ -156,13 +187,15 @@ IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAdaboost,
   for (unsigned i = 0; i < sizeof(paths)/sizeof(paths[0]); ++i) {
     testing::InSequence dummy;
     EXPECT_CALL(holder_, OnResult(true, false)).Times(1);
-    EXPECT_CALL(holder_, OnResult(true, true)).Times(1);
+    EXPECT_CALL(holder_, OnResult(true, true))
+        .WillOnce(testing::InvokeWithoutArgs(quitSoon));
     NavigateAndWait(paths[i]);
   }
   {
     testing::InSequence dummy;
     EXPECT_CALL(holder_, OnResult(false, false)).Times(1);
-    EXPECT_CALL(holder_, OnResult(false, true)).Times(1);
+    EXPECT_CALL(holder_, OnResult(false, true))
+        .WillOnce(testing::InvokeWithoutArgs(quitSoon));
     NavigateAndWait(kNonArticlePath);
   }
 }