Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2171813003:
Fix timeout of DistillablePageUtilsBrowserTest on DrMemory (Closed)
DescriptionFix timeout of DistillablePageUtilsBrowserTest on DrMemory
BUG=561085, 571263
Committed: https://crrev.com/ca28f4ea9e37b01d8427ebe889f854ffb8851976
Cr-Commit-Position: refs/heads/master@{#408021}
Patch Set 1 #
Total comments: 18
Patch Set 2 : address comments #Patch Set 3 : don't use default param #Patch Set 4 : use signed time #
Messages
Total messages: 17 (4 generated)
wychen@chromium.org changed reviewers: + thestig@chromium.org
PTAL. I emulated DrMemory by making the timeout settings 10~100X smaller. How feasible is it to test flakiness of these with DrMemory before landing them?
On 2016/07/21 22:36:31, wychen wrote: > PTAL. > > I emulated DrMemory by making the timeout settings 10~100X smaller. How feasible > is it to test flakiness of these with DrMemory before landing them? It's not that hard to run with DrMemory if you have a Windows machine: https://www.chromium.org/developers/how-tos/using-drmemory You also need to remove the tools/valgrind/gtest_exclude/browser_tests.gtest-drmemory.txt entries to re-enable the tests on DrMemory bots.
We may have discussed this before, but is there any way to avoid waiting for a specific amount of time? Assuming you want to go with just extending the wait time, nits below: https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right): https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:60: const unsigned kWaitAfterLastCall = 100; 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. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:62: // If there are no expected calls, we wait for a while to make sure there are s/we/the test/ Sometimes "we" can be unclear as to whom "we" refers to. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:69: // QuitWhenIdleClosure() would become nop if it is called before s/nop/no-op/ https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:72: // All tests are limited by kWaitAfterLastCall or kWaitNoExpectedCall, so Refer to variables as |variable_name| https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:79: void quitAfter(int time_ms) { C++ methods have CapitalNames(). Have you been writing too much Java lately? ;) https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:108: void NavigateAndWait(const char* url, unsigned timeout_ms = 0) { int here as well. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:108: void NavigateAndWait(const char* url, unsigned timeout_ms = 0) { There aren't that many callers. Just fix the callsites to pass in 0, rather than having a default argument.
On 2016/07/21 22:44:05, Lei Zhang wrote: > On 2016/07/21 22:36:31, wychen wrote: > > PTAL. > > > > I emulated DrMemory by making the timeout settings 10~100X smaller. How > feasible > > is it to test flakiness of these with DrMemory before landing them? > > It's not that hard to run with DrMemory if you have a Windows machine: > https://www.chromium.org/developers/how-tos/using-drmemory I don't have one, and I've seen peers setting up a Windows virtual machine and it was painful. Is it possible to modify the config in the CL (just for tests) so that the tryjob runs specific tests for multiple times? > You also need to remove the > tools/valgrind/gtest_exclude/browser_tests.gtest-drmemory.txt entries to > re-enable the tests on DrMemory bots. Will do.
On 2016/07/21 23:00:05, wychen wrote: > On 2016/07/21 22:44:05, Lei Zhang wrote: > > On 2016/07/21 22:36:31, wychen wrote: > > > PTAL. > > > > > > I emulated DrMemory by making the timeout settings 10~100X smaller. How > > feasible > > > is it to test flakiness of these with DrMemory before landing them? > > > > It's not that hard to run with DrMemory if you have a Windows machine: > > https://www.chromium.org/developers/how-tos/using-drmemory > I don't have one, and I've seen peers setting up a Windows virtual machine > and it was painful. Yes, that would be painful. Don't do Windows in a VM to build Chrome. > Is it possible to modify the config in the CL (just for tests) so that > the tryjob runs specific tests for multiple times? Not as far as I know. I can test for you if you'd like, though it will probably have to wait until Monday with all the things I have going on.
https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right): https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:60: const unsigned kWaitAfterLastCall = 100; On 2016/07/21 22:54:26, Lei Zhang wrote: > 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. Converted all to unsigned. Added Ms. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:62: // If there are no expected calls, we wait for a while to make sure there are On 2016/07/21 22:54:26, Lei Zhang wrote: > s/we/the test/ > > Sometimes "we" can be unclear as to whom "we" refers to. Done. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:69: // QuitWhenIdleClosure() would become nop if it is called before On 2016/07/21 22:54:26, Lei Zhang wrote: > s/nop/no-op/ Done. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:72: // All tests are limited by kWaitAfterLastCall or kWaitNoExpectedCall, so On 2016/07/21 22:54:26, Lei Zhang wrote: > Refer to variables as |variable_name| Done. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:79: void quitAfter(int time_ms) { On 2016/07/21 22:54:26, Lei Zhang wrote: > C++ methods have CapitalNames(). Have you been writing too much Java lately? ;) Oops. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:108: void NavigateAndWait(const char* url, unsigned timeout_ms = 0) { On 2016/07/21 22:54:26, Lei Zhang wrote: > int here as well. Done. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:108: void NavigateAndWait(const char* url, unsigned timeout_ms = 0) { On 2016/07/21 22:54:26, Lei Zhang wrote: > There aren't that many callers. Just fix the callsites to pass in 0, rather than > having a default argument. 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?
It'll take a little bit of time to rebuild for DrMemory, but I'll do just that and test this out for you. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right): https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:108: void NavigateAndWait(const char* url, unsigned timeout_ms = 0) { On 2016/07/22 22:39:53, wychen wrote: > On 2016/07/21 22:54:26, Lei Zhang wrote: > > There aren't that many callers. Just fix the callsites to pass in 0, rather > than > > having a default argument. > > 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? 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. :-[
Thanks for testing this for me. We did discuss about the testing methodology, and testing callback is certainly less flaky than using timeout. The thing is, the number of calls could vary. If we don't check for extra calls to the callback function, we could just terminate the test when the expected number of calls are reached, and there would be no timeout needed. There has been a bug causing extra calls, and I'd like to be able to catch this in the tests, so I decided to still wait for a while after the last expected callback. This way, extra calls can likely be caught on normal bots, and it shouldn't fail on DrMemory bots due to timeout issues, since the timeout waiting for extra calls wouldn't fail the test if there are no extra calls. The one thing I don't like is that I have to use |kDefaultTimeoutMs|. The racing condition do happen if I change the waiting time to 0.01X to emulate DrMemory speed, but I don't have an elegant way to handle it. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right): https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:108: void NavigateAndWait(const char* url, unsigned timeout_ms = 0) { On 2016/07/23 00:51:24, Lei Zhang wrote: > On 2016/07/22 22:39:53, wychen wrote: > > On 2016/07/21 22:54:26, Lei Zhang wrote: > > > There aren't that many callers. Just fix the callsites to pass in 0, rather > > than > > > having a default argument. > > > > 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? > > 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. :-[ Make sense. Removed the default parameter.
I tested this a few times under DrMemory and it passes all my tests, so let's give this a shot and see if it makes the bots happy too. lgtm https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right): https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:60: const unsigned kWaitAfterLastCall = 100; On 2016/07/22 22:39:53, wychen wrote: > On 2016/07/21 22:54:26, Lei Zhang wrote: > > 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. > > Converted all to unsigned. Added Ms. Any reason to favor unsigned? TimeDelta::FromMilliseconds() ultimately takes an int64_t. At least match the signed-ness?
Thanks for testing this. Let's see how stable it is on bots. https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right): https://codereview.chromium.org/2171813003/diff/1/chrome/browser/dom_distille... chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:60: const unsigned kWaitAfterLastCall = 100; On 2016/07/26 20:56:35, Lei Zhang wrote: > On 2016/07/22 22:39:53, wychen wrote: > > On 2016/07/21 22:54:26, Lei Zhang wrote: > > > 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. > > > > Converted all to unsigned. Added Ms. > > Any reason to favor unsigned? TimeDelta::FromMilliseconds() ultimately takes an > int64_t. At least match the signed-ness? Done
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2171813003/#ps60001 (title: "use signed time")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix timeout of DistillablePageUtilsBrowserTest on DrMemory BUG=561085, 571263 ========== to ========== Fix timeout of DistillablePageUtilsBrowserTest on DrMemory BUG=561085, 571263 Committed: https://crrev.com/ca28f4ea9e37b01d8427ebe889f854ffb8851976 Cr-Commit-Position: refs/heads/master@{#408021} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ca28f4ea9e37b01d8427ebe889f854ffb8851976 Cr-Commit-Position: refs/heads/master@{#408021} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
