Fix DeviceStatusCollectorTest.MaxStoredPeriods.

Fix DeviceStatusCollectorTest.MaxStoredPeriods.

BUG=157848
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168788

[Mattias Nissler (ping if slow), 2012-10-25 14:27:19]

https://codereview.chromium.org/11271024/diff/1/chrome/browser/policy/device_...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://codereview.chromium.org/11271024/diff/1/chrome/browser/policy/device_...
chrome/browser/policy/device_status_collector_unittest.cc:346: Time baseline =
Time::Now().LocalMidnight();
One thing: Should we convert these tests to starting at hardcoded date? The
current time is essentially indeterministic input to the test, so test
reproducibility is not guaranteed.

[bartfab (slow), 2012-10-25 14:30:49]

https://codereview.chromium.org/11271024/diff/1/chrome/browser/policy/device_...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://codereview.chromium.org/11271024/diff/1/chrome/browser/policy/device_...
chrome/browser/policy/device_status_collector_unittest.cc:336:
TEST_F(DeviceStatusCollectorTest, MaxStoredPeriods) {
This test has been disabled in r164043. You will need to rebase and re-enable
the test.

[Joao da Silva, 2012-10-25 15:24:17]

PTAL

https://codereview.chromium.org/11271024/diff/1/chrome/browser/policy/device_...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://codereview.chromium.org/11271024/diff/1/chrome/browser/policy/device_...
chrome/browser/policy/device_status_collector_unittest.cc:336:
TEST_F(DeviceStatusCollectorTest, MaxStoredPeriods) {
On 2012/10/25 14:30:49, bartfab wrote:
> This test has been disabled in r164043. You will need to rebase and re-enable
> the test.

Done.

https://codereview.chromium.org/11271024/diff/1/chrome/browser/policy/device_...
chrome/browser/policy/device_status_collector_unittest.cc:346: Time baseline =
Time::Now().LocalMidnight();
On 2012/10/25 14:27:19, Mattias Nissler wrote:
> One thing: Should we convert these tests to starting at hardcoded date? The
> current time is essentially indeterministic input to the test, so test
> reproducibility is not guaranteed.

That's a good idea. Done.

[bartfab (slow), 2012-10-26 09:35:08]

lgtm

[Patrick Dubroy, 2012-10-26 09:36:46]

https://codereview.chromium.org/11271024/diff/5002/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://codereview.chromium.org/11271024/diff/5002/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:204:
EXPECT_TRUE(TestBaselineTime() - timestamp < TimeDelta::FromMinutes(10));
Isn't the delta always going to be negative here?

[Joao da Silva, 2012-11-15 13:25:48]

@dubroy: PTAL

http://codereview.chromium.org/11271024/diff/5002/chrome/browser/policy/devic...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/11271024/diff/5002/chrome/browser/policy/devic...
chrome/browser/policy/device_status_collector_unittest.cc:204:
EXPECT_TRUE(TestBaselineTime() - timestamp < TimeDelta::FromMinutes(10));
On 2012/10/26 09:36:46, dubroy wrote:
> Isn't the delta always going to be negative here?

The timestamp is expected to be some time in the past, so this shouldn't
normally be negative. In this test, the timestamp is set to 4 or 5 minutes
before the baseline.

[Patrick Dubroy, 2012-11-15 13:53:01]

LGTM, though I'm not sure this test is really that useful. It's not testing
anything meaningful, just testing that data gets shuffled around properly.

https://codereview.chromium.org/11271024/diff/5002/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://codereview.chromium.org/11271024/diff/5002/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:204:
EXPECT_TRUE(TestBaselineTime() - timestamp < TimeDelta::FromMinutes(10));
On 2012/11/15 13:25:48, Joao da Silva wrote:
> On 2012/10/26 09:36:46, dubroy wrote:
> > Isn't the delta always going to be negative here?
> 
> The timestamp is expected to be some time in the past, so this shouldn't
> normally be negative. In this test, the timestamp is set to 4 or 5 minutes
> before the baseline.

Ah, I see.

[Joao da Silva, 2012-11-16 09:31:21]

So the reason this kept failing on the bot was due to its timezone. Running on
my workstation with TZ=US/Pacific triggers the bot failure. This is caused by
the timestamp relative to local midnight.

The latest patch set passes on the bots (see patch set 5) by overriding the
environment temporarily.

@dubroy: please have yet another look at the test :-)

@phajdan.jr: please review base/test; do you think this belongs there?

@thakis: please do an owner check on base/ and chrome/common/

Thanks!

[Patrick Dubroy, 2012-11-16 10:30:30]

lgtm

[Paweł Hajdan Jr., 2012-11-16 17:38:41]

LGTM!

[Nico, 2012-11-16 18:21:38]

lgtm, but I'm not a base owner

[commit-bot: I haz the power, 2012-11-16 18:22:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/11271024/16005

[commit-bot: I haz the power, 2012-11-16 18:22:11]

Failed to apply patch for
chrome/browser/policy/device_status_collector_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/policy/device_status_collector_unittest.cc
  Hunk #1 FAILED at 7.
  Hunk #2 succeeded at 37 (offset 1 line).
  Hunk #3 succeeded at 74 (offset 1 line).
  Hunk #4 succeeded at 104 (offset 1 line).
  Hunk #5 succeeded at 143 (offset 1 line).
  Hunk #6 succeeded at 209 (offset 5 lines).
  Hunk #7 succeeded at 233 (offset 5 lines).
  Hunk #8 succeeded at 350 (offset 5 lines).
  Hunk #9 succeeded at 360 (offset 5 lines).
  Hunk #10 succeeded at 393 (offset 5 lines).
  Hunk #11 succeeded at 420 (offset 5 lines).
  Hunk #12 succeeded at 505 (offset 5 lines).
  1 out of 12 hunks FAILED -- saving rejects to file
chrome/browser/policy/device_status_collector_unittest.cc.rej

Patch:       chrome/browser/policy/device_status_collector_unittest.cc
Index: chrome/browser/policy/device_status_collector_unittest.cc
diff --git a/chrome/browser/policy/device_status_collector_unittest.cc
b/chrome/browser/policy/device_status_collector_unittest.cc
index
8c5e0f4ee6954c80fd2fd6b68465990151d4da0c..89ba3490f5bebe67d03b7eb16768ad7f1bb17ea5
100644
--- a/chrome/browser/policy/device_status_collector_unittest.cc
+++ b/chrome/browser/policy/device_status_collector_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
+#include "base/test/scoped_environment_variable.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/cros_settings_names.h"
 #include "chrome/browser/chromeos/settings/cros_settings_provider.h"
@@ -36,8 +37,15 @@ namespace {
 
 const int64 kMillisecondsPerDay = Time::kMicrosecondsPerDay / 1000;
 
+const char kTimezoneEnvVariable[] = "TZ";
+const char kTestTimezone[] = "UTC";
+
 scoped_ptr<content::Geoposition> mock_position_to_return_next;
 
+Time TestBaselineTime() {
+  return Time::UnixEpoch() + TimeDelta::FromDays(42) + TimeDelta::FromHours(5);
+}
+
 void SetMockPositionToReturnNext(const content::Geoposition &position) {
   mock_position_to_return_next.reset(new content::Geoposition(position));
 }
@@ -66,9 +74,12 @@ class TestingDeviceStatusCollector : public
policy::DeviceStatusCollector {
       : policy::DeviceStatusCollector(local_state,
                                       provider,
                                       &MockPositionUpdateRequester) {
-    // Set the baseline time to a fixed value (1 AM) to prevent test flakiness
+    // Set the baseline time to a fixed value to prevent test flakiness
     // due to a single activity period spanning two days.
-    SetBaselineTime(Time::Now().LocalMidnight() + TimeDelta::FromHours(1));
+    SetBaselineTime(TestBaselineTime());
+
+    // Initialize the collector after setting the baseline time.
+    Init();
   }
 
   void Simulate(IdleState* states, int len) {
@@ -93,7 +104,7 @@ class TestingDeviceStatusCollector : public
policy::DeviceStatusCollector {
  protected:
   virtual void CheckIdleState() OVERRIDE {
     // This should never be called in testing, as it results in a dbus call.
-    NOTREACHED();
+    ADD_FAILURE();
   }
 
   // Each time this is called, returns a time that is a fixed increment
@@ -132,7 +143,8 @@ class DeviceStatusCollectorTest : public testing::Test {
     : message_loop_(MessageLoop::TYPE_UI),
       ui_thread_(content::BrowserThread::UI, &message_loop_),
       file_thread_(content::BrowserThread::FILE, &message_loop_),
-      io_thread_(content::BrowserThread::IO, &message_loop_) {
+      io_thread_(content::BrowserThread::IO, &message_loop_),
+      timezone_override_(kTimezoneEnvVariable, kTestTimezone) {
     TestingDeviceStatusCollector::RegisterPrefs(&prefs_);
 
     EXPECT_CALL(statistics_provider_, GetMachineStatistic(_, NotNull()))
@@ -193,8 +205,8 @@ class DeviceStatusCollectorTest : public testing::Test {
     EXPECT_DOUBLE_EQ(-7.8, location.longitude());
     EXPECT_DOUBLE_EQ(3., location.accuracy());
     // Check that the timestamp is not older than ten minutes.
-    EXPECT_TRUE(Time::Now() - Time::FromDoubleT(location.timestamp() / 1000.) <
-                TimeDelta::FromMinutes(10));
+    Time timestamp = Time::FromDoubleT(location.timestamp() / 1000.0);
+    EXPECT_TRUE(TestBaselineTime() - timestamp < TimeDelta::FromMinutes(10));
   }
 
   void CheckThatALocationErrorIsReported() {
@@ -217,6 +229,7 @@ class DeviceStatusCollectorTest : public testing::Test {
   content::TestBrowserThread file_thread_;
   content::TestBrowserThread io_thread_;
 
+  base::ScopedEnvironmentVariable timezone_override_;
   TestingPrefService prefs_;
   chromeos::system::MockStatisticsProvider statistics_provider_;
   scoped_ptr<TestingDeviceStatusCollector> status_collector_;
@@ -333,9 +346,7 @@ TEST_F(DeviceStatusCollectorTest, Times) {
   EXPECT_EQ(3 * ActivePeriodMilliseconds(), GetActiveMilliseconds(status_));
 }
 
-// Fails after after WebKit roll [132375:132450]
-// http://crbug.com/157848
-TEST_F(DeviceStatusCollectorTest, DISABLED_MaxStoredPeriods) {
+TEST_F(DeviceStatusCollectorTest, MaxStoredPeriods) {
   IdleState test_states[] = {
     IDLE_STATE_ACTIVE,
     IDLE_STATE_IDLE
@@ -345,7 +356,7 @@ TEST_F(DeviceStatusCollectorTest, DISABLED_MaxStoredPeriods)
{
   cros_settings_->SetBoolean(chromeos::kReportDeviceActivityTimes, true);
   status_collector_->set_max_stored_past_activity_days(max_days - 1);
   status_collector_->set_max_stored_future_activity_days(1);
-  Time baseline = Time::Now().LocalMidnight();
+  Time baseline = TestBaselineTime();
 
   // Simulate 12 active periods.
   for (int i = 0; i < static_cast<int>(max_days) + 2; i++) {
@@ -378,7 +389,7 @@ TEST_F(DeviceStatusCollectorTest, DISABLED_MaxStoredPeriods)
{
   // Check that we don't exceed the max number of periods.
   status_.clear_active_period();
   GetStatus();
-  EXPECT_LT(status_.active_period_size(), static_cast<int>(max_days));
+  EXPECT_LE(status_.active_period_size(), static_cast<int>(max_days));
 }
 
 TEST_F(DeviceStatusCollectorTest, ActivityTimesDisabledByDefault) {
@@ -405,7 +416,7 @@ TEST_F(DeviceStatusCollectorTest, ActivityCrossingMidnight)
{
 
   // Set the baseline time to 10 seconds after midnight.
   status_collector_->SetBaselineTime(
-      Time::Now().LocalMidnight() + TimeDelta::FromSeconds(10));
+      TestBaselineTime().LocalMidnight() + TimeDelta::FromSeconds(10));
 
   status_collector_->Simulate(test_states, 1);
   GetStatus();
@@ -490,12 +501,12 @@ TEST_F(DeviceStatusCollectorTest, Location) {
   valid_fix.latitude = 4.3;
   valid_fix.longitude = -7.8;
   valid_fix.accuracy = 3.;
-  valid_fix.timestamp = Time::Now();
+  valid_fix.timestamp = TestBaselineTime() - TimeDelta::FromMinutes(5);
 
   content::Geoposition invalid_fix;
   invalid_fix.error_code =
       content::Geoposition::ERROR_CODE_POSITION_UNAVAILABLE;
-  invalid_fix.timestamp = Time::Now();
+  invalid_fix.timestamp = TestBaselineTime() - TimeDelta::FromMinutes(4);
 
   // Check that when device location reporting is disabled, no location is
   // reported.

[Joao da Silva, 2012-11-16 18:29:09]

Thanks all for the reviews.

@jar: please have a look at the base/ changes, do you think this makes sense
there?

[jar (doing other things), 2012-11-16 20:29:22]

Generally things only go into base when we have a multitude of users of the
functionality.  I don't see that here, so it is probably best left where it was,
inside a unittest.cc file.

https://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/dev...
File chrome/browser/policy/device_status_collector.cc (right):

https://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/dev...
chrome/browser/policy/device_status_collector.cc:396: TimeDelta elapsed =
GetCurrentTime() - position_.timestamp;
The old code used Time::Now(), which is not a monotonic time source (it can jump
when we go in/out of daylight savings, and it jumps when a user "fixes" their
clock).

Is this the same flavor of time?  I wasn't sure what was being called... but it
seems to mask the probable issue.  

Most commonly, we use base::TimeTicks, which is guaranteed to be monotonically
increasing.... but I wasn't sure where postion_.timestamp came from.

https://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/dev...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

https://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/dev...
chrome/browser/policy/device_status_collector_unittest.cc:47: return
Time::UnixEpoch() + TimeDelta::FromDays(42) + TimeDelta::FromHours(5);
Can you add a comment indicating how these numbers were chosen, and what the
implications are?

https://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/dev...
chrome/browser/policy/device_status_collector_unittest.cc:397:
EXPECT_LE(status_.active_period_size(), static_cast<int>(max_days));
nit: It is strange that all but one use of |max_days| undergoes a cast.  Why is
this unsigned?... or perhaps, why is the slot that needs to be set in line 362
unsigned?

https://codereview.chromium.org/11271024/diff/14003/chrome/common/multi_proce...
File chrome/common/multi_process_lock_unittest.cc (left):

https://codereview.chromium.org/11271024/diff/14003/chrome/common/multi_proce...
chrome/common/multi_process_lock_unittest.cc:24: : name_(name),
environment_(base::Environment::Create()) {
nit: you could fix this up to have one initializer per line (you did it when you
pulled it into a separate file).

[jar (doing other things), 2012-11-16 21:38:22]

+cc Ryan, who may comment on the planned refactor to put the env changing tests
into their own subprocess.

[Ryan Sleevi, 2012-11-16 22:20:48]

So Jim briefly discussed this with me as an overview, and I gave him some
feedback that may or may not apply.

My comment was that using Environment->SetVar in unit tests is typically very
rare, and can at times be buggy (particularly when threads are involved or
third-party libraries that may be using environ or local copies). However, from
looking at this CL, presuming I've understood all the threading, I think you're
fine here.

As for general utility, the pattern you're using occurs only in a few places:
  - chrome/browser/locale_tests_browsertest.cc
  - chrome/browser/logging_chrome_browsertest.cc
  - chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc
  - chrome/common/multi_process_lock_unittest.cc
  - google_apis/google_api_keys_unittest.cc (probably broken here?)

As such, a better place to put this might actually be in chrome/test/base

That said, as it applies to this specific test, I think Jim's spot on the money
that instead of requiring TZ hackery, you should instead be looking at ways to
make the test (and the data supplied) be timezone independent. base::TimeTicks
is exactly that, and I think it's desirable for many reasons (included the
reason listed below)

I realize that GPS datasources typically include UTC timestamps, hence the
presumed inclusion of the timestamp parameter in the geolocation source, but
from the quick scan of this CL, it looks like you may be conflating the
geolocation source time with the local time, and I think in practice they may
differ - and sometimes wildly so. Designing the test (and functionality) to
reliably handle inconsistencies and skews seems like a better design overall.

https://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/dev...
File chrome/browser/policy/device_status_collector.cc (right):

https://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/dev...
chrome/browser/policy/device_status_collector.cc:396: TimeDelta elapsed =
GetCurrentTime() - position_.timestamp;
On 2012/11/16 20:29:23, jar wrote:
> The old code used Time::Now(), which is not a monotonic time source (it can
jump
> when we go in/out of daylight savings, and it jumps when a user "fixes" their
> clock).
> 
> Is this the same flavor of time?  I wasn't sure what was being called... but
it
> seems to mask the probable issue.  
> 
> Most commonly, we use base::TimeTicks, which is guaranteed to be monotonically
> increasing.... but I wasn't sure where postion_.timestamp came from.

I have to agree with Jim here. I think conflating the timestamp received from
the Geolocation source (line 438?) with the system time might be a mistake. It
seems much more reasonable to use base::TimeTicks to manage the polling interval
- to keep it at a true 30 minute interval - and then include in the report the
timesource according to GPS (presumably, a UTC timestamp)

Just a $.02 drive by

[Joao da Silva, 2012-11-20 09:10:51]

Thanks for the comments. I've scaled this back a bit to just fix the test and
enable it again. The only change required for that is to override the timezone,
and I've moved the file to the browser_tests target so that the tests run in a
separate process and don't override the environment for other tests.

I'm not familiar with this feature itself, and I'll leave the comments about the
time measurement issues to the original authors (@barfab, @dubroy). This CL is
just meant to enable the test again and get coverage for this.

@bartfab: this only touches policy/ now, please have another look.

http://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector.cc:396: TimeDelta elapsed =
GetCurrentTime() - position_.timestamp;
On 2012/11/16 22:20:48, Ryan Sleevi wrote:
> On 2012/11/16 20:29:23, jar wrote:
> > The old code used Time::Now(), which is not a monotonic time source (it can
> jump
> > when we go in/out of daylight savings, and it jumps when a user "fixes"
their
> > clock).
> > 
> > Is this the same flavor of time?  I wasn't sure what was being called... but
> it
> > seems to mask the probable issue.  
> > 

It's the same; GetCurrentTime() is a virtual that returns Time::Now() but can be
overridden in tests.

> > Most commonly, we use base::TimeTicks, which is guaranteed to be
monotonically
> > increasing.... but I wasn't sure where postion_.timestamp came from.
> 
> I have to agree with Jim here. I think conflating the timestamp received from
> the Geolocation source (line 438?) with the system time might be a mistake. It
> seems much more reasonable to use base::TimeTicks to manage the polling
interval
> - to keep it at a true 30 minute interval - and then include in the report the
> timesource according to GPS (presumably, a UTC timestamp)
> 
> Just a $.02 drive by

Those are good points. I'm not familiar with this code, adding @bartfab to
comment on that.

http://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/devi...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:47: return
Time::UnixEpoch() + TimeDelta::FromDays(42) + TimeDelta::FromHours(5);
On 2012/11/16 20:29:23, jar wrote:
> Can you add a comment indicating how these numbers were chosen, and what the
> implications are?

This has been removed. It was a misguided attempt to make this test pass under
any timezone.

http://codereview.chromium.org/11271024/diff/14003/chrome/browser/policy/devi...
chrome/browser/policy/device_status_collector_unittest.cc:397:
EXPECT_LE(status_.active_period_size(), static_cast<int>(max_days));
On 2012/11/16 20:29:23, jar wrote:
> nit: It is strange that all but one use of |max_days| undergoes a cast.  Why
is
> this unsigned?... or perhaps, why is the slot that needs to be set in line 362
> unsigned?

The casts have been removed from the test. Why the slot is unsigned has to be
asked to the original authors :-) @dubroy, do you have an idea about that?

[bartfab (slow), 2012-11-20 10:44:03]

Still lgtm - I am not one of the original authors though. I know little about
this test and I do not know how valuable it is. I may have fixed it up in the
past, that is all.

[commit-bot: I haz the power, 2012-11-20 10:45:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/11271024/17018

[Joao da Silva, 2012-11-20 10:46:22]

On 2012/11/20 10:44:03, bartfab wrote:
> Still lgtm - I am not one of the original authors though. I know little about
> this test and I do not know how valuable it is. I may have fixed it up in the
> past, that is all.

I thought you wrote the geolocation code?

[Mattias Nissler (ping if slow), 2012-11-20 11:26:49]

LGTM

[bartfab (slow), 2012-11-20 11:56:37]

Meh, terrible memory. Yes, that stuff I wrote :).

On 2012/11/20 10:46:22, Joao da Silva wrote:
> On 2012/11/20 10:44:03, bartfab wrote:
> > Still lgtm - I am not one of the original authors though. I know little
about
> > this test and I do not know how valuable it is. I may have fixed it up in
the
> > past, that is all.
> 
> I thought you wrote the geolocation code?

[commit-bot: I haz the power, 2012-11-20 12:51:37]

Change committed as 168788