[chromium-dev] Time::Now() in tests

[s***@chromium.org]

I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It assumed
the current day would have 24 hours, which in retrospect is clearly not
safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and should be
stopped by a global PRESUBMIT. The disadvantages of Time::Now() that I see
in tests are:

- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.

If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.

There's been a thread (Testing base::OneShotTimer / "waiting" in a browser
test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug <https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414> there
are plans to process wide mock base::Time::Now().

Does anyone have concerns about this?

Thanks,
Sky

['Alex Clarke' via Chromium-dev]

Agreed that Now() can be problematic, although down the line it should be
possible to mock it via base::ScopedTaskEnvironment. See
https://bugs.chromium.org/p/chromium/issues/detail?id=905412

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.
It seems to me like putting Time::Now() in tests is dangerous and should
be stopped by a global PRESUBMIT. The disadvantages of Time::Now() that I
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug <https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414> there
are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

[d***@chromium.org]

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and should be

Post by s***@chromium.org
stopped by a global PRESUBMIT. The disadvantages of Time::Now() that I see
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug <https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414> there
are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

[Sunny Sachanandani]

If you're testing the execution of delayed tasks, you can probably use
base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and should

Post by s***@chromium.org
be stopped by a global PRESUBMIT. The disadvantages of Time::Now() that I
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug <https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Tommy C. Li]

I've found that using base::Clock in the production class greatly improves
testability, as then tests can inject in a SimpleTestClock.

Tommy

On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny Sachanandani

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably use
base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and should

Post by s***@chromium.org
be stopped by a global PRESUBMIT. The disadvantages of Time::Now() that I
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug <https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Yuri Wiitala]

+1 to Tommi's point.

As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly improves
testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny Sachanandani

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably use
base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and should

Post by s***@chromium.org
be stopped by a global PRESUBMIT. The disadvantages of Time::Now() that I
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Tommy C. Li]

Yuri - that's an interesting thought. Is it INfeasible?

For instance, what if we marked those as deprecated and added a presubmit
check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny Sachanandani

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably use
base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Yuri Wiitala]

I shouldn't have used the word "infeasible." :) However, this is something
the Chromium community needs to decide is worth the migration effort. (At
the same time, maybe we would want to switch everything over to
std::chrono?)

As for eliminating global now() functions: The deprecation/presubmit check
would be a good start; but before doing something like that, we'd need to
first plumb pointers to clock instances throughout our object graph in such
a way as they are readily-accessible wherever a time source is needed.
Also, we should remove base::DefaultClock and DefaultTickClock, since their
GetInstance() is just another flavor of "global function pointer."

Such a migration is going to be on the scale of the
base::Callback→base::OnceCallback migration, except much harder (given the
need to plumb things through the object graph). A quick check on the number
of call points:

~/chromium/src$ git grep -P '(Time|TimeTicks|ThreadTicks)::Now' | wc -l

7628

All said, there are huge benefits to be had here:

- Eliminate a huge source (the main source?) of test flakiness (e.g.,
http://crbug.com/866930).
- Code becomes simulation-ready for unit testing and/or experimenting
with new ideas.
- Safer to fix long-standing bugs requiring changing clock sources
(e.g., to resolve things like http://crbug.com/166153).
- Improved clock control for Headless Chrome (virtualized clocks, when
using the browser to render pages in a service framework).

Post by Tommy C. Li
Yuri - that's an interesting thought. Is it INfeasible?
For instance, what if we marked those as deprecated and added a presubmit
check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny Sachanandani

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably use
base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org?utm_medium=email&utm_source=footer>
.

[Daniel Cheng]

I guess it's not clear to me: when mocking out clocks, why would we only
want to mock out some of them? That seems like a recipe to get into a
situation where a test accidentally mocks some clocks but not others.
Wouldn't a global hook (that integrates with task scheduling as well, so
time advances in a consistent manner) be more reliable for this sort of
thing?

Daniel

Post by Yuri Wiitala
I shouldn't have used the word "infeasible." :) However, this is something
the Chromium community needs to decide is worth the migration effort. (At
the same time, maybe we would want to switch everything over to
std::chrono?)
As for eliminating global now() functions: The deprecation/presubmit check
would be a good start; but before doing something like that, we'd need to
first plumb pointers to clock instances throughout our object graph in such
a way as they are readily-accessible wherever a time source is needed.
Also, we should remove base::DefaultClock and DefaultTickClock, since their
GetInstance() is just another flavor of "global function pointer."
Such a migration is going to be on the scale of the
base::Callback→base::OnceCallback migration, except much harder (given the
need to plumb things through the object graph). A quick check on the number
~/chromium/src$ git grep -P '(Time|TimeTicks|ThreadTicks)::Now' | wc -l
7628
- Eliminate a huge source (the main source?) of test flakiness (e.g.,
http://crbug.com/866930).
- Code becomes simulation-ready for unit testing and/or experimenting
with new ideas.
- Safer to fix long-standing bugs requiring changing clock sources
(e.g., to resolve things like http://crbug.com/166153).
- Improved clock control for Headless Chrome (virtualized clocks, when
using the browser to render pages in a service framework).

Post by Tommy C. Li
Yuri - that's an interesting thought. Is it INfeasible?
For instance, what if we marked those as deprecated and added a presubmit
check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny Sachanandani

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably use
base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should be
sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org?utm_medium=email&utm_source=footer>
.

[Yuri Wiitala]

Global mocking and un-mocking muddy the program state w.r.t. the scheduler
assuming the same clock source, both when tasks enter and leave the task
queue. The doc that was linked from http://crbug.com/866930 describes one
(of several) attempts at global clock mocking:
https://docs.google.com/document/d/1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo/edit

Granted, I do see your point (well, I'm inferring it) about a global clock
being a good idea for consistency's sake. The problem, as we keep
discovering, is that the global clock must never change: If it is mocked,
it should never be un-mocked or changed to something else for the life of
the process. That's why, for Chrome Headless versus normal browser mode,
the global time functions are "set once and never change" in their
respective main() functions.

Post by Daniel Cheng
I guess it's not clear to me: when mocking out clocks, why would we only
want to mock out some of them? That seems like a recipe to get into a
situation where a test accidentally mocks some clocks but not others.
Wouldn't a global hook (that integrates with task scheduling as well, so
time advances in a consistent manner) be more reliable for this sort of
thing?
Daniel

Post by Yuri Wiitala
I shouldn't have used the word "infeasible." :) However, this is
something the Chromium community needs to decide is worth the migration
effort. (At the same time, maybe we would want to switch everything over to
std::chrono?)
As for eliminating global now() functions: The deprecation/presubmit
check would be a good start; but before doing something like that, we'd
need to first plumb pointers to clock instances throughout our object graph
in such a way as they are readily-accessible wherever a time source is
needed. Also, we should remove base::DefaultClock and DefaultTickClock,
since their GetInstance() is just another flavor of "global function
pointer."
Such a migration is going to be on the scale of the
base::Callback→base::OnceCallback migration, except much harder (given the
need to plumb things through the object graph). A quick check on the number
~/chromium/src$ git grep -P '(Time|TimeTicks|ThreadTicks)::Now' | wc -l
7628
- Eliminate a huge source (the main source?) of test flakiness (e.g.,
http://crbug.com/866930).
- Code becomes simulation-ready for unit testing and/or experimenting
with new ideas.
- Safer to fix long-standing bugs requiring changing clock sources
(e.g., to resolve things like http://crbug.com/166153).
- Improved clock control for Headless Chrome (virtualized clocks,
when using the browser to render pages in a service framework).

Post by Tommy C. Li
Yuri - that's an interesting thought. Is it INfeasible?
For instance, what if we marked those as deprecated and added a
presubmit check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny Sachanandani

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably
use base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should
be sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in a
browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org?utm_medium=email&utm_source=footer>
.

['Alex Clarke' via Chromium-dev]

I think we can (at least try to) override time globally in the browser.
For Headless we built a way to do this for the Renderer, see
AutoAdvancingVirtualTimeDomain
<https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/scheduler/main_thread/auto_advancing_virtual_time_domain.cc?q=base::subtle::ScopedTimeClockOverrides+f:auto_advancing_virtual_time_domain.cc&g=0&l=42>.
I'm under the impression we can do something very similar controlled by
ScopedTaskEnvironment.

Post by Yuri Wiitala
Global mocking and un-mocking muddy the program state w.r.t. the scheduler
assuming the same clock source, both when tasks enter and leave the task
queue. The doc that was linked from http://crbug.com/866930 describes one
https://docs.google.com/document/d/1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo/edit
Granted, I do see your point (well, I'm inferring it) about a global clock
being a good idea for consistency's sake. The problem, as we keep
discovering, is that the global clock must never change: If it is mocked,
it should never be un-mocked or changed to something else for the life of
the process. That's why, for Chrome Headless versus normal browser mode,
the global time functions are "set once and never change" in their
respective main() functions.

Post by Daniel Cheng
I guess it's not clear to me: when mocking out clocks, why would we only
want to mock out some of them? That seems like a recipe to get into a
situation where a test accidentally mocks some clocks but not others.
Wouldn't a global hook (that integrates with task scheduling as well, so
time advances in a consistent manner) be more reliable for this sort of
thing?
Daniel

Post by Yuri Wiitala
I shouldn't have used the word "infeasible." :) However, this is
something the Chromium community needs to decide is worth the migration
effort. (At the same time, maybe we would want to switch everything over to
std::chrono?)
As for eliminating global now() functions: The deprecation/presubmit
check would be a good start; but before doing something like that, we'd
need to first plumb pointers to clock instances throughout our object graph
in such a way as they are readily-accessible wherever a time source is
needed. Also, we should remove base::DefaultClock and DefaultTickClock,
since their GetInstance() is just another flavor of "global function
pointer."
Such a migration is going to be on the scale of the
base::Callback→base::OnceCallback migration, except much harder (given the
need to plumb things through the object graph). A quick check on the number
~/chromium/src$ git grep -P '(Time|TimeTicks|ThreadTicks)::Now' | wc -l
7628
- Eliminate a huge source (the main source?) of test flakiness
(e.g., http://crbug.com/866930).
- Code becomes simulation-ready for unit testing and/or
experimenting with new ideas.
- Safer to fix long-standing bugs requiring changing clock sources
(e.g., to resolve things like http://crbug.com/166153).
- Improved clock control for Headless Chrome (virtualized clocks,
when using the browser to render pages in a service framework).

Post by Tommy C. Li
Yuri - that's an interesting thought. Is it INfeasible?
For instance, what if we marked those as deprecated and added a
presubmit check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably
use base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should
be sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in
a browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BN%2BEKYbmv%2Bxj9T1rRLqHrv3p%2BKxeC8%2B-Bx9m%2BO7QZR%2BUqXp%2BQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BN%2BEKYbmv%2Bxj9T1rRLqHrv3p%2BKxeC8%2B-Bx9m%2BO7QZR%2BUqXp%2BQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Yuri Wiitala]

For Headless, AAVirtualTimeDomain overrides the global time functions using
this:
https://cs.chromium.org/chromium/src/base/time/time_override.h?sq=package:chromium&g=0&l=29

Global clock mocking is fine as long as it's done once, at the beginning of
a process's execution (before any Now() function would be called). Also,
it's dangerous to ever revert or change the global clock mocking because
it's very difficult to guarantee Time/TimeTicks values from the mocked
clock won't intermingle with those from the non-mocked clock (e.g., pending
tasks that remain in task queues).

Post by 'Alex Clarke' via Chromium-dev
I think we can (at least try to) override time globally in the browser.
For Headless we built a way to do this for the Renderer, see
AutoAdvancingVirtualTimeDomain
<https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/scheduler/main_thread/auto_advancing_virtual_time_domain.cc?q=base::subtle::ScopedTimeClockOverrides+f:auto_advancing_virtual_time_domain.cc&g=0&l=42>.
I'm under the impression we can do something very similar controlled by
ScopedTaskEnvironment.

Post by Yuri Wiitala
Global mocking and un-mocking muddy the program state w.r.t. the
scheduler assuming the same clock source, both when tasks enter and leave
the task queue. The doc that was linked from http://crbug.com/866930
https://docs.google.com/document/d/1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo/edit
Granted, I do see your point (well, I'm inferring it) about a global
clock being a good idea for consistency's sake. The problem, as we keep
discovering, is that the global clock must never change: If it is mocked,
it should never be un-mocked or changed to something else for the life of
the process. That's why, for Chrome Headless versus normal browser mode,
the global time functions are "set once and never change" in their
respective main() functions.

Post by Daniel Cheng
I guess it's not clear to me: when mocking out clocks, why would we only
want to mock out some of them? That seems like a recipe to get into a
situation where a test accidentally mocks some clocks but not others.
Wouldn't a global hook (that integrates with task scheduling as well, so
time advances in a consistent manner) be more reliable for this sort of
thing?
Daniel

Post by Yuri Wiitala
I shouldn't have used the word "infeasible." :) However, this is
something the Chromium community needs to decide is worth the migration
effort. (At the same time, maybe we would want to switch everything over to
std::chrono?)
As for eliminating global now() functions: The deprecation/presubmit
check would be a good start; but before doing something like that, we'd
need to first plumb pointers to clock instances throughout our object graph
in such a way as they are readily-accessible wherever a time source is
needed. Also, we should remove base::DefaultClock and DefaultTickClock,
since their GetInstance() is just another flavor of "global function
pointer."
Such a migration is going to be on the scale of the
base::Callback→base::OnceCallback migration, except much harder (given the
need to plumb things through the object graph). A quick check on the number
~/chromium/src$ git grep -P '(Time|TimeTicks|ThreadTicks)::Now' | wc -l
7628
- Eliminate a huge source (the main source?) of test flakiness
(e.g., http://crbug.com/866930).
- Code becomes simulation-ready for unit testing and/or
experimenting with new ideas.
- Safer to fix long-standing bugs requiring changing clock sources
(e.g., to resolve things like http://crbug.com/166153).
- Improved clock control for Headless Chrome (virtualized clocks,
when using the browser to render pages in a service framework).

Post by Tommy C. Li
Yuri - that's an interesting thought. Is it INfeasible?
For instance, what if we marked those as deprecated and added a
presubmit check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably
use base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that

the production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should
be sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in
a browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the
Google Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BN%2BEKYbmv%2Bxj9T1rRLqHrv3p%2BKxeC8%2B-Bx9m%2BO7QZR%2BUqXp%2BQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BN%2BEKYbmv%2Bxj9T1rRLqHrv3p%2BKxeC8%2B-Bx9m%2BO7QZR%2BUqXp%2BQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Tommy C. Li]

Ah - I didn't realize the problems that could arise from global clock
mocking.

In practice, I have found that mocking local clocks to be totally adequate
for many unit tests.

To start the transition, maybe we can deprecate the static Now() method and
tell new users to construct a DefaultClock?

I'm not sure we have to plumb a pointers to clock instances throughout the
object graph when leaf classes could just construct their own clocks.

This is what TemplateURLService
does: https://cs.chromium.org/chromium/src/chrome/browser/search_engines/template_url_service_unittest.cc?q=set_clock+template_url_service&dr=C&l=392

Post by Yuri Wiitala
Global mocking and un-mocking muddy the program state w.r.t. the scheduler
assuming the same clock source, both when tasks enter and leave the task
queue. The doc that was linked from http://crbug.com/866930 describes one
https://docs.google.com/document/d/1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo/edit
Granted, I do see your point (well, I'm inferring it) about a global clock
being a good idea for consistency's sake. The problem, as we keep
discovering, is that the global clock must never change: If it is mocked,
it should never be un-mocked or changed to something else for the life of
the process. That's why, for Chrome Headless versus normal browser mode,
the global time functions are "set once and never change" in their
respective main() functions.

Post by Daniel Cheng
I guess it's not clear to me: when mocking out clocks, why would we only
want to mock out some of them? That seems like a recipe to get into a
situation where a test accidentally mocks some clocks but not others.
Wouldn't a global hook (that integrates with task scheduling as well, so
time advances in a consistent manner) be more reliable for this sort of
thing?
Daniel

Post by Yuri Wiitala
I shouldn't have used the word "infeasible." :) However, this is
something the Chromium community needs to decide is worth the migration
effort. (At the same time, maybe we would want to switch everything over to
std::chrono?)
As for eliminating global now() functions: The deprecation/presubmit
check would be a good start; but before doing something like that, we'd
need to first plumb pointers to clock instances throughout our object graph
in such a way as they are readily-accessible wherever a time source is
needed. Also, we should remove base::DefaultClock and DefaultTickClock,
since their GetInstance() is just another flavor of "global function
pointer."
Such a migration is going to be on the scale of the
base::Callback→base::OnceCallback migration, except much harder (given the
need to plumb things through the object graph). A quick check on the number
~/chromium/src$ git grep -P '(Time|TimeTicks|ThreadTicks)::Now' | wc -l
7628
- Eliminate a huge source (the main source?) of test flakiness
(e.g., http://crbug.com/866930).
- Code becomes simulation-ready for unit testing and/or
experimenting with new ideas.
- Safer to fix long-standing bugs requiring changing clock sources
(e.g., to resolve things like http://crbug.com/166153).
- Improved clock control for Headless Chrome (virtualized clocks,
when using the browser to render pages in a service framework).

Post by Tommy C. Li
Yuri - that's an interesting thought. Is it INfeasible?
For instance, what if we marked those as deprecated and added a
presubmit check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably
use base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that the

production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should
be sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in
a browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org?utm_medium=email&utm_source=footer>
.

[Yuri Wiitala]

The main issue--what Daniel Cheng was mentioning--is that everywhere you
see a Time or a TimeTicks, it is assumed to be from the same clock. So, you
can do math on it, and everything is fine. If leaf classes allocate their
own Clock instances, they generally would not know if the rest of the
object graph is actually using the same time source.

Perhaps it would be good for us to add some kind of "time source ID" member
to all Time and TimeTicks values? But only on DCHECK-enabled builds?
Something like:

template <...> class TimeBase {
...
private:
#if DCHECK_IS_ON()
const void* time_source_;
#endif
};

...and the math/comparison operators would all:
DCHECK_EQ(this->time_source_, other.time_source_);

Post by Tommy C. Li
Ah - I didn't realize the problems that could arise from global clock
mocking.
In practice, I have found that mocking local clocks to be totally adequate
for many unit tests.
To start the transition, maybe we can deprecate the static Now() method
and tell new users to construct a DefaultClock?
I'm not sure we have to plumb a pointers to clock instances throughout the
object graph when leaf classes could just construct their own clocks.
https://cs.chromium.org/chromium/src/chrome/browser/search_engines/template_url_service_unittest.cc?q=set_clock+template_url_service&dr=C&l=392

Post by Yuri Wiitala
Global mocking and un-mocking muddy the program state w.r.t. the
scheduler assuming the same clock source, both when tasks enter and leave
the task queue. The doc that was linked from http://crbug.com/866930
https://docs.google.com/document/d/1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo/edit
Granted, I do see your point (well, I'm inferring it) about a global
clock being a good idea for consistency's sake. The problem, as we keep
discovering, is that the global clock must never change: If it is mocked,
it should never be un-mocked or changed to something else for the life of
the process. That's why, for Chrome Headless versus normal browser mode,
the global time functions are "set once and never change" in their
respective main() functions.

Post by Daniel Cheng
I guess it's not clear to me: when mocking out clocks, why would we only
want to mock out some of them? That seems like a recipe to get into a
situation where a test accidentally mocks some clocks but not others.
Wouldn't a global hook (that integrates with task scheduling as well, so
time advances in a consistent manner) be more reliable for this sort of
thing?
Daniel

Post by Yuri Wiitala
I shouldn't have used the word "infeasible." :) However, this is
something the Chromium community needs to decide is worth the migration
effort. (At the same time, maybe we would want to switch everything over to
std::chrono?)
As for eliminating global now() functions: The deprecation/presubmit
check would be a good start; but before doing something like that, we'd
need to first plumb pointers to clock instances throughout our object graph
in such a way as they are readily-accessible wherever a time source is
needed. Also, we should remove base::DefaultClock and DefaultTickClock,
since their GetInstance() is just another flavor of "global function
pointer."
Such a migration is going to be on the scale of the
base::Callback→base::OnceCallback migration, except much harder (given the
need to plumb things through the object graph). A quick check on the number
~/chromium/src$ git grep -P '(Time|TimeTicks|ThreadTicks)::Now' | wc -l
7628
- Eliminate a huge source (the main source?) of test flakiness
(e.g., http://crbug.com/866930).
- Code becomes simulation-ready for unit testing and/or
experimenting with new ideas.
- Safer to fix long-standing bugs requiring changing clock sources
(e.g., to resolve things like http://crbug.com/166153).
- Improved clock control for Headless Chrome (virtualized clocks,
when using the browser to render pages in a service framework).

Post by Tommy C. Li
Yuri - that's an interesting thought. Is it INfeasible?
For instance, what if we marked those as deprecated and added a
presubmit check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably
use base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that

the production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they should
be sharing a base::Clock. Or, seemingly more often, use base::TimeTicks
instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting" in
a browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the
Google Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/34b0aa61-9c97-4f74-b04e-8e13f653d82c%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/34b0aa61-9c97-4f74-b04e-8e13f653d82c%40chromium.org?utm_medium=email&utm_source=footer>
.

[Tommy Li]

Yuri,

I really like that idea.

Can we make it so that it's nullptr if it's from a "real" clock?

It seems like two different clock instances should still issue compatible
Time instances unless either one of them is a mock.

Post by Yuri Wiitala
The main issue--what Daniel Cheng was mentioning--is that everywhere you
see a Time or a TimeTicks, it is assumed to be from the same clock. So, you
can do math on it, and everything is fine. If leaf classes allocate their
own Clock instances, they generally would not know if the rest of the
object graph is actually using the same time source.
Perhaps it would be good for us to add some kind of "time source ID"
member to all Time and TimeTicks values? But only on DCHECK-enabled builds?
template <...> class TimeBase {
...
#if DCHECK_IS_ON()
const void* time_source_;
#endif
};
DCHECK_EQ(this->time_source_, other.time_source_);

Post by Tommy C. Li
Ah - I didn't realize the problems that could arise from global clock
mocking.
In practice, I have found that mocking local clocks to be totally
adequate for many unit tests.
To start the transition, maybe we can deprecate the static Now() method
and tell new users to construct a DefaultClock?
I'm not sure we have to plumb a pointers to clock instances throughout
the object graph when leaf classes could just construct their own clocks.
https://cs.chromium.org/chromium/src/chrome/browser/search_engines/template_url_service_unittest.cc?q=set_clock+template_url_service&dr=C&l=392

Post by Yuri Wiitala
Global mocking and un-mocking muddy the program state w.r.t. the
scheduler assuming the same clock source, both when tasks enter and leave
the task queue. The doc that was linked from http://crbug.com/866930
https://docs.google.com/document/d/1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo/edit
Granted, I do see your point (well, I'm inferring it) about a global
clock being a good idea for consistency's sake. The problem, as we keep
discovering, is that the global clock must never change: If it is mocked,
it should never be un-mocked or changed to something else for the life of
the process. That's why, for Chrome Headless versus normal browser mode,
the global time functions are "set once and never change" in their
respective main() functions.

Post by Daniel Cheng
I guess it's not clear to me: when mocking out clocks, why would we
only want to mock out some of them? That seems like a recipe to get into a
situation where a test accidentally mocks some clocks but not others.
Wouldn't a global hook (that integrates with task scheduling as well, so
time advances in a consistent manner) be more reliable for this sort of
thing?
Daniel

Post by Yuri Wiitala
I shouldn't have used the word "infeasible." :) However, this is
something the Chromium community needs to decide is worth the migration
effort. (At the same time, maybe we would want to switch everything over to
std::chrono?)
As for eliminating global now() functions: The deprecation/presubmit
check would be a good start; but before doing something like that, we'd
need to first plumb pointers to clock instances throughout our object graph
in such a way as they are readily-accessible wherever a time source is
needed. Also, we should remove base::DefaultClock and DefaultTickClock,
since their GetInstance() is just another flavor of "global function
pointer."
Such a migration is going to be on the scale of the
base::Callback→base::OnceCallback migration, except much harder (given the
need to plumb things through the object graph). A quick check on the number
~/chromium/src$ git grep -P '(Time|TimeTicks|ThreadTicks)::Now' | wc -l
7628
- Eliminate a huge source (the main source?) of test flakiness
(e.g., http://crbug.com/866930).
- Code becomes simulation-ready for unit testing and/or
experimenting with new ideas.
- Safer to fix long-standing bugs requiring changing clock sources
(e.g., to resolve things like http://crbug.com/166153).
- Improved clock control for Headless Chrome (virtualized clocks,
when using the browser to render pages in a service framework).

Post by Tommy C. Li
Yuri - that's an interesting thought. Is it INfeasible?
For instance, what if we marked those as deprecated and added a
presubmit check to prevent new usages?

Post by Yuri Wiitala
+1 to Tommi's point.
As base/time OWNER, I see a lot of interesting CLs relating to test
flakiness, platform clock behaviors, etc. If it were feasible, I'd push for
the removal of all the global TimeXYZ::Now() functions; and instead require
dependency injection of time sources in all code.

Post by Tommy C. Li
I've found that using base::Clock in the production class greatly
improves testability, as then tests can inject in a SimpleTestClock.
Tommy
On Thursday, November 15, 2018 at 5:02:33 PM UTC-8, Sunny

Post by Sunny Sachanandani
If you're testing the execution of delayed tasks, you can probably
use base::TestMockTimeTaskRunner
<https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?l=67>,
and you'll probably need to provide a TickClock instance e.g.
SimpleTestTickClock
<https://cs.chromium.org/chromium/src/base/test/simple_test_tick_clock.h?l=18>
to the code being tested.

Post by s***@chromium.org

Post by s***@chromium.org
I recently wrote a unittest that used Time::Now() and it started failing
the day after daylights savings time <https://crbug.com/901698>. It
assumed the current day would have 24 hours, which in retrospect is clearly
not safe. But at the time, I didn't even consider the possibility.

It seems to me like putting Time::Now() in tests is dangerous and

Post by s***@chromium.org
should be stopped by a global PRESUBMIT. The disadvantages of Time::Now()
- Each test run is slightly different.
- Daylight Savings Time can cause temporary failures.
- Leap Year can cause temporary failures.
If running into this, I'd be careful to ask if this means that

the production code would also suffer from these problems. Code that is
measuring distance in time should generally be using TimeTicks, as Time is
not guaranteed to be monotonic, and jumps when the computer is put to
sleep, etc. Generally TimeTicks is the better choice unless you really want
to speak about wall-clock time. So for these reasons Time is dangerous in
tests, but also dangerous outside tests.

Post by s***@chromium.org
If you want your tests and impl to agree on the time, they
should be sharing a base::Clock. Or, seemingly more often, use
base::TimeTicks instead of base::Time.
There's been a thread (Testing base::OneShotTimer / "waiting"
in a browser test
<https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GyyJOmWGSVk>)
and a bug
<https://bugs.chromium.org/p/chromium/issues/detail?id=905412> about
process wide mocking of base::TimeTicks. But I don't this changes not
wanting Time::Now() in tests. And it doesn't sound like
<https://chromium-review.googlesource.com/c/chromium/src/+/1324414>
there are plans to process wide mock base::Time::Now().
Does anyone have concerns about this?
Thanks,
Sky
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the
Google Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/e78d75b2-7511-4605-bdaf-122214fac38e%40chromium.org?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the
Google Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaQZCMe-aWj2tZ81CatD3jQ%3DdbAMNw2VB_H5STCV%3DbO%2B2A%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/aa1eb417-dcc9-4606-b063-d258675de547%40chromium.org?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/34b0aa61-9c97-4f74-b04e-8e13f653d82c%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/34b0aa61-9c97-4f74-b04e-8e13f653d82c%40chromium.org?utm_medium=email&utm_source=footer>
.