Adding the ability to use a simulated clock for unit tests.

Adding the ability to use a simulated clock for unit tests.

This will be useful for any tests that test objects with time-dependent
behavior. It will allow such tests to be written in such a way that their
outcome is more repeatable (less flaky), and will also allow such tests
to finish quicker. For example, a test for STUN timeout doesn't need to
wait the full timeout interval in real time; it can simply advance the
simulated clock.

BUG=webrtc:4925
R=pthatcher@webrtc.org

Committed: https://crrev.com/b3c6810be3283963db5779a50f3df957b5881f91
Cr-Commit-Position: refs/heads/master@{#12950}

[Taylor Brandstetter, 2016-04-19 19:18:10]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2016-04-19 19:18:11]

Hey Peter. I'm sheriffing this week, and am getting tired of seeing tests fail
that could be improved by a simulated clock, that I went ahead and made one.
Some things are a little hacky, so your feedback is appreciated.

[juberti2, 2016-04-25 20:18:22]

juberti@chromium.org changed reviewers:
+ juberti@chromium.org

[juberti2, 2016-04-25 20:18:23]

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/simulatedclock.cc
File webrtc/base/simulatedclock.cc (right):

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/simulatedclock.cc...
webrtc/base/simulatedclock.cc:29: MessageQueueManager::WakeAllMessageQueues();
A comment explaining the intent here would be useful.

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/simulatedclock.h
File webrtc/base/simulatedclock.h (right):

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/simulatedclock.h#...
webrtc/base/simulatedclock.h:21: class SimulatedClock : public ClockInterface {
Should this be FakeClock, to match our other fakes?

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/timeutils.cc
File webrtc/base/timeutils.cc (right):

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/timeutils.cc#newc...
webrtc/base/timeutils.cc:42: if (clock_) {
Do we need to mention that this is unsynchronized?

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/timeutils.cc#newc...
webrtc/base/timeutils.cc:46: #if defined(WEBRTC_MAC)
Should we factor this into a real clock implementation?

[Taylor Brandstetter, 2016-04-26 01:22:32]

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/simulatedclock.cc
File webrtc/base/simulatedclock.cc (right):

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/simulatedclock.cc...
webrtc/base/simulatedclock.cc:29: MessageQueueManager::WakeAllMessageQueues();
On 2016/04/25 20:18:22, juberti2 wrote:
> A comment explaining the intent here would be useful.

Done.

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/simulatedclock.h
File webrtc/base/simulatedclock.h (right):

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/simulatedclock.h#...
webrtc/base/simulatedclock.h:21: class SimulatedClock : public ClockInterface {
On 2016/04/25 20:18:23, juberti2 wrote:
> Should this be FakeClock, to match our other fakes?

Done.

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/timeutils.cc
File webrtc/base/timeutils.cc (right):

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/timeutils.cc#newc...
webrtc/base/timeutils.cc:42: if (clock_) {
On 2016/04/25 20:18:23, juberti2 wrote:
> Do we need to mention that this is unsynchronized?

I mention in timeutils.h that SetClock (which is the only thing that modifies
clock_) isn't thread-safe.

https://codereview.webrtc.org/1895933003/diff/1/webrtc/base/timeutils.cc#newc...
webrtc/base/timeutils.cc:46: #if defined(WEBRTC_MAC)
On 2016/04/25 20:18:23, juberti2 wrote:
> Should we factor this into a real clock implementation?

Done.

[Taylor Brandstetter, 2016-05-24 16:17:56]

deadbeef@webrtc.org changed reviewers:
- juberti@chromium.org

[Taylor Brandstetter, 2016-05-24 16:17:56]

It would be great if we could land this. As I worried, the unit tests for
caching the DTLS hello ended up being flaky when changing the timeout to 50ms:
https://codereview.webrtc.org/1981463002/

[pthatcher1, 2016-05-24 18:05:46]

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.cc
File webrtc/base/fakeclock.cc (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.cc#...
webrtc/base/fakeclock.cc:41: AdvanceTimeNanos(micros * 1000);
You could use kNumNanosecsPerMicrosec.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.cc#...
webrtc/base/fakeclock.cc:45: AdvanceTimeNanos(millis * 1000 * 1000);
And kNumNanosecsPerMillisec and kNumNanosecsPerSec.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.h
File webrtc/base/fakeclock.h (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.h#n...
webrtc/base/fakeclock.h:31: void SetTimeNanos(uint64_t nanos);
It might make sense to copy Chrome's TimeDelta class, which would let you do
this:

void Advance(TimeDelta delta);

Advance(TimeDelta::FromSeconds(n));
Advance(TimeDelta::FromMilliseconds(n));

Chrome calls it MockClock.


Or like other codebases:

void SleepTime(Duration d);

SleepTime(Seconds(n));
SleepTime(Milliseconds(n));

That codebase calls it MockClock.


Or like Go:

Add(Duration d)

Add(n * Microsecond)

Go calls is FakeClock.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc
File webrtc/base/timeutils.cc (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:50: const MacClock default_clock_;
I think it would make more sense to do a typedef MacClock Clock inside of each
ifdef.  Then, one could construct a new Clock() without knowing the OS they are
on.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:50: const MacClock default_clock_;
Is Chrome going to be OK with this?  They don't allow any static initializers. 
I don't know exactly what is OK and what's not, but I know they are more strict
in their check then we are, so you may want to try and compile chrome to see if
it works.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:95: const ClockInterface* clock_ = &default_clock_;
I've not seen other code where we have the member variable name style "foo_"
used for global constants.  Do we have other places where we do that?

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:98: clock_ = clock ? clock : &default_clock_;
Does this take ownership of the clock?  Should it delete it when it's cleared? 
At least make it clear.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:103: return clock_->TimeNanos();
I think instead of having a global clock that TimeNanos() calls, would it be
feasible to inject Clock objects into all the places that call TimeNanos()?  

We already do that in several places with the Timing class.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.h#n...
webrtc/base/timeutils.h:38: };
Can you put a TODO to move this into a clock.h?  In fact, could you add clock.h
now and just include this file, so that new code can start using it and old code
can start moving over?  

Can you also make a TODO to remove Timing in timing.h, since it will no longer
be needed?

[Taylor Brandstetter, 2016-05-24 21:48:00]

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.cc
File webrtc/base/fakeclock.cc (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.cc#...
webrtc/base/fakeclock.cc:41: AdvanceTimeNanos(micros * 1000);
On 2016/05/24 18:05:46, pthatcher1 wrote:
> You could use kNumNanosecsPerMicrosec.

Done.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.cc#...
webrtc/base/fakeclock.cc:45: AdvanceTimeNanos(millis * 1000 * 1000);
On 2016/05/24 18:05:46, pthatcher1 wrote:
> And kNumNanosecsPerMillisec and kNumNanosecsPerSec.

Done.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.h
File webrtc/base/fakeclock.h (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/fakeclock.h#n...
webrtc/base/fakeclock.h:31: void SetTimeNanos(uint64_t nanos);
On 2016/05/24 18:05:46, pthatcher1 wrote:
> It might make sense to copy Chrome's TimeDelta class, which would let you do
> this:
> 
> void Advance(TimeDelta delta);
> 
> Advance(TimeDelta::FromSeconds(n));
> Advance(TimeDelta::FromMilliseconds(n));
> 
> Chrome calls it MockClock.
> 
> 
> Or like other codebases:
> 
> void SleepTime(Duration d);
> 
> SleepTime(Seconds(n));
> SleepTime(Milliseconds(n));
> 
> That codebase calls it MockClock.
> 
> 
> Or like Go:
> 
> Add(Duration d)
> 
> Add(n * Microsecond)
> 
> Go calls is FakeClock.

Makes sense. I didn't do this initially because everything else just uses nanos
of milliseconds. But we could change that eventually. It would be more readable
to see "PostDelayed(..., TimeDelta::FromSeconds(10))" rather than
"PostDelayed(..., 10000)".

I took TimeDelta and changed some of the units/types (we don't need "days" but
we do need "nanoseconds"). I also removed the handling of numeric_limits::max
and overflow, since that doesn't seem like it would ever be necessary for
WebRTC.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc
File webrtc/base/timeutils.cc (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:50: const MacClock default_clock_;
On 2016/05/24 18:05:46, pthatcher1 wrote:
> I think it would make more sense to do a typedef MacClock Clock inside of each
> ifdef.  Then, one could construct a new Clock() without knowing the OS they
are
> on.

Done.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:50: const MacClock default_clock_;
On 2016/05/24 18:05:46, pthatcher1 wrote:
> Is Chrome going to be OK with this?  They don't allow any static initializers.

> I don't know exactly what is OK and what's not, but I know they are more
strict
> in their check then we are, so you may want to try and compile chrome to see
if
> it works.

Good catch. Fixing this would need to use some kind of singleton implementation
which is a lot of overkill for something that was working fine before, and
doesn't really need to be converted to an object since it has no state. I'll
just change it back, since objectify-ing the existing time code is orthogonal to
this CL.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:95: const ClockInterface* clock_ = &default_clock_;
On 2016/05/24 18:05:46, pthatcher1 wrote:
> I've not seen other code where we have the member variable name style "foo_"
> used for global constants.  Do we have other places where we do that?

I don't remember why I named it that, but it looks like the proper pattern is
"g_name". Fixed.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:98: clock_ = clock ? clock : &default_clock_;
On 2016/05/24 18:05:46, pthatcher1 wrote:
> Does this take ownership of the clock?  Should it delete it when it's cleared?

> At least make it clear.  

No it doesn't; I made the comment in the header file more clear.

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.cc#...
webrtc/base/timeutils.cc:103: return clock_->TimeNanos();
On 2016/05/24 18:05:46, pthatcher1 wrote:
> I think instead of having a global clock that TimeNanos() calls, would it be
> feasible to inject Clock objects into all the places that call TimeNanos()?  
> 
> We already do that in several places with the Timing class.

It's feasible, and it would be ideal (since it would mean no global variable,
and people could implement a clock for their own platform and pass it in). But I
think it would be quite the expansive change. Could it be done in a separate CL?

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.h#n...
webrtc/base/timeutils.h:38: };
On 2016/05/24 18:05:46, pthatcher1 wrote:
> Can you put a TODO to move this into a clock.h?  In fact, could you add
clock.h
> now and just include this file, so that new code can start using it and old
code
> can start moving over?  
> 
> Can you also make a TODO to remove Timing in timing.h, since it will no longer
> be needed?

What do you mean by "new code start moving over"? What are they moving to?

[pthatcher1, 2016-05-26 17:48:38]

lgtm

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.h#n...
webrtc/base/timeutils.h:38: };
On 2016/05/24 21:48:00, Taylor Brandstetter wrote:
> On 2016/05/24 18:05:46, pthatcher1 wrote:
> > Can you put a TODO to move this into a clock.h?  In fact, could you add
> clock.h
> > now and just include this file, so that new code can start using it and old
> code
> > can start moving over?  
> > 
> > Can you also make a TODO to remove Timing in timing.h, since it will no
longer
> > be needed?
> 
> What do you mean by "new code start moving over"? What are they moving to?

I mean we replace all uses of Timing with ClockInterface and delete Timing.

[Taylor Brandstetter, 2016-05-26 21:41:31]

On 2016/05/26 17:48:38, pthatcher1 wrote:
> lgtm
> 
> https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.h
> File webrtc/base/timeutils.h (right):
> 
>
https://codereview.webrtc.org/1895933003/diff/20001/webrtc/base/timeutils.h#n...
> webrtc/base/timeutils.h:38: };
> On 2016/05/24 21:48:00, Taylor Brandstetter wrote:
> > On 2016/05/24 18:05:46, pthatcher1 wrote:
> > > Can you put a TODO to move this into a clock.h?  In fact, could you add
> > clock.h
> > > now and just include this file, so that new code can start using it and
old
> > code
> > > can start moving over?  
> > > 
> > > Can you also make a TODO to remove Timing in timing.h, since it will no
> longer
> > > be needed?
> > 
> > What do you mean by "new code start moving over"? What are they moving to?
> 
> I mean we replace all uses of Timing with ClockInterface and delete Timing.

Ah. I didn't intend for ClockInterface to replace Timing; at the moment it's
just an implementation detail to hook in a simulated clock. But I guess we could
do that eventually.

[pthatcher1, 2016-05-26 21:43:23]

I think we should do that.  ClockInterface looks better than Timing.  Please put
a TODO somewhere.

[Taylor Brandstetter, 2016-05-26 22:37:31]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-05-26 22:37:31]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1895933003/#ps100001 (title:
"Merge with master")

[commit-bot: I haz the power, 2016-05-26 22:37:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895933003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895933003/100001

[commit-bot: I haz the power, 2016-05-26 22:42:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-26 22:42:31]

Try jobs failed on following builders:
  android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)

[Taylor Brandstetter, 2016-05-26 23:06:40]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-05-26 23:06:40]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1895933003/#ps120001 (title:
"Moving new file to correct .gyp file.")

[commit-bot: I haz the power, 2016-05-26 23:06:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895933003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895933003/120001

[commit-bot: I haz the power, 2016-05-26 23:26:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-26 23:26:02]

Try jobs failed on following builders:
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/12513)

[Taylor Brandstetter, 2016-05-27 17:19:09]

Peter, I submitted a few more patches to fix minor issues; do you want to review
again?

[pthatcher1, 2016-05-27 17:49:38]

lgtm

[Taylor Brandstetter, 2016-05-27 20:48:39]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-05-27 20:48:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895933003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895933003/180001

[commit-bot: I haz the power, 2016-05-27 20:50:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 20:50:54]

Try jobs failed on following builders:
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9746)

[Taylor Brandstetter, 2016-05-27 21:16:12]

Description was changed from

==========
Adding the ability to use a simulated clock for unit tests.

This will be useful for any tests that test objects with time-dependent
behavior. It will allow such tests to be written in such a way that their
outcome is more repeatable (less flaky), and will also allow such tests
to finish quicker. For example, a test for STUN timeout doesn't need to
wait the full timeout interval in real time; it can simply advance the
simulated clock.

BUG=webrtc:4925
==========

to

==========
Adding the ability to use a simulated clock for unit tests.

This will be useful for any tests that test objects with time-dependent
behavior. It will allow such tests to be written in such a way that their
outcome is more repeatable (less flaky), and will also allow such tests
to finish quicker. For example, a test for STUN timeout doesn't need to
wait the full timeout interval in real time; it can simply advance the
simulated clock.

BUG=webrtc:4925
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/b3c6810be3283963db5779a50...
==========

[Taylor Brandstetter, 2016-05-27 21:16:14]

Committed patchset #11 (id:200001) manually as
b3c6810be3283963db5779a50f3df957b5881f91 (presubmit successful).

[commit-bot: I haz the power, 2016-05-27 21:16:14]

Description was changed from

==========
Adding the ability to use a simulated clock for unit tests.

This will be useful for any tests that test objects with time-dependent
behavior. It will allow such tests to be written in such a way that their
outcome is more repeatable (less flaky), and will also allow such tests
to finish quicker. For example, a test for STUN timeout doesn't need to
wait the full timeout interval in real time; it can simply advance the
simulated clock.

BUG=webrtc:4925
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/b3c6810be3283963db5779a50...
==========

to

==========
Adding the ability to use a simulated clock for unit tests.

This will be useful for any tests that test objects with time-dependent
behavior. It will allow such tests to be written in such a way that their
outcome is more repeatable (less flaky), and will also allow such tests
to finish quicker. For example, a test for STUN timeout doesn't need to
wait the full timeout interval in real time; it can simply advance the
simulated clock.

BUG=webrtc:4925
R=pthatcher@webrtc.org

Committed: https://crrev.com/b3c6810be3283963db5779a50f3df957b5881f91
Cr-Commit-Position: refs/heads/master@{#12950}
==========