Time::Now() in tests

[sk...@chromium.org]

I recently wrote a unittest that used Time::Now() and it started failing the day after daylights savings time. 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) and a bug 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 there are plans to process wide mock base::Time::Now().

Does anyone have concerns about this?

Thanks,

Sky

[Alex Clarke]

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

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
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 email to chromium-dev...@chromium.org.
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.

[dan...@chromium.org]

On Wed, Nov 14, 2018 at 8:28 PM <sk...@chromium.org> wrote:

I recently wrote a unittest that used Time::Now() and it started failing the day after daylights savings time. 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 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.

 

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) and a bug 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 there are plans to process wide mock base::Time::Now().

Does anyone have concerns about this?

Thanks,

Sky

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
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.

[Sunny Sachanandani]

If you're testing the execution of delayed tasks, you can probably use base::TestMockTimeTaskRunner, and you'll probably need to provide a TickClock instance e.g. SimpleTestTickClock to the code being tested.

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.

[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

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

[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?

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

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.

[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

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

[Alex Clarke]

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. I'm under the impression we can do something very similar controlled by ScopedTaskEnvironment.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

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.

[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

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

[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_);

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.

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