Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563273002:
Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that (Closed)
|
Created:
4 years ago by Zhongyi Shi Modified:
4 years ago Reviewers:
Ryan Hamilton CC:
chromium-reviews, cbentzel+watch_chromium.org, xunjieli Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange the HttpServerPropertiesManager::net_task_runner_ to use a TestMockTimeTaskRunner in unittests so that
we are able to verify tasks posted with delay in the future.
Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread
will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all
the tasks complete running.
BUG=670519
Committed: https://crrev.com/65aa8f6576dce9b163c7c53fe5aecc9a1b44c00b
Cr-Commit-Position: refs/heads/master@{#437980}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move SetTaskRunner to HttpServerPropertiesManager #
Total comments: 8
Patch Set 3 : address rch's comments #
Messages
Total messages: 40 (29 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. BUG=670519 ========== to ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_->FastForwardUntilNoTasksRemain() so that all the tasks completes running. BUG=670519 ==========
Description was changed from ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_->FastForwardUntilNoTasksRemain() so that all the tasks completes running. BUG=670519 ========== to ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_->FastForwardUntilNoTasksRemain() so that all the tasks completes running. BUG=670519 ==========
Description was changed from ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_->FastForwardUntilNoTasksRemain() so that all the tasks completes running. BUG=670519 ========== to ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks completes running. BUG=670519 ==========
Description was changed from ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks completes running. BUG=670519 ========== to ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks completes running. BUG=670519 ==========
Description was changed from ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks completes running. BUG=670519 ========== to ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 ==========
zhongyi@chromium.org changed reviewers: + rch@chromium.org
The CQ bit was checked by zhongyi@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Can you mention HttpServerProperties in the CL description (and title) so there's more context? https://codereview.chromium.org/2563273002/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/2563273002/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:176: http_server_props_manager_.get(), net_test_task_runner_); Instead of doing this in the test, can you change HttpServerPropertiesManager to set the timer's task runner to the net_task_runner_? https://codereview.chromium.org/2563273002/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:378: base::RunLoop().RunUntilIdle(); base::RunLoop().RunUntilIdle(); runs the main message loop. Since you're using a test task runner, I don't think you need to run this. Does the code work if you leave it off? (If not, I guess that's the pref's thread's runner?)
Description was changed from ========== Change the net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 ========== to ========== Change the HttpServerPropertiesManager::net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 ==========
Description was changed from ========== Change the HttpServerPropertiesManager::net_task_runner to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 ========== to ========== Change the HttpServerPropertiesManager::net_task_runner_ to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 ==========
https://codereview.chromium.org/2563273002/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/2563273002/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:378: base::RunLoop().RunUntilIdle(); On 2016/12/11 19:36:57, Ryan Hamilton wrote: > base::RunLoop().RunUntilIdle(); runs the main message loop. Since you're using a > test task runner, I don't think you need to run this. Does the code work if you > leave it off? (If not, I guess that's the pref's thread's runner?) Yup. This is the pref's thread's runner. And in the next CL, I will add another TestMockTimeTaskRunner, which replaces the pref's task runner. And we won't calling base::RunLoop() any more.
The CQ bit was checked by zhongyi@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...
zhongyi@chromium.org changed reviewers: + xunjieli@chromium.org
Updated: move the SetTaskRunner to HttpServerProperties. https://codereview.chromium.org/2563273002/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/2563273002/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:176: http_server_props_manager_.get(), net_test_task_runner_); On 2016/12/11 19:36:57, Ryan Hamilton wrote: > Instead of doing this in the test, can you change HttpServerPropertiesManager to > set the timer's task runner to the net_task_runner_? Done.
Just a couple nits. LGTM otherwise. https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... net/http/http_server_properties_manager.cc:103: network_prefs_update_timer_->SetTaskRunner(std::move(network_task_runner_)); Do you need to move the task runner? It's a scoped_refptr, so I think it's copyable. I'm not sure what std::move does in this case, but it kinda seems like it might leave network_task_runner_ set to null. https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... net/http/http_server_properties_manager.h:240: friend class HttpServerPropertiesManagerPeer; Is this still needed? https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:225: scoped_refptr<TestMockTimeTaskRunner> net_test_task_runner_; Just confirming, there are no new tests in the CL, just the plumbing to test the timer? https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:631: net_test_task_runner_->FastForwardUntilNoTasksRemain(); nit: sometimes there are newlines after this call and sometimes not. Either way sounds fine to me, so pick whichever you prefer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
looks good! Deferring this CL to rch. Mocking out the task runner does seem cleaner than mocking out the timer. It'll be very nice to get rid of base::RunLoop().RunUntilIdle() once the other task runner is switched over.
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by zhongyi@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
zhongyi@chromium.org changed reviewers: - xunjieli@chromium.org
Thanks a lot for helping the review. Ryan, all the nits are addresses. I will land this CL if no objection raised. Moving xunjieli@ to cc. https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... net/http/http_server_properties_manager.cc:103: network_prefs_update_timer_->SetTaskRunner(std::move(network_task_runner_)); On 2016/12/12 05:30:20, Ryan Hamilton wrote: > Do you need to move the task runner? It's a scoped_refptr, so I think it's > copyable. I'm not sure what std::move does in this case, but it kinda seems like > it might leave network_task_runner_ set to null. Done. https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... net/http/http_server_properties_manager.h:240: friend class HttpServerPropertiesManagerPeer; On 2016/12/12 05:30:20, Ryan Hamilton wrote: > Is this still needed? Done. Sorry for leaving this around. https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... File net/http/http_server_properties_manager_unittest.cc (right): https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:225: scoped_refptr<TestMockTimeTaskRunner> net_test_task_runner_; On 2016/12/12 05:30:20, Ryan Hamilton wrote: > Just confirming, there are no new tests in the CL, just the plumbing to test the > timer? Yup. New tests will come in later CLs. https://codereview.chromium.org/2563273002/diff/40001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:631: net_test_task_runner_->FastForwardUntilNoTasksRemain(); On 2016/12/12 05:30:20, Ryan Hamilton wrote: > nit: sometimes there are newlines after this call and sometimes not. Either way > sounds fine to me, so pick whichever you prefer. Done.
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/2563273002/#ps80001 (title: "address rch's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Looks great! LGTM
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481582164396590,
"parent_rev": "d2e1c86892b6969004493407a61796e63ac58c3f", "commit_rev":
"7952fbd460ca354119d098ef5a217d1e70d9b93a"}
Message was sent while issue was closed.
Description was changed from ========== Change the HttpServerPropertiesManager::net_task_runner_ to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 ========== to ========== Change the HttpServerPropertiesManager::net_task_runner_ to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 Review-Url: https://codereview.chromium.org/2563273002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Change the HttpServerPropertiesManager::net_task_runner_ to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 Review-Url: https://codereview.chromium.org/2563273002 ========== to ========== Change the HttpServerPropertiesManager::net_task_runner_ to use a TestMockTimeTaskRunner in unittests so that we are able to verify tasks posted with delay in the future. Now all the tests that were ExpectCacheUpdate or ExpectScheduleUpdatePrefsOnNetworkThread will need to call net_test_task_runner_>FastForwardUntilNoTasksRemain() so that all the tasks complete running. BUG=670519 Committed: https://crrev.com/65aa8f6576dce9b163c7c53fe5aecc9a1b44c00b Cr-Commit-Position: refs/heads/master@{#437980} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/65aa8f6576dce9b163c7c53fe5aecc9a1b44c00b Cr-Commit-Position: refs/heads/master@{#437980} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
