Win 10 Inactive toast experiment metrics and storage modifications.

chrome/installer/util/experiment_storage_unittest.cc

Index: chrome/installer/util/experiment_storage_unittest.cc
diff --git a/chrome/installer/util/experiment_storage_unittest.cc b/chrome/installer/util/experiment_storage_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..064cc1e0c03616a95e8335725aedaf2a5733ca81
--- /dev/null
+++ b/chrome/installer/util/experiment_storage_unittest.cc
@@ -0,0 +1,109 @@
+// 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_storage.h"
+
+#include "base/base64.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/test_reg_util_win.h"
+#include "chrome/install_static/install_details.h"
+#include "chrome/install_static/test/scoped_install_details.h"
+#include "chrome/installer/util/experiment.h"
+#include "chrome/installer/util/experiment_metrics.h"
+#include "chrome/installer/util/google_update_settings.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace installer {
+
+class ExperimentStorageTest : public ::testing::TestWithParam<bool> {

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

please document the parameter

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

Done.

+ protected:
+  ExperimentStorageTest()
+      : system_level_install_(GetParam()),
+        scoped_install_details_(system_level_install_, 0) {}
+
+  void TestExperimentMetrics(ExperimentMetrics* metrics) {

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

same comment regarding this function name as in other file

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

Done.

+    metrics->SetState(ExperimentMetrics::kGroupAssigned);
+    metrics->toast_location = ExperimentMetrics::kOverTaskbarPin;
+    metrics->toast_count = 1;
+    metrics->first_toast_offset = 30;
+    metrics->toast_hour = 3;
+    metrics->last_used_bucket = 2;
+    metrics->display_time_bucket = 11;
+    metrics->session_length_bucket = 36;
+  }
+
+  void SetUp() {

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

nit: override

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

Done.

+    ::testing::TestWithParam<bool>::SetUp();
+    HKEY root = system_level_install_ ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
+    ASSERT_NO_FATAL_FAILURE(override_manager_.OverrideRegistry(root));
+
+    // Create an empty participation key since participation registry is assumed
+    // to be present for chrome build.
+    base::win::RegKey key;
+    key.Create(

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

ASSERT_EQ(ERROR_SUCCESS, key.Create(..));

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

Done.

+        root,
+        install_static::InstallDetails::Get().GetClientStateKeyPath().c_str(),
+        KEY_QUERY_VALUE);

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

KEY_WOW64_32KEY | KEY_QUERY_VALUE

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

Done.

+  }
+
+ private:
+  bool system_level_install_;
+  install_static::ScopedInstallDetails scoped_install_details_;
+  registry_util::RegistryOverrideManager override_manager_;
+};

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

DISALLOW_COPY_AND_ASSIGN

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

Done.

+
+TEST_P(ExperimentStorageTest, TestEncodeDecodeMetrics) {
+  ExperimentMetrics metrics;
+  TestExperimentMetrics(&metrics);
+  base::string16 encoded_metrics(ExperimentStorage::EncodeMetrics(metrics));
+  EXPECT_EQ(encoded_metrics, base::ASCIIToUTF16("5BIMD4IA"));

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

base::ASCIIToUTF16("5BIMD4IA") -> L"5BIMD4IA"

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

Done.

+  ExperimentMetrics decoded_metrics;
+  ASSERT_TRUE(
+      ExperimentStorage::DecodeMetrics(encoded_metrics, &decoded_metrics));
+  EXPECT_EQ(encoded_metrics, ExperimentStorage::EncodeMetrics(decoded_metrics));

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

?  EXPECT_EQ(metrics, decoded_metrics);

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

Was checking by re-encoding the decoding the metrics. Although agree it is
better to check the individual values directly. Done.

+}

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

could you add a test for an instance with every field at max value?

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

Done. Added one for max and one for all 0.

+
+TEST_P(ExperimentStorageTest, TestReadWriteParticipation) {
+  ExperimentStorage storage;
+  ExperimentStorage::Participation expected =
+      ExperimentStorage::Participation::kIsParticipating;
+  ASSERT_TRUE(storage.AcquireLock()->WriteParticipation(expected));
+  ExperimentStorage::Participation p;
+  ASSERT_TRUE(storage.AcquireLock()->ReadParticipation(&p));
+  EXPECT_EQ(p, expected);
+}
+
+TEST_P(ExperimentStorageTest, TestLoadStoreExperiment) {
+  Experiment experiment;
+  ExperimentMetrics metrics;
+  TestExperimentMetrics(&metrics);
+  experiment.InitializeFromMetrics(metrics);
+  ExperimentStorage storage;
+  ASSERT_TRUE(storage.AcquireLock()->StoreExperiment(experiment));
+  Experiment stored_experiment;
+  ASSERT_TRUE(storage.AcquireLock()->LoadExperiment(&stored_experiment));
+  EXPECT_EQ(stored_experiment.state(), ExperimentMetrics::kGroupAssigned);

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

can you compare the two instances directly (EXPECT_EQ(experiment,
stored_experiment);)?

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

Done. (Tested with a small number of values set for experiment, since it is only
testing experiment Load/store are working. Added more cases for Experiment class
setters separately)

+}
+
+TEST_P(ExperimentStorageTest, TestLoadStoreMetrics) {
+  ExperimentStorage storage;
+  ExperimentMetrics metrics;
+  TestExperimentMetrics(&metrics);
+  ASSERT_TRUE(storage.AcquireLock()->StoreMetrics(metrics));
+  ExperimentMetrics stored_metrics;
+  ASSERT_TRUE(storage.AcquireLock()->LoadMetrics(&stored_metrics));
+  EXPECT_EQ(ExperimentStorage::EncodeMetrics(stored_metrics),
+            base::ASCIIToUTF16("5BIMD4IA"));
+  EXPECT_EQ(ExperimentStorage::EncodeMetrics(stored_metrics),

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

could you compare the instances rather than their encodings?

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

Done. Added an operator== in ExperimentMetrics (Since it is a struct storing
plain data it can be generally useful outside test).

+            ExperimentStorage::EncodeMetrics(metrics));
+}
+
+INSTANTIATE_TEST_CASE_P(UserLevel,
+                        ExperimentStorageTest,
+                        ::testing::Values(false));
+
+INSTANTIATE_TEST_CASE_P(SystemLevel,
+                        ExperimentStorageTest,
+                        ::testing::Values(true));
+}  // namespace installer

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

nit: blank line above this

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

Done.