Win 10 Inactive toast experiment metrics and storage modifications.

chrome/installer/util/experiment_unittest.cc

Index: chrome/installer/util/experiment_unittest.cc
diff --git a/chrome/installer/util/experiment_unittest.cc b/chrome/installer/util/experiment_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..850dc9a6bb00296268b8bc49800acf8323971b4b
--- /dev/null
+++ b/chrome/installer/util/experiment_unittest.cc
@@ -0,0 +1,94 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/installer/util/experiment.h"
+
+#include <cmath>
+
+#include "base/time/time.h"
+#include "chrome/installer/util/experiment_metrics.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace installer {
+
+class ExperimentTest : public ::testing::Test {
+ protected:
+  void TestExperimentMetrics(ExperimentMetrics* metrics) {

[grt (UTC plus 2), 2017/05/31 12:24:10]

nit: this name makes me think that this function is somehow testing |metrics|.
how about PopulateMetricsWithTestData? alternatively, move this initialization
into TestInitializeFromMetrics since that's the only place it's used. doing so
makes it easier to eyeball the various checks there.

[nikunjb, 2017/06/02 05:11:24]

This is no longer needed for this test.

+    metrics->SetState(ExperimentMetrics::kGroupAssigned);
+    metrics->group = 5;
+    metrics->toast_location = ExperimentMetrics::kOverTaskbarPin;
+    metrics->toast_count = 1;
+    metrics->first_toast_offset = 30;
+    metrics->toast_hour = 3;
+    metrics->last_used_bucket = 126;      // Corresponds to 1621 days inactive.
+    metrics->display_time_bucket = 25;    // Corresponds to 32875 sec
+    metrics->session_length_bucket = 50;  // Corresponds to 3962 min
+  }
+};
+
+TEST_F(ExperimentTest, TestInitializeFromMetrics) {
+  ExperimentMetrics metrics;
+  TestExperimentMetrics(&metrics);
+  Experiment experiment;
+  experiment.InitializeFromMetrics(metrics);
+
+  ASSERT_EQ(metrics.state, experiment.state());

[grt (UTC plus 2), 2017/05/31 12:24:10]

please use EXPECT_* rather than ASSERT_* for cases where it's okay for the test
to proceed on failure. in these cases here, there's no reason not to evaluate
all of the conditions if any one fails. ASSERT_* is great for making sure the
test stops if some prerequisite isn't satisfied.

[nikunjb, 2017/06/02 05:11:24]

Done.

+  ASSERT_EQ(metrics.group, experiment.group());
+  ASSERT_EQ(metrics.toast_location, experiment.toast_location());
+  ASSERT_EQ(1621, experiment.inactive_days());
+  ASSERT_EQ(metrics.toast_count, experiment.toast_count());
+  base::Time expected_display_time =
+      (base::Time::UnixEpoch() +
+       base::TimeDelta::FromSeconds(
+           ExperimentMetrics::kExperimentStartSeconds) +
+       base::TimeDelta::FromDays(metrics.first_toast_offset));
+  ASSERT_EQ(expected_display_time, experiment.first_display_time());
+  ASSERT_EQ(expected_display_time, experiment.latest_display_time());
+  ASSERT_EQ(base::TimeDelta::FromMinutes(3962),
+            experiment.user_session_uptime());
+  ASSERT_EQ(base::TimeDelta::FromSeconds(32875), experiment.action_delay());
+}
+
+TEST_F(ExperimentTest, TestSetters) {

[grt (UTC plus 2), 2017/05/31 12:24:10]

would you break this up into individual test functions for each Experiment
method? it's easier to audit them and will be more sensible should one of them
start failing down the road.

[nikunjb, 2017/06/02 05:11:24]

Done. Added separate test.

Left this test as it is since it is one overall test with all setter.

+  Experiment experiment;
+  // Set experiment states so that the generated experiment metrics
+  // is similar to TestExperimentMetrics.
+  experiment.AssignGroup(5);
+  experiment.SetState(ExperimentMetrics::kGroupAssigned);
+  experiment.SetToastLocation(ExperimentMetrics::kOverTaskbarPin);
+  experiment.SetInactiveDays(1621);
+  experiment.SetToastCount(1);
+  base::Time test_display_time =
+      (base::Time::UnixEpoch() +
+       base::TimeDelta::FromSeconds(
+           ExperimentMetrics::kExperimentStartSeconds) +
+       base::TimeDelta::FromDays(30) + base::TimeDelta::FromHours(3));
+  experiment.SetDisplayTime(test_display_time);

[grt (UTC plus 2), 2017/05/31 12:24:10]

please add a test to confirm that SetDisplayTime(25 may 2017) results in
experiment.metrics().first_toast_offset == 0.

[nikunjb, 2017/06/02 05:11:24]

Added in TestSetDisplayTime. (Although note that this is an invalid value, first
valid cohort is 26th May. I haven't added a DCHECK for this because this is
almost always the case since the day has passed.)

+  experiment.SetUserSessionUptime(base::TimeDelta::FromMinutes(3962));
+  experiment.SetActionDelay(base::TimeDelta::FromSeconds(32875));
+
+  ASSERT_EQ(ExperimentMetrics::kGroupAssigned, experiment.state());
+  ASSERT_EQ(5, experiment.group());
+  ASSERT_EQ(ExperimentMetrics::kOverTaskbarPin, experiment.toast_location());
+  ASSERT_EQ(1621, experiment.inactive_days());
+  ASSERT_EQ(1, experiment.toast_count());
+  ASSERT_EQ(test_display_time, experiment.first_display_time());
+  ASSERT_EQ(test_display_time, experiment.latest_display_time());
+  ASSERT_EQ(base::TimeDelta::FromMinutes(3962),
+            experiment.user_session_uptime());
+  ASSERT_EQ(base::TimeDelta::FromSeconds(32875), experiment.action_delay());
+
+  ASSERT_EQ(ExperimentMetrics::kGroupAssigned, experiment.metrics().state);
+  ASSERT_EQ(5, experiment.metrics().group);
+  ASSERT_EQ(ExperimentMetrics::kOverTaskbarPin,
+            experiment.metrics().toast_location);
+  ASSERT_EQ(1, experiment.metrics().toast_count);
+  ASSERT_EQ(30, experiment.metrics().first_toast_offset);
+  ASSERT_EQ(3, experiment.metrics().toast_hour);

[grt (UTC plus 2), 2017/05/31 12:24:10]

this one will be hard to test since it's tz-dependent. rather than testing an
explicit value, how about testing that the value from two toastings an hour
apart differ by one? alternatively, use Time::FromLocalExploded to compute a
display time specifically for testing this value.

[nikunjb, 2017/06/02 05:11:24]

Removed toast hour check from here and added a separate test for individual
setter (TestSetDisplayTime).

+  ASSERT_EQ(126, experiment.metrics().last_used_bucket);
+  ASSERT_EQ(25, experiment.metrics().display_time_bucket);
+  ASSERT_EQ(50, experiment.metrics().session_length_bucket);
+}
+
+}  // namespace installer