Fix timeout of DistillablePageUtilsBrowserTest on DrMemory

chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc

 // Copyright 2015 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 <string.h>
 
 #include "base/command_line.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ skipping 35 matching lines @@
 
 class MockDelegate : public Holder {
  public:
   MOCK_METHOD2(OnResult, void(bool, bool));
 
   base::Callback<void(bool, bool)> GetDelegate() {
     return base::Bind(&MockDelegate::OnResult, base::Unretained(this));
   }
 };
 
+// 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[]>
 class DistillablePageUtilsBrowserTestOption : public InProcessBrowserTest {
  public:
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitch(switches::kEnableDomDistiller);
     command_line->AppendSwitchASCII(switches::kReaderModeHeuristics,
         Option);
   }
 
   void SetUpOnMainThread() override {
     InProcessBrowserTest::SetUpOnMainThread();
     ASSERT_TRUE(embedded_test_server()->Start());
     web_contents_ =
         browser()->tab_strip_model()->GetActiveWebContents();
     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);
     }
 
     // This blocks until the navigation has completely finished.
     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();
   }
 
   MockDelegate holder_;
   content::WebContents* web_contents_;
 };
 
 
 using DistillablePageUtilsBrowserTestAlways =
     DistillablePageUtilsBrowserTestOption<kAlwaysTrue>;
 
 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);
   }
 }
 
 
 using DistillablePageUtilsBrowserTestNone =
     DistillablePageUtilsBrowserTestOption<kNone>;
 
 IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestNone,
                        TestDelegate) {
   EXPECT_CALL(holder_, OnResult(_, _)).Times(0);
-  NavigateAndWait(kSimpleArticlePath);
+  NavigateAndWait(kSimpleArticlePath, kWaitNoExpectedCall);
 }
 
 
 using DistillablePageUtilsBrowserTestOG =
     DistillablePageUtilsBrowserTestOption<kOGArticle>;
 
 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);
   }
 }
 
 
 using DistillablePageUtilsBrowserTestAdaboost =
     DistillablePageUtilsBrowserTestOption<kAdaBoost>;
 
 IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAdaboost,
                        TestDelegate) {
   const char* paths[] = {kSimpleArticlePath, kSimpleArticleIFramePath};
   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);
   }
 }
 
 }  // namespace dom_distiller