|
|
Created:
4 years, 8 months ago by fdoray Modified:
4 years, 8 months ago CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTaskScheduler: Move TaskFactory out of scheduler_thread_pool_unittest.cc
This will allow the factory to be reused to test TaskSchedulerImpl.
TBR=danakj@chromium.org
BUG=553459
Committed: https://crrev.com/44fda214399b68ac4a4d7cc07feaf52253c59e7e
Cr-Commit-Position: refs/heads/master@{#389491}
Patch Set 1 #
Total comments: 8
Patch Set 2 : CR robliao #3 (rename enum, expect in test file rather than util file, namespace test) #Patch Set 3 : added comments #Patch Set 4 : self review #Patch Set 5 : rebase #
Messages
Total messages: 30 (12 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
Can you review this CL? Thanks.
https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_thread_pool_unittest.cc:80: YES, Maybe this can be enum class ThreadIdleBehavior { WAIT_ALL, IGNORE, (Can't think of a better name here at the moment). } https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... File base/task_scheduler/test_task_factory.cc (right): https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... base/task_scheduler/test_task_factory.cc:33: EXPECT_TRUE(task_runner_->PostTask( Given that this is a utility, EXPECT_TRUE is probably less appropriate. The failure modes below also deserve reconsideration. It might be useful to allow consumers to specify options on what failure modes to report and what to check. https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... File base/task_scheduler/test_task_factory.h (right): https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... base/task_scheduler/test_task_factory.h:28: class TestTaskFactory { This should go into the test namespace.
robliao@: Can you take another look? Note: I prefer to get reviews on these CLs first: - https://codereview.chromium.org/1892033003/ SchedulerUniqueStack - https://codereview.chromium.org/1906083002/ Remove base/task_scheduler/utils.h/.cc - https://codereview.chromium.org/1903103007/ Make SchedulerWorkerThread own its delegate. - https://codereview.chromium.org/1906813003/ TimeDelta instead of TimeTicks in Task's constructor. - https://codereview.chromium.org/1876363004/ Support ExecutionMode::SINGLE_THREADED. https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_thread_pool_unittest.cc (right): https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_thread_pool_unittest.cc:80: YES, On 2016/04/21 01:38:17, robliao wrote: > Maybe this can be > enum class ThreadIdleBehavior { > WAIT_ALL, > IGNORE, (Can't think of a better name here at the moment). > } Done. https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... File base/task_scheduler/test_task_factory.cc (right): https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... base/task_scheduler/test_task_factory.cc:33: EXPECT_TRUE(task_runner_->PostTask( On 2016/04/21 01:38:17, robliao wrote: > Given that this is a utility, EXPECT_TRUE is probably less appropriate. > > The failure modes below also deserve reconsideration. It might be useful to > allow consumers to specify options on what failure modes to report and what to > check. Moved the EXPECT_TRUE to callers. Do you suggest that we provide a way for callers to enable/disable checks done in RunTaskCallback? I don't think this is useful since all checks would be enabled all the time. We could set booleans when conditions are not met (e.g. when a task runs more than once) and let users of TestTaskFactory set expectations for the values of these booleans, but gab@ said that less state makes the class easier to read. https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... File base/task_scheduler/test_task_factory.h (right): https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... base/task_scheduler/test_task_factory.h:28: class TestTaskFactory { On 2016/04/21 01:38:17, robliao wrote: > This should go into the test namespace. Done.
gab@chromium.org changed reviewers: - gab@chromium.org
One reviewer is enough for this one? Leaving it to Rob unless requested otherwise :-)
On 2016/04/22 16:53:54, gab wrote: > One reviewer is enough for this one? Leaving it to Rob unless requested > otherwise :-) sgtm. My impression is that this is a simple refactor.
lgtm https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... File base/task_scheduler/test_task_factory.cc (right): https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... base/task_scheduler/test_task_factory.cc:33: EXPECT_TRUE(task_runner_->PostTask( On 2016/04/22 15:54:03, fdoray wrote: > On 2016/04/21 01:38:17, robliao wrote: > > Given that this is a utility, EXPECT_TRUE is probably less appropriate. > > > > The failure modes below also deserve reconsideration. It might be useful to > > allow consumers to specify options on what failure modes to report and what to > > check. > > Moved the EXPECT_TRUE to callers. > > Do you suggest that we provide a way for callers to enable/disable checks done > in RunTaskCallback? I don't think this is useful since all checks would be > enabled all the time. > > We could set booleans when conditions are not met (e.g. when a task runs more > than once) and let users of TestTaskFactory set expectations for the values of > these booleans, but gab@ said that less state makes the class easier to read. The overarching question here is would consumers of the util expect tests to fail due to internal workings of this class. Since this is our util class, we can make that decision. I tend to like to see the failures within the unit test file as it makes it easier to see how things can fail within the file, but this is just personal preference. If we have a really good reason for enforcing this upon all consumers, then so be it.
https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... File base/task_scheduler/test_task_factory.cc (right): https://codereview.chromium.org/1911493002/diff/1/base/task_scheduler/test_ta... base/task_scheduler/test_task_factory.cc:33: EXPECT_TRUE(task_runner_->PostTask( On 2016/04/22 23:04:19, robliao wrote: > On 2016/04/22 15:54:03, fdoray wrote: > > On 2016/04/21 01:38:17, robliao wrote: > > > Given that this is a utility, EXPECT_TRUE is probably less appropriate. > > > > > > The failure modes below also deserve reconsideration. It might be useful to > > > allow consumers to specify options on what failure modes to report and what > to > > > check. > > > > Moved the EXPECT_TRUE to callers. > > > > Do you suggest that we provide a way for callers to enable/disable checks done > > in RunTaskCallback? I don't think this is useful since all checks would be > > enabled all the time. > > > > We could set booleans when conditions are not met (e.g. when a task runs more > > than once) and let users of TestTaskFactory set expectations for the values of > > these booleans, but gab@ said that less state makes the class easier to read. > > The overarching question here is would consumers of the util expect tests to > fail due to internal workings of this class. > > Since this is our util class, we can make that decision. I tend to like to see > the failures within the unit test file as it makes it easier to see how things > can fail within the file, but this is just personal preference. If we have a > really good reason for enforcing this upon all consumers, then so be it. I added comments in the header file to document the failures that can be generated by this class. If I moved the failures to the test file, there would be duplication between the thread pool and task scheduler tests, which is this CL tries to avoid.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/1911493002/#ps80001 (title: "rebase")
The CQ bit was unchecked by fdoray@chromium.org
fdoray@chromium.org changed reviewers: + danakj@chromium.org
Can you review this CL? Thanks.
danakj@: Can you review this CL? Thanks.
On 2016/04/25 14:28:08, fdoray wrote: > danakj@: Can you review this CL? Thanks. I think it's okay to forgo Dana on this refactor CL (I withdrew myself for that same reason). Let's keep her skills and energy for the real stuff :-).
On 2016/04/25 15:12:21, gab wrote: > On 2016/04/25 14:28:08, fdoray wrote: > > danakj@: Can you review this CL? Thanks. > > I think it's okay to forgo Dana on this refactor CL (I withdrew myself for that > same reason). Let's keep her skills and energy for the real stuff :-). I need an owner for base/BUILD.gn and base/base.gyp danakj@: Can you review just base/BUILD.gn and base/base.gyp?
On 2016/04/25 15:13:10, fdoray wrote: > On 2016/04/25 15:12:21, gab wrote: > > On 2016/04/25 14:28:08, fdoray wrote: > > > danakj@: Can you review this CL? Thanks. > > > > I think it's okay to forgo Dana on this refactor CL (I withdrew myself for > that > > same reason). Let's keep her skills and energy for the real stuff :-). > > > I need an owner for base/BUILD.gn and base/base.gyp > > danakj@: Can you review just base/BUILD.gn and base/base.gyp? chrome/ has a rule where TBR is okay for trivial GYP/GN file +/-. Pretty sure base/ works the same way (though it's rarer for base/ to have subowners like we have in base/task_scheduler).
Description was changed from ========== TaskScheduler: Move TaskFactory out of scheduler_thread_pool_unittest.cc This will allow the factory to be reused to test TaskSchedulerImpl. BUG=553459 ========== to ========== TaskScheduler: Move TaskFactory out of scheduler_thread_pool_unittest.cc This will allow the factory to be reused to test TaskSchedulerImpl. TBR=danakj@chromium.org BUG=553459 ==========
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911493002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911493002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911493002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911493002/80001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Move TaskFactory out of scheduler_thread_pool_unittest.cc This will allow the factory to be reused to test TaskSchedulerImpl. TBR=danakj@chromium.org BUG=553459 ========== to ========== TaskScheduler: Move TaskFactory out of scheduler_thread_pool_unittest.cc This will allow the factory to be reused to test TaskSchedulerImpl. TBR=danakj@chromium.org BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Move TaskFactory out of scheduler_thread_pool_unittest.cc This will allow the factory to be reused to test TaskSchedulerImpl. TBR=danakj@chromium.org BUG=553459 ========== to ========== TaskScheduler: Move TaskFactory out of scheduler_thread_pool_unittest.cc This will allow the factory to be reused to test TaskSchedulerImpl. TBR=danakj@chromium.org BUG=553459 Committed: https://crrev.com/44fda214399b68ac4a4d7cc07feaf52253c59e7e Cr-Commit-Position: refs/heads/master@{#389491} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/44fda214399b68ac4a4d7cc07feaf52253c59e7e Cr-Commit-Position: refs/heads/master@{#389491}
Message was sent while issue was closed.
LGTM |