Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2223063005:
Some changes to the testing infrastructure in find_request_manager_browsertest.cc. (Closed)
DescriptionSome changes to the testing infrastructure in find_request_manager_browsertest.cc.
These changes will improve on some (very) occasional flakyness that I've noticed in the tests in there. They are also completely necessary for a test to work that will be added soon in an upcoming CL.
BUG=621660
Committed: https://crrev.com/2f06e61927d77b49240266b75710908c8bf3710b
Cr-Commit-Position: refs/heads/master@{#411071}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments. #Messages
Total messages: 20 (13 generated)
Description was changed from ========== Some changes to the testing infrastructure in find_request_manager_browsertest.cc. These changes will improve on some (very) occasional flakyness that I've noticed in the tests in there. They are also completely necessary for a test to work that will be added soon in an upcoming CL. ========== to ========== Some changes to the testing infrastructure in find_request_manager_browsertest.cc. These changes will improve on some (very) occasional flakyness that I've noticed in the tests in there. They are also completely necessary for a test to work that will be added soon in an upcoming CL. BUG=621660 ==========
paulmeyer@chromium.org changed reviewers: + lfg@chromium.org
+lfg@
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2223063005/diff/1/content/browser/find_reques... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/2223063005/diff/1/content/browser/find_reques... content/browser/find_request_manager_browsertest.cc:144: message_loop_runner_ = new content::MessageLoopRunner; This is a bit strange. You just called Quit() on the message_loop, and you destroy it right after? Or am I missing something and there's something else holding the refptr to the old message loop? Also, it seems that if WaitFor() is called from inside the message loop (with something other than NOTHING) this will run a nested message loop which is really bad.
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2223063005/diff/1/content/browser/find_reques... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/2223063005/diff/1/content/browser/find_reques... content/browser/find_request_manager_browsertest.cc:144: message_loop_runner_ = new content::MessageLoopRunner; On 2016/08/10 03:19:16, lfg wrote: > This is a bit strange. You just called Quit() on the message_loop, and you > destroy it right after? Or am I missing something and there's something else > holding the refptr to the old message loop? > > Also, it seems that if WaitFor() is called from inside the message loop (with > something other than NOTHING) this will run a nested message loop which is > really bad. You're right, it is strange. Based on how the function is used, these problems don't come up, but it doesn't explicitly guarantee that, and it looks weird. I've cleaned it up by splitting the stop waiting part into its own function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm
The CQ bit was checked by paulmeyer@chromium.org
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.
Description was changed from ========== Some changes to the testing infrastructure in find_request_manager_browsertest.cc. These changes will improve on some (very) occasional flakyness that I've noticed in the tests in there. They are also completely necessary for a test to work that will be added soon in an upcoming CL. BUG=621660 ========== to ========== Some changes to the testing infrastructure in find_request_manager_browsertest.cc. These changes will improve on some (very) occasional flakyness that I've noticed in the tests in there. They are also completely necessary for a test to work that will be added soon in an upcoming CL. BUG=621660 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Some changes to the testing infrastructure in find_request_manager_browsertest.cc. These changes will improve on some (very) occasional flakyness that I've noticed in the tests in there. They are also completely necessary for a test to work that will be added soon in an upcoming CL. BUG=621660 ========== to ========== Some changes to the testing infrastructure in find_request_manager_browsertest.cc. These changes will improve on some (very) occasional flakyness that I've noticed in the tests in there. They are also completely necessary for a test to work that will be added soon in an upcoming CL. BUG=621660 Committed: https://crrev.com/2f06e61927d77b49240266b75710908c8bf3710b Cr-Commit-Position: refs/heads/master@{#411071} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2f06e61927d77b49240266b75710908c8bf3710b Cr-Commit-Position: refs/heads/master@{#411071} |
