Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514293004:
Modernize TimerTests prior to https://codereview.chromium.org/2491613004 (Closed)
DescriptionModernize TimerTests prior to https://codereview.chromium.org/2491613004
https://codereview.chromium.org/2491613004 failed TimerTest.OneShotTimer_CustomTaskRunner
with the old tests per lacking a TaskRunnerHandle instance. Fixing that
led to a bunch of cleanup. Decoupling from that CL to make it clear that the
new tests pass in the old world and that the new world then passes the tests
without tweaking them.
Mostly moving towards RunLoop/TaskRunners/WaitableEvents to ease a few things.
Also removed incorrect DCHECK in Timer::PostNewScheduledTask() which was tripped
by the new version of TimerTest.OneShotTimer_CustomTaskRunner
BUG=587199, 552633
Committed: https://crrev.com/175490c1043e6c6ce85d42eaeb1dcfb34b2c9267
Cr-Commit-Position: refs/heads/master@{#434764}
Patch Set 1 : #
Total comments: 15
Patch Set 2 : review:danakj #
Dependent Patchsets: Messages
Total messages: 40 (27 generated)
Description was changed from ========== Modernize TimerTests prior to https://codereview.chromium.org/2491613004 https://codereview.chromium.org/2491613004 failed TimerTest.OneShotTimer_CustomTaskRunner with the old tests per lacking a TaskRunnerHandle instance. Fixing that led to a bunch of cleanup. Decoupling from that CL to make it clear that the new tests pass in the old world and that the new world then passes the tests without tweaking them. Also removed incorrect DCHECK in Timer::PostNewScheduledTask() which was tripped by the new version of TimerTest.OneShotTimer_CustomTaskRunner BUG=587199, 552633 ========== to ========== Modernize TimerTests prior to https://codereview.chromium.org/2491613004 https://codereview.chromium.org/2491613004 failed TimerTest.OneShotTimer_CustomTaskRunner with the old tests per lacking a TaskRunnerHandle instance. Fixing that led to a bunch of cleanup. Decoupling from that CL to make it clear that the new tests pass in the old world and that the new world then passes the tests without tweaking them. Mostly moving towards RunLoop/TaskRunners/WaitableEvents to ease a few things. Also removed incorrect DCHECK in Timer::PostNewScheduledTask() which was tripped by the new version of TimerTest.OneShotTimer_CustomTaskRunner BUG=587199, 552633 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by gab@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...
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTAL, extracted test refactor from https://codereview.chromium.org/2491613004/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:70: timer_->SetTaskRunner(task_runner); move https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:72: quit_closure_ = Bind(IgnoreResult(&SingleThreadTaskRunner::PostTask), It took me a while to understand this, I thought it was trying to give tasks a chance to run inbetween or something. A comment like .. if timer_ is bound to another task runner, Run() may run on a different thread from the control thread where RunLoop is being used. The timer should cause the RunLoop to exit, but it needs to do it by posting back to the control thread in that case. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:74: std::move(quit_closure_)); It would be less "Hmmmmmm" I think if you just used run_loop_.QuitClosure() here because right now this is like recursive and I was like really wondering what you're maybe going to do calling this more than once to get multiple levels of nested quit closures. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:248: // |a| should have fired despired |b| starting after it on the same sequence should not have fired? despite? https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:398: EXPECT_FALSE(did_run.IsSignaled()); This is flake if this thread stalls until the other thread signals.. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:406: EXPECT_FALSE(f.IsRunning()); Hm, this feels like it could be racey too? Like is it not that you are testing that inside the method bound to the timer, IsRunning() is already false when it runs? But you're doing it with a signal to another thread instead of just doing it inside the bound method? https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:620: } // namespace why not this at the bottom so the whole file is base{anon{}}
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Thanks, done, PTAnL. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:70: timer_->SetTaskRunner(task_runner); On 2016/11/23 19:47:10, danakj wrote: > move Done. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:72: quit_closure_ = Bind(IgnoreResult(&SingleThreadTaskRunner::PostTask), On 2016/11/23 19:47:09, danakj wrote: > It took me a while to understand this, I thought it was trying to give tasks a > chance to run inbetween or something. > > A comment like .. if timer_ is bound to another task runner, Run() may run on a > different thread from the control thread where RunLoop is being used. The timer > should cause the RunLoop to exit, but it needs to do it by posting back to the > control thread in that case. Done. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:74: std::move(quit_closure_)); On 2016/11/23 19:47:10, danakj wrote: > It would be less "Hmmmmmm" I think if you just used run_loop_.QuitClosure() here > because right now this is like recursive and I was like really wondering what > you're maybe going to do calling this more than once to get multiple levels of > nested quit closures. Done. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:248: // |a| should have fired despired |b| starting after it on the same sequence On 2016/11/23 19:47:10, danakj wrote: > should not have fired? despite? Oops, many typos... done! https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:398: EXPECT_FALSE(did_run.IsSignaled()); On 2016/11/23 19:47:09, danakj wrote: > This is flake if this thread stalls until the other thread signals.. Good catch, removed since Run() already verifies that the timing elapsed before it fired. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:406: EXPECT_FALSE(f.IsRunning()); On 2016/11/23 19:47:10, danakj wrote: > Hm, this feels like it could be racey too? Like is it not that you are testing > that inside the method bound to the timer, IsRunning() is already false when it > runs? But you're doing it with a signal to another thread instead of just doing > it inside the bound method? This is testing that anything that sequentially happens-after Run() sees the timer as not running. It isn't racy or that'd be a bug. https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:620: } // namespace On 2016/11/23 19:47:10, danakj wrote: > why not this at the bottom so the whole file is base{anon{}} I'm not sure what the original reason for this was but I've been putting fixtures in the anon namespace and tests outside of it for years... Any reason to do different?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:50001) has been deleted
The CQ bit was checked by gab@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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) 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) chromeos_x86-generic_chromium_compile_only_ng 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_compile_dbg_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_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by gab@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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@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.
ping danakj, thanks
LGTM https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:620: } // namespace On 2016/11/24 04:14:17, gab wrote: > On 2016/11/23 19:47:10, danakj wrote: > > why not this at the bottom so the whole file is base{anon{}} > > I'm not sure what the original reason for this was but I've been putting > fixtures in the anon namespace and tests outside of it for years... Any reason > to do different? Just simpler IMO, its hard to put something outside the anon namespace by mistake if it's the whole file.
On 2016/11/28 20:59:49, danakj wrote: > LGTM > > https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... > File base/timer/timer_unittest.cc (right): > > https://codereview.chromium.org/2514293004/diff/40001/base/timer/timer_unitte... > base/timer/timer_unittest.cc:620: } // namespace > On 2016/11/24 04:14:17, gab wrote: > > On 2016/11/23 19:47:10, danakj wrote: > > > why not this at the bottom so the whole file is base{anon{}} > > > > I'm not sure what the original reason for this was but I've been putting > > fixtures in the anon namespace and tests outside of it for years... Any reason > > to do different? > > Just simpler IMO, its hard to put something outside the anon namespace by > mistake if it's the whole file. Is it possible I remember hearing that tests being outside the anon-namespace allows detecting naming conflicts at link-time instead of runtime?
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On Mon, Nov 28, 2016 at 1:06 PM, <gab@chromium.org> wrote: > On 2016/11/28 20:59:49, danakj wrote: > > LGTM > > > > > https://codereview.chromium.org/2514293004/diff/40001/ > base/timer/timer_unittest.cc > > File base/timer/timer_unittest.cc (right): > > > > > https://codereview.chromium.org/2514293004/diff/40001/ > base/timer/timer_unittest.cc#newcode620 > > base/timer/timer_unittest.cc:620: } // namespace > > On 2016/11/24 04:14:17, gab wrote: > > > On 2016/11/23 19:47:10, danakj wrote: > > > > why not this at the bottom so the whole file is base{anon{}} > > > > > > I'm not sure what the original reason for this was but I've been > putting > > > fixtures in the anon namespace and tests outside of it for years... Any > reason > > > to do different? > > > > Just simpler IMO, its hard to put something outside the anon namespace by > > mistake if it's the whole file. > > Is it possible I remember hearing that tests being outside the > anon-namespace > allows detecting naming conflicts at link-time instead of runtime? > I do recall something about tests outside the anon namespace being useful but also that it was such a rare situation. You can't have conflicts if everything is in anon space right, so must be something else? > > https://codereview.chromium.org/2514293004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/28 21:10:17, danakj wrote: > On Mon, Nov 28, 2016 at 1:06 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > > On 2016/11/28 20:59:49, danakj wrote: > > > LGTM > > > > > > > > https://codereview.chromium.org/2514293004/diff/40001/ > > base/timer/timer_unittest.cc > > > File base/timer/timer_unittest.cc (right): > > > > > > > > https://codereview.chromium.org/2514293004/diff/40001/ > > base/timer/timer_unittest.cc#newcode620 > > > base/timer/timer_unittest.cc:620: } // namespace > > > On 2016/11/24 04:14:17, gab wrote: > > > > On 2016/11/23 19:47:10, danakj wrote: > > > > > why not this at the bottom so the whole file is base{anon{}} > > > > > > > > I'm not sure what the original reason for this was but I've been > > putting > > > > fixtures in the anon namespace and tests outside of it for years... Any > > reason > > > > to do different? > > > > > > Just simpler IMO, its hard to put something outside the anon namespace by > > > mistake if it's the whole file. > > > > Is it possible I remember hearing that tests being outside the > > anon-namespace > > allows detecting naming conflicts at link-time instead of runtime? > > > > I do recall something about tests outside the anon namespace being useful > but also that it was such a rare situation. You can't have conflicts if > everything is in anon space right, so must be something else? But the names could still conflict at runtime (i.e. --gtest_filter?)
On Mon, Nov 28, 2016 at 1:13 PM, <gab@chromium.org> wrote: > On 2016/11/28 21:10:17, danakj wrote: > > On Mon, Nov 28, 2016 at 1:06 PM, > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> > wrote: > > > > > On 2016/11/28 20:59:49, danakj wrote: > > > > LGTM > > > > > > > > > > > https://codereview.chromium.org/2514293004/diff/40001/ > > > base/timer/timer_unittest.cc > > > > File base/timer/timer_unittest.cc (right): > > > > > > > > > > > https://codereview.chromium.org/2514293004/diff/40001/ > > > base/timer/timer_unittest.cc#newcode620 > > > > base/timer/timer_unittest.cc:620: } // namespace > > > > On 2016/11/24 04:14:17, gab wrote: > > > > > On 2016/11/23 19:47:10, danakj wrote: > > > > > > why not this at the bottom so the whole file is base{anon{}} > > > > > > > > > > I'm not sure what the original reason for this was but I've been > > > putting > > > > > fixtures in the anon namespace and tests outside of it for > years... Any > > > reason > > > > > to do different? > > > > > > > > Just simpler IMO, its hard to put something outside the anon > namespace by > > > > mistake if it's the whole file. > > > > > > Is it possible I remember hearing that tests being outside the > > > anon-namespace > > > allows detecting naming conflicts at link-time instead of runtime? > > > > > > > I do recall something about tests outside the anon namespace being useful > > but also that it was such a rare situation. You can't have conflicts if > > everything is in anon space right, so must be something else? > > But the names could still conflict at runtime (i.e. --gtest_filter?) > Oh.. I suppose so. Maybe that's what it was which seemed unlikely to me since the test names are based on class which is in the file name, so u'd have file-name collisions too. But yah. > > https://codereview.chromium.org/2514293004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/28 21:30:26, danakj wrote: > On Mon, Nov 28, 2016 at 1:13 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > > On 2016/11/28 21:10:17, danakj wrote: > > > On Mon, Nov 28, 2016 at 1:06 PM, > > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> > > wrote: > > > > > > > On 2016/11/28 20:59:49, danakj wrote: > > > > > LGTM > > > > > > > > > > > > > > https://codereview.chromium.org/2514293004/diff/40001/ > > > > base/timer/timer_unittest.cc > > > > > File base/timer/timer_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2514293004/diff/40001/ > > > > base/timer/timer_unittest.cc#newcode620 > > > > > base/timer/timer_unittest.cc:620: } // namespace > > > > > On 2016/11/24 04:14:17, gab wrote: > > > > > > On 2016/11/23 19:47:10, danakj wrote: > > > > > > > why not this at the bottom so the whole file is base{anon{}} > > > > > > > > > > > > I'm not sure what the original reason for this was but I've been > > > > putting > > > > > > fixtures in the anon namespace and tests outside of it for > > years... Any > > > > reason > > > > > > to do different? > > > > > > > > > > Just simpler IMO, its hard to put something outside the anon > > namespace by > > > > > mistake if it's the whole file. > > > > > > > > Is it possible I remember hearing that tests being outside the > > > > anon-namespace > > > > allows detecting naming conflicts at link-time instead of runtime? > > > > > > > > > > I do recall something about tests outside the anon namespace being useful > > > but also that it was such a rare situation. You can't have conflicts if > > > everything is in anon space right, so must be something else? > > > > But the names could still conflict at runtime (i.e. --gtest_filter?) > > > > Oh.. I suppose so. Maybe that's what it was which seemed unlikely to me > since the test names are based on class which is in the file name, so u'd > have file-name collisions too. But yah. Right, maybe I'll stop bothering :)
CQ is committing da patch.
Bot data: {"patchset_id": 70001, "attempt_start_ts": 1480367223413160,
"parent_rev": "ca09181bcbf30e99dbdf39ddf2f17eca90b0822c", "commit_rev":
"6410756988b35a801689b3be8ac525b7a85e5216"}
Message was sent while issue was closed.
Description was changed from ========== Modernize TimerTests prior to https://codereview.chromium.org/2491613004 https://codereview.chromium.org/2491613004 failed TimerTest.OneShotTimer_CustomTaskRunner with the old tests per lacking a TaskRunnerHandle instance. Fixing that led to a bunch of cleanup. Decoupling from that CL to make it clear that the new tests pass in the old world and that the new world then passes the tests without tweaking them. Mostly moving towards RunLoop/TaskRunners/WaitableEvents to ease a few things. Also removed incorrect DCHECK in Timer::PostNewScheduledTask() which was tripped by the new version of TimerTest.OneShotTimer_CustomTaskRunner BUG=587199, 552633 ========== to ========== Modernize TimerTests prior to https://codereview.chromium.org/2491613004 https://codereview.chromium.org/2491613004 failed TimerTest.OneShotTimer_CustomTaskRunner with the old tests per lacking a TaskRunnerHandle instance. Fixing that led to a bunch of cleanup. Decoupling from that CL to make it clear that the new tests pass in the old world and that the new world then passes the tests without tweaking them. Mostly moving towards RunLoop/TaskRunners/WaitableEvents to ease a few things. Also removed incorrect DCHECK in Timer::PostNewScheduledTask() which was tripped by the new version of TimerTest.OneShotTimer_CustomTaskRunner BUG=587199, 552633 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Modernize TimerTests prior to https://codereview.chromium.org/2491613004 https://codereview.chromium.org/2491613004 failed TimerTest.OneShotTimer_CustomTaskRunner with the old tests per lacking a TaskRunnerHandle instance. Fixing that led to a bunch of cleanup. Decoupling from that CL to make it clear that the new tests pass in the old world and that the new world then passes the tests without tweaking them. Mostly moving towards RunLoop/TaskRunners/WaitableEvents to ease a few things. Also removed incorrect DCHECK in Timer::PostNewScheduledTask() which was tripped by the new version of TimerTest.OneShotTimer_CustomTaskRunner BUG=587199, 552633 ========== to ========== Modernize TimerTests prior to https://codereview.chromium.org/2491613004 https://codereview.chromium.org/2491613004 failed TimerTest.OneShotTimer_CustomTaskRunner with the old tests per lacking a TaskRunnerHandle instance. Fixing that led to a bunch of cleanup. Decoupling from that CL to make it clear that the new tests pass in the old world and that the new world then passes the tests without tweaking them. Mostly moving towards RunLoop/TaskRunners/WaitableEvents to ease a few things. Also removed incorrect DCHECK in Timer::PostNewScheduledTask() which was tripped by the new version of TimerTest.OneShotTimer_CustomTaskRunner BUG=587199, 552633 Committed: https://crrev.com/175490c1043e6c6ce85d42eaeb1dcfb34b2c9267 Cr-Commit-Position: refs/heads/master@{#434764} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/175490c1043e6c6ce85d42eaeb1dcfb34b2c9267 Cr-Commit-Position: refs/heads/master@{#434764} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
