Use a TickClock instead of TimeTicks::Now() in Timer.

Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can provide a mock implementation
of the TickClock that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077

Committed: https://crrev.com/6e0c8d22b864d84946417eb6379c1513c36d48b0
Cr-Commit-Position: refs/heads/master@{#432113}

[jameswest, 2016-11-07 23:17:39]

Description was changed from

==========
Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can replace the TickClock with a
mock implementation that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077

Change-Id: I7f79c68454c5543293072b226506d756e4e3fbbc
==========

to

==========
Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can replace the TickClock with a
mock implementation that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077

Change-Id: I7f79c68454c5543293072b226506d756e4e3fbbc
==========

[jameswest, 2016-11-07 23:17:39]

jameswest@google.com changed reviewers:
+ avi@chromium.org, danakj@chromium.org

[Avi (use Gerrit), 2016-11-08 16:25:34]

avi@chromium.org changed reviewers:
- avi@chromium.org

[Avi (use Gerrit), 2016-11-08 16:25:35]

Yes, this is cool, but my OK doesn't matter here so I'll wander off.

[Avi (use Gerrit), 2016-11-08 16:27:24]

On 2016/11/08 16:25:35, Avi wrote:
> Yes, this is cool, but my OK doesn't matter here so I'll wander off.

Wait, sorry, I was asked on a different CL to remove myself if I wanted to, and
I confused it with this CL.

In any case, this is base/. LGTM for what it's worth, but I don't think my LG
helps.

[danakj, 2016-11-09 19:57:38]

https://codereview.chromium.org/2484023002/diff/1/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2484023002/diff/1/base/timer/timer.h#newcode100
base/timer/timer.h:100: virtual void SetTickClock(std::unique_ptr<TickClock>
tick_clock);
This class being virtual means subclasses need to use the tick clock too.

Here's one example I found through codesearch:
https://cs.chromium.org/chromium/src/components/timers/alarm_timer_chromeos.c...

How can we help ensure subclasses do the right thing?

Here's one idea:
1. Add a protected pure-virtual-but-implemented method Now() that calls
TimeTicks::Now(). (looks like "virtual base::TimeTicks Now() = 0;" and a defn of
that in the .cc file)
2. Subclasses will not compile without making a definition of Now() themselves.
Production can call the base class (and convert direct calls to TimeTicks::Now()
to "Now()" in the process. Tests can do whatever they like.

What do you think?

[jameswest, 2016-11-10 00:26:05]

https://codereview.chromium.org/2484023002/diff/1/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2484023002/diff/1/base/timer/timer.h#newcode100
base/timer/timer.h:100: virtual void SetTickClock(std::unique_ptr<TickClock>
tick_clock);
On 2016/11/09 19:57:38, danakj wrote:
> This class being virtual means subclasses need to use the tick clock too.
> 
> Here's one example I found through codesearch:
>
https://cs.chromium.org/chromium/src/components/timers/alarm_timer_chromeos.c...
> 
> How can we help ensure subclasses do the right thing?
> 
> Here's one idea:
> 1. Add a protected pure-virtual-but-implemented method Now() that calls
> TimeTicks::Now(). (looks like "virtual base::TimeTicks Now() = 0;" and a defn
of
> that in the .cc file)
> 2. Subclasses will not compile without making a definition of Now()
themselves.
> Production can call the base class (and convert direct calls to
TimeTicks::Now()
> to "Now()" in the process. Tests can do whatever they like.
> 
> What do you think?

I'm fine with that idea, but it's currently possible to instantiate Timer
directly because it doesn't have any pure virtual methods. If we add the pure
virtual method, that will no longer be possible. I don't think anything directly
instantiates Timer other than its unit tests, but are we OK with requiring only
subclasses be used?

[danakj, 2016-11-10 01:31:27]

On Wed, Nov 9, 2016 at 4:26 PM, <jameswest@google.com> wrote:

>
> https://codereview.chromium.org/2484023002/diff/1/base/timer/timer.h
> File base/timer/timer.h (right):
>
> https://codereview.chromium.org/2484023002/diff/1/base/
> timer/timer.h#newcode100
> base/timer/timer.h:100: virtual void
> SetTickClock(std::unique_ptr<TickClock> tick_clock);
> On 2016/11/09 19:57:38, danakj wrote:
> > This class being virtual means subclasses need to use the tick clock
> too.
> >
> > Here's one example I found through codesearch:
> >
> https://cs.chromium.org/chromium/src/components/
> timers/alarm_timer_chromeos.cc?rcl=1478693914&l=74
> >
> > How can we help ensure subclasses do the right thing?
> >
> > Here's one idea:
> > 1. Add a protected pure-virtual-but-implemented method Now() that
> calls
> > TimeTicks::Now(). (looks like "virtual base::TimeTicks Now() = 0;" and
> a defn of
> > that in the .cc file)
> > 2. Subclasses will not compile without making a definition of Now()
> themselves.
> > Production can call the base class (and convert direct calls to
> TimeTicks::Now()
> > to "Now()" in the process. Tests can do whatever they like.
> >
> > What do you think?
>
> I'm fine with that idea, but it's currently possible to instantiate
> Timer directly because it doesn't have any pure virtual methods. If we
> add the pure virtual method, that will no longer be possible. I don't
> think anything directly instantiates Timer other than its unit tests,
> but are we OK with requiring only subclasses be used?
>

Ah, right that's not so good. I was thinking in terms of forcing subclasses
to pay attention, but not at the expense of requiring a subclass.. maybe
you can just do a careful audit to make sure you get all the subclasses'
use of TimeTicks::Now? Or do you have any other ideas that could help?


>
> https://codereview.chromium.org/2484023002/
>

-- 
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.

[jameswest, 2016-11-10 02:20:52]

> Ah, right that's not so good. I was thinking in terms of forcing subclasses
> to pay attention, but not at the expense of requiring a subclass.. maybe
> you can just do a careful audit to make sure you get all the subclasses'
> use of TimeTicks::Now? Or do you have any other ideas that could help?

I can add a ForTesting suffix to SetTickClock() and make it non-virtual so that
it can only be used for tests. In production, it would always use
DefaultTickClock, which uses TimeTicks::Now(), so it would work correctly in
production even if a subclass uses TimeTicks::Now(). I can then add a protected
non-virtual Now() method to Timer so that subclasses can get the time from the
TickClock. If a subclass uses TimeTicks::Now() instead of Timer::Now(), it
should only affect any unit tests that use SetTickClockForTesting().

The only subclasses I found outside of timer.h were in base/timer/mock_timer.h
and components/timers/alarm_timer_chromeos.h.

[jameswest, 2016-11-10 02:52:34]

On 2016/11/10 02:20:52, jameswest wrote:
> > Ah, right that's not so good. I was thinking in terms of forcing subclasses
> > to pay attention, but not at the expense of requiring a subclass.. maybe
> > you can just do a careful audit to make sure you get all the subclasses'
> > use of TimeTicks::Now? Or do you have any other ideas that could help?
> 
> I can add a ForTesting suffix to SetTickClock() and make it non-virtual so
that
> it can only be used for tests. In production, it would always use
> DefaultTickClock, which uses TimeTicks::Now(), so it would work correctly in
> production even if a subclass uses TimeTicks::Now(). I can then add a
protected
> non-virtual Now() method to Timer so that subclasses can get the time from the
> TickClock. If a subclass uses TimeTicks::Now() instead of Timer::Now(), it
> should only affect any unit tests that use SetTickClockForTesting().
> 
> The only subclasses I found outside of timer.h were in base/timer/mock_timer.h
> and components/timers/alarm_timer_chromeos.h.

Actually, I realized I'd rather not use the ForTesting suffix because we have at
least one class that uses a TickClock and creates Timers, and I'd like for it to
always set the timers to use the TickClock that the class is using.

[jameswest, 2016-11-10 03:06:11]

On 2016/11/10 02:52:34, jameswest wrote:
> On 2016/11/10 02:20:52, jameswest wrote:
> > > Ah, right that's not so good. I was thinking in terms of forcing
subclasses
> > > to pay attention, but not at the expense of requiring a subclass.. maybe
> > > you can just do a careful audit to make sure you get all the subclasses'
> > > use of TimeTicks::Now? Or do you have any other ideas that could help?
> > 
> > I can add a ForTesting suffix to SetTickClock() and make it non-virtual so
> that
> > it can only be used for tests. In production, it would always use
> > DefaultTickClock, which uses TimeTicks::Now(), so it would work correctly in
> > production even if a subclass uses TimeTicks::Now(). I can then add a
> protected
> > non-virtual Now() method to Timer so that subclasses can get the time from
the
> > TickClock. If a subclass uses TimeTicks::Now() instead of Timer::Now(), it
> > should only affect any unit tests that use SetTickClockForTesting().
> > 
> > The only subclasses I found outside of timer.h were in
base/timer/mock_timer.h
> > and components/timers/alarm_timer_chromeos.h.
> 
> Actually, I realized I'd rather not use the ForTesting suffix because we have
at
> least one class that uses a TickClock and creates Timers, and I'd like for it
to
> always set the timers to use the TickClock that the class is using.

One option would be to add a new constructor that takes a TickClock instead of a
setter. If any subclasses wanted to support using a different TickClock, they
would also need to add a constructor that takes a TickClock and pass it through
to the base class.

Also, how do you feel about passing a raw pointer to a TickClock versus passing
a unique_ptr to a TickClock? We will sometimes need to use the same TickClock
for multiple Timers or for a Timer and the class that owns it. The two options I
can think of to handle that case are either pass a raw pointer to Timer and use
TimeTicks::Now() if the pointer is null or create a new TickClock implementation
that takes a raw pointer to another TickClock, similar to MockTickClock in
base/test/test_mock_time_task_runner.cc.

[danakj, 2016-11-12 00:42:59]

On Wed, Nov 9, 2016 at 7:06 PM, <jameswest@google.com> wrote:

> On 2016/11/10 02:52:34, jameswest wrote:
> > On 2016/11/10 02:20:52, jameswest wrote:
> > > > Ah, right that's not so good. I was thinking in terms of forcing
> subclasses
> > > > to pay attention, but not at the expense of requiring a subclass..
> maybe
> > > > you can just do a careful audit to make sure you get all the
> subclasses'
> > > > use of TimeTicks::Now? Or do you have any other ideas that could
> help?
> > >
> > > I can add a ForTesting suffix to SetTickClock() and make it
> non-virtual so
> > that
> > > it can only be used for tests. In production, it would always use
> > > DefaultTickClock, which uses TimeTicks::Now(), so it would work
> correctly in
> > > production even if a subclass uses TimeTicks::Now(). I can then add a
> > protected
> > > non-virtual Now() method to Timer so that subclasses can get the time
> from
> the
> > > TickClock. If a subclass uses TimeTicks::Now() instead of
> Timer::Now(), it
> > > should only affect any unit tests that use SetTickClockForTesting().
> > >
> > > The only subclasses I found outside of timer.h were in
> base/timer/mock_timer.h
> > > and components/timers/alarm_timer_chromeos.h.
> >
> > Actually, I realized I'd rather not use the ForTesting suffix because we
> have
> at
> > least one class that uses a TickClock and creates Timers, and I'd like
> for it
> to
> > always set the timers to use the TickClock that the class is using.
>
> One option would be to add a new constructor that takes a TickClock
> instead of a
> setter. If any subclasses wanted to support using a different TickClock,
> they
> would also need to add a constructor that takes a TickClock and pass it
> through
> to the base class.
>

Sorry for the slow reply. I like this, constructors are nice.


> Also, how do you feel about passing a raw pointer to a TickClock versus
> passing
> a unique_ptr to a TickClock?
>

I'm okay with it if it's not awkward in the code that uses it. We can
always iterate if it turns out bad.


> We will sometimes need to use the same TickClock
> for multiple Timers or for a Timer and the class that owns it. The two
> options I
> can think of to handle that case are either pass a raw pointer to Timer
> and use
> TimeTicks::Now() if the pointer is null or create a new TickClock
> implementation
> that takes a raw pointer to another TickClock, similar to MockTickClock in
> base/test/test_mock_time_task_runner.cc.
>
> https://codereview.chromium.org/2484023002/
>

-- 
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.

[jameswest, 2016-11-15 00:55:58]

On 2016/11/12 00:42:59, danakj wrote:
> On Wed, Nov 9, 2016 at 7:06 PM, <mailto:jameswest@google.com> wrote:
> 
> > On 2016/11/10 02:52:34, jameswest wrote:
> > > On 2016/11/10 02:20:52, jameswest wrote:
> > > > > Ah, right that's not so good. I was thinking in terms of forcing
> > subclasses
> > > > > to pay attention, but not at the expense of requiring a subclass..
> > maybe
> > > > > you can just do a careful audit to make sure you get all the
> > subclasses'
> > > > > use of TimeTicks::Now? Or do you have any other ideas that could
> > help?
> > > >
> > > > I can add a ForTesting suffix to SetTickClock() and make it
> > non-virtual so
> > > that
> > > > it can only be used for tests. In production, it would always use
> > > > DefaultTickClock, which uses TimeTicks::Now(), so it would work
> > correctly in
> > > > production even if a subclass uses TimeTicks::Now(). I can then add a
> > > protected
> > > > non-virtual Now() method to Timer so that subclasses can get the time
> > from
> > the
> > > > TickClock. If a subclass uses TimeTicks::Now() instead of
> > Timer::Now(), it
> > > > should only affect any unit tests that use SetTickClockForTesting().
> > > >
> > > > The only subclasses I found outside of timer.h were in
> > base/timer/mock_timer.h
> > > > and components/timers/alarm_timer_chromeos.h.
> > >
> > > Actually, I realized I'd rather not use the ForTesting suffix because we
> > have
> > at
> > > least one class that uses a TickClock and creates Timers, and I'd like
> > for it
> > to
> > > always set the timers to use the TickClock that the class is using.
> >
> > One option would be to add a new constructor that takes a TickClock
> > instead of a
> > setter. If any subclasses wanted to support using a different TickClock,
> > they
> > would also need to add a constructor that takes a TickClock and pass it
> > through
> > to the base class.
> >
> 
> Sorry for the slow reply. I like this, constructors are nice.
> 
> 
> > Also, how do you feel about passing a raw pointer to a TickClock versus
> > passing
> > a unique_ptr to a TickClock?
> >
> 
> I'm okay with it if it's not awkward in the code that uses it. We can
> always iterate if it turns out bad.
> 
> 
> > We will sometimes need to use the same TickClock
> > for multiple Timers or for a Timer and the class that owns it. The two
> > options I
> > can think of to handle that case are either pass a raw pointer to Timer
> > and use
> > TimeTicks::Now() if the pointer is null or create a new TickClock
> > implementation
> > that takes a raw pointer to another TickClock, similar to MockTickClock in
> > base/test/test_mock_time_task_runner.cc.
> >
> > https://codereview.chromium.org/2484023002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

I changed it so that a raw pointer to a TickClock may optionally be provided to
the constructor for any of the Timer implementations, and Timer will continue to
use TimeTicks::Now() if no TickClock is provided. This works well with the code
I'm trying to unit test.

[jameswest, 2016-11-15 01:00:54]

Description was changed from

==========
Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can replace the TickClock with a
mock implementation that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077

Change-Id: I7f79c68454c5543293072b226506d756e4e3fbbc
==========

to

==========
Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can provide a mock implementation
of the TickClock that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077
==========

[danakj, 2016-11-15 01:05:33]

LGTM.

You /could/ leave the chromeos timer out if u want now too since it would need
to provide the constructor to use a TickClock anyhow. Same for any other
subclasses. I don't mind you being exhaustive either.

[danakj, 2016-11-15 01:06:44]

On Mon, Nov 14, 2016 at 5:05 PM, <danakj@chromium.org> wrote:

> LGTM.
>
> You /could/ leave the chromeos timer out if u want now too since it would
> need
> to provide the constructor to use a TickClock anyhow. Same for any other
> subclasses. I don't mind you being exhaustive either.
>

Oh, I know this is for a test to use, but it would be nice to add a unit
test to base/ with a custom TickClock that shows this is working.

-- 
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.

[jameswest, 2016-11-15 02:46:38]

On 2016/11/15 01:06:44, danakj wrote:
> On Mon, Nov 14, 2016 at 5:05 PM, <mailto:danakj@chromium.org> wrote:
> 
> > LGTM.
> >
> > You /could/ leave the chromeos timer out if u want now too since it would
> > need
> > to provide the constructor to use a TickClock anyhow. Same for any other
> > subclasses. I don't mind you being exhaustive either.
> >
> 
> Oh, I know this is for a test to use, but it would be nice to add a unit
> test to base/ with a custom TickClock that shows this is working.
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

I added unit tests. I removed the Chrome OS alarm changes because they should
probably be unit tested as well and I'd rather not do that since I'd have to
rely on CQ to compile and run the tests.

[jameswest, 2016-11-15 02:49:35]

The CQ bit was checked by jameswest@google.com

[jameswest, 2016-11-15 02:49:36]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2484023002/#ps60001
(title: "Added unit tests and removed Chrome OS changes.")

[danakj, 2016-11-15 02:50:00]

Thanks for the tests! They LGTM

[commit-bot: I haz the power, 2016-11-15 02:50:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 05:20:25]

Description was changed from

==========
Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can provide a mock implementation
of the TickClock that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077
==========

to

==========
Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can provide a mock implementation
of the TickClock that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077
==========

[commit-bot: I haz the power, 2016-11-15 05:20:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-15 05:22:52]

Description was changed from

==========
Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can provide a mock implementation
of the TickClock that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077
==========

to

==========
Use a TickClock instead of TimeTicks::Now() in Timer.

Code that uses a Timer cannot be unit tested without using sleep
or a mock timer because Timer uses TimeTicks::Now() to calculate the
run time for scheduled tasks. Using sleep makes tests slow and
unreliable, and using mock timers frequently requires unit tests to
have knowledge of internal implementation details of the code being
tested. By using a TickClock, tests can provide a mock implementation
of the TickClock that can be advanced without the use of sleep,
allowing tests to reliably verify the behavior of code as time
advances.

BUG=internal b/32644077

Committed: https://crrev.com/6e0c8d22b864d84946417eb6379c1513c36d48b0
Cr-Commit-Position: refs/heads/master@{#432113}
==========

[commit-bot: I haz the power, 2016-11-15 05:22:53]

Patchset 4 (id:??) landed as
https://crrev.com/6e0c8d22b864d84946417eb6379c1513c36d48b0
Cr-Commit-Position: refs/heads/master@{#432113}