Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1634803004:
Convert CookieMonster tests to use base::RunLoop (Closed)
DescriptionConvert CookieMonster tests to use base::RunLoop
CookieMonster tests post a
base::MessageLoop::QuitWhenIdleClosure task which
could be run at the start of some other test, since
the unit tests are invoked in the same fixture on
Android. This CL converts these CookieMonster tests
to use base::RunLoop so they don't influence other
tests.
BUG=568282
Committed: https://crrev.com/b2bec813bb029f85f709860cd3eecce647df8cc1
Cr-Commit-Position: refs/heads/master@{#371908}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Add an out-of-line destructor #
Total comments: 19
Patch Set 4 : Comments #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Convert CookieMonster tests to use base::RunLoop BUG= ========== to ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a Quit task which affected other tests on Android, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 ==========
Description was changed from ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a Quit task which affected other tests on Android, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 ========== to ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 ==========
Description was changed from ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 ========== to ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other test, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 ==========
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
Hi Matt, I am still finishing up try jobs, but converting CookieMonster seems to fix the flake at least locally. PTAL. Thanks!
https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:837: // saving the provided callback and sending a quit to the message loop. Docs are outdated. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:838: void ExpectLoadCall() { Maybe add some sort of CHECK to make sure this is only called once, along with a comment? run loops can be reused, and while it doesn't look like any tests do that, I don't see any current reason why they can't do that. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:846: void ExpectLoadForKeyCall(const std::string& key) { This was never being called with true, right? https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:871: scoped_ptr<base::RunLoop> load_run_loop_; No need to use a scoped_ptr https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:895: scoped_ptr<base::RunLoop> loop(new base::RunLoop); None of these need to use scoped_ptrs. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... File net/cookies/cookie_store_test_callbacks.cc (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.cc:25: loop_to_quit_(new base::RunLoop) {} nit: Can just replace this with: CookieCallback::CookieCallback() : CookieCallback(base::MessageLoop::current()) ... https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... File net/cookies/cookie_store_test_callbacks.h (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.h:28: bool did_run() { return did_run_; } This is no longer needed - the fact that the message loop is exited should be sufficient evidence this did run (If any other tests post a stray quit message, like the tests you're fixing did, that's their fault - shouldn't be up to these tests to detect that) https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.h:52: scoped_ptr<base::RunLoop> loop_to_quit_; No need to use a scoped_ptr.
PTAL. thanks https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:837: // saving the provided callback and sending a quit to the message loop. On 2016/01/27 16:55:42, mmenke wrote: > Docs are outdated. Done. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:838: void ExpectLoadCall() { On 2016/01/27 16:55:42, mmenke wrote: > Maybe add some sort of CHECK to make sure this is only called once, along with a > comment? run loops can be reused, and while it doesn't look like any tests do > that, I don't see any current reason why they can't do that. Done. Added a boolean, not sure if there's a cleaner way. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:846: void ExpectLoadForKeyCall(const std::string& key) { On 2016/01/27 16:55:42, mmenke wrote: > This was never being called with true, right? It was never being called with true. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:871: scoped_ptr<base::RunLoop> load_run_loop_; On 2016/01/27 16:55:42, mmenke wrote: > No need to use a scoped_ptr Done. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_monster_... net/cookies/cookie_monster_unittest.cc:895: scoped_ptr<base::RunLoop> loop(new base::RunLoop); On 2016/01/27 16:55:42, mmenke wrote: > None of these need to use scoped_ptrs. Done. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... File net/cookies/cookie_store_test_callbacks.cc (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.cc:25: loop_to_quit_(new base::RunLoop) {} On 2016/01/27 16:55:42, mmenke wrote: > nit: Can just replace this with: > > CookieCallback::CookieCallback() : CookieCallback(base::MessageLoop::current()) > ... Hmm.. Not sure if I understand. I don't think I can stick a message loop as a base::Thread*, right? Are you suggesting changing the argument? https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... File net/cookies/cookie_store_test_callbacks.h (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.h:28: bool did_run() { return did_run_; } On 2016/01/27 16:55:42, mmenke wrote: > This is no longer needed - the fact that the message loop is exited should be > sufficient evidence this did run (If any other tests post a stray quit message, > like the tests you're fixing did, that's their fault - shouldn't be up to these > tests to detect that) Done. https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.h:52: scoped_ptr<base::RunLoop> loop_to_quit_; On 2016/01/27 16:55:42, mmenke wrote: > No need to use a scoped_ptr. Done.
https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... File net/cookies/cookie_store_test_callbacks.cc (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.cc:25: loop_to_quit_(new base::RunLoop) {} On 2016/01/27 18:35:02, xunjieli wrote: > On 2016/01/27 16:55:42, mmenke wrote: > > nit: Can just replace this with: > > > > CookieCallback::CookieCallback() : > CookieCallback(base::MessageLoop::current()) > > ... > > Hmm.. Not sure if I understand. I don't think I can stick a message loop as a > base::Thread*, right? Are you suggesting changing the argument? No, I'm suggest defining this constructor in terms of the other constructor. CookieCallback::CookieCallback() : CookieCallback(base::MessageLoop::current()) { } Will say when this constructor is called, call the other one, with the current message loop as the argument. So don't have to duplicate shared initialization.
https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... File net/cookies/cookie_store_test_callbacks.cc (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.cc:25: loop_to_quit_(new base::RunLoop) {} On 2016/01/27 18:52:17, mmenke wrote: > On 2016/01/27 18:35:02, xunjieli wrote: > > On 2016/01/27 16:55:42, mmenke wrote: > > > nit: Can just replace this with: > > > > > > CookieCallback::CookieCallback() : > > CookieCallback(base::MessageLoop::current()) > > > ... > > > > Hmm.. Not sure if I understand. I don't think I can stick a message loop as a > > base::Thread*, right? Are you suggesting changing the argument? > > No, I'm suggest defining this constructor in terms of the other constructor. > > CookieCallback::CookieCallback() : CookieCallback(base::MessageLoop::current()) > { > } > > Will say when this constructor is called, call the other one, with the current > message loop as the argument. So don't have to duplicate shared initialization. The other constructor takes in a base::Thread, not a MessageLoop.
LGTM, thanks for tackling this issue so aggressively! https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... File net/cookies/cookie_store_test_callbacks.cc (right): https://codereview.chromium.org/1634803004/diff/2/net/cookies/cookie_store_te... net/cookies/cookie_store_test_callbacks.cc:25: loop_to_quit_(new base::RunLoop) {} On 2016/01/27 18:54:53, xunjieli wrote: > On 2016/01/27 18:52:17, mmenke wrote: > > On 2016/01/27 18:35:02, xunjieli wrote: > > > On 2016/01/27 16:55:42, mmenke wrote: > > > > nit: Can just replace this with: > > > > > > > > CookieCallback::CookieCallback() : > > > CookieCallback(base::MessageLoop::current()) > > > > ... > > > > > > Hmm.. Not sure if I understand. I don't think I can stick a message loop as > a > > > base::Thread*, right? Are you suggesting changing the argument? > > > > No, I'm suggest defining this constructor in terms of the other constructor. > > > > CookieCallback::CookieCallback() : > CookieCallback(base::MessageLoop::current()) > > { > > } > > > > Will say when this constructor is called, call the other one, with the current > > message loop as the argument. So don't have to duplicate shared > initialization. > > The other constructor takes in a base::Thread, not a MessageLoop. Oops...sorry about that.
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1634803004/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1634803004/50001
Message was sent while issue was closed.
Description was changed from ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other test, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 ========== to ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other test, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001)
Message was sent while issue was closed.
Description was changed from ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other test, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 ========== to ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other test, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 Committed: https://crrev.com/b2bec813bb029f85f709860cd3eecce647df8cc1 Cr-Commit-Position: refs/heads/master@{#371908} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b2bec813bb029f85f709860cd3eecce647df8cc1 Cr-Commit-Position: refs/heads/master@{#371908}
Message was sent while issue was closed.
dmoneysligh@gmail.com changed reviewers: + dmoneysligh@gmail.com
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
Description was changed from ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other test, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 Committed: https://crrev.com/b2bec813bb029f85f709860cd3eecce647df8cc1 Cr-Commit-Position: refs/heads/master@{#371908} ========== to ========== Convert CookieMonster tests to use base::RunLoop CookieMonster tests post a base::MessageLoop::QuitWhenIdleClosure task which could be run at the start of some other test, since the unit tests are invoked in the same fixture on Android. This CL converts these CookieMonster tests to use base::RunLoop so they don't influence other tests. BUG=568282 Committed: https://crrev.com/b2bec813bb029f85f709860cd3eecce647df8cc1 Cr-Commit-Position: refs/heads/master@{#371908} ==========
Message was sent while issue was closed.
xunjieli@chromium.org changed reviewers: - dmoneysligh@gmail.com |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
