Beacons for tracking default browser status.

chrome/installer/util/beacons_unittest.cc

Index: chrome/installer/util/beacons_unittest.cc
diff --git a/chrome/installer/util/beacons_unittest.cc b/chrome/installer/util/beacons_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..270553feed61381024fb2e5d0c3498e41c0e9d97
--- /dev/null
+++ b/chrome/installer/util/beacons_unittest.cc
@@ -0,0 +1,272 @@
+// Copyright 2015 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/beacons.h"
+
+#include "base/base_paths.h"
+#include "base/memory/scoped_vector.h"
+#include "base/path_service.h"
+#include "base/test/scoped_path_override.h"
+#include "base/test/test_reg_util_win.h"
+#include "base/win/registry.h"
+#include "base/win/win_util.h"
+#include "chrome/installer/util/browser_distribution.h"
+#include "chrome/installer/util/test_app_registration_data.h"
+#include "chrome/installer/util/util_constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::Bool;
+using ::testing::Combine;
+using ::testing::Values;
+
+class TestBeacon : public Beacon {

[gab, 2015/05/26 18:54:35]

So a TestBeacon is a Beacon with a public constructor essentially? Feels like a
weird way to "friend" a test.

I think I'd almost prefer a private Beacon constructor which explicitly friends
the test, no? (rather than having this weird "inherit me and you shall be my
friend" paradigm)

Maybe Erik has a better idea.

[grt (UTC plus 2), 2015/05/26 20:54:02]

I've seen and used this from time to time. I find friend distasteful. No one is
going to subclass it in Chrome, so the protected ctor is not a risk. I think
keeping the test names out of the public header is a good thing.

[gab, 2015/05/29 13:37:36]

Well at least friend puts the code owner of Beacon in control of who can build
it, whereas this paradigm allows anyone to essentially backdoor friend (without
any other use for inheritance). I understand why you find friend distasteful,
but to me this is just a hidden friend which IMO is even worse. Friend'ing tests
no matter how distasteful is at least a more common paradigm I'd say (i.e. 528
instances of FRIEND_TEST_ALL_PREFIXES() in chromium).

[grt (UTC plus 2), 2015/05/29 15:31:10]

Rejiggered as discussed.

+ public:
+  using BeaconType = Beacon::BeaconType;
+  using BeaconScope = Beacon::BeaconScope;
+
+  TestBeacon(base::StringPiece16 name,
+             BeaconType type,
+             BeaconScope scope,
+             bool system_install,
+             const AppRegistrationData& registration_data)
+      : Beacon(name, type, scope, system_install, registration_data) {}
+};
+
+// A test fixture that exercises a test beacon.
+class BeaconTest
+    : public ::testing::TestWithParam<::testing::tuple<TestBeacon::BeaconType,
+                                                       TestBeacon::BeaconScope,
+                                                       bool>> {

[gab, 2015/05/26 18:54:35]

I think you can name the parameters here (doesn't change anything just like
naming parameters in a header, but it helps the reader -- especially for the
bool)

[grt (UTC plus 2), 2015/05/26 20:54:02]

Do you mean BeaconType, BeaconScope, and bool? Those are template arguments. I
don't think there any other ways to name them.

[gab, 2015/05/29 13:37:37]

Ah ok, I guess I was recalling something like base::Callback<void(Foo foo, Bar
bar)> where you can give unused method parameters names, perhaps this doesn't
work on template arguments directly, if so, ignore me :-)

+ protected:
+  static const base::char16 kBeaconName[];
+
+  BeaconTest()
+      : beacon_type_(::testing::get<0>(GetParam())),
+        beacon_scope_(::testing::get<1>(GetParam())),
+        system_install_(::testing::get<2>(GetParam())),
+        beacon_(kBeaconName,
+                beacon_type_,
+                beacon_scope_,
+                system_install_,
+                app_registration_data_) {
+    // Override the registry so that tests can freely push state to it.
+    registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER);
+    registry_override_manager_.OverrideRegistry(HKEY_LOCAL_MACHINE);
+  }
+
+  TestAppRegistrationData app_registration_data_;
+  TestBeacon::BeaconType beacon_type_;
+  TestBeacon::BeaconScope beacon_scope_;
+  bool system_install_;
+  TestBeacon beacon_;
+  registry_util::RegistryOverrideManager registry_override_manager_;

[gab, 2015/05/26 18:54:35]

This member should be private IMO, no reason the tests should need to override
further.

[grt (UTC plus 2), 2015/05/26 20:54:01]

Done.

+};
+
+// static
+const base::char16 BeaconTest::kBeaconName[] = L"TestBeacon";
+
+// Nothing in the regsitry, so the becon should not exist.

[gab, 2015/05/26 18:54:35]

s/becon/beacon

[grt (UTC plus 2), 2015/05/26 20:54:02]

Done.

+TEST_P(BeaconTest, GetNonExistant) {
+  ASSERT_TRUE(beacon_.Get().is_null());
+}
+
+// Updating and then getting the beacon should return a value.
+TEST_P(BeaconTest, UpdateAndGet) {
+  beacon_.Update();
+  ASSERT_FALSE(beacon_.Get().is_null());
+}
+
+// Tests that updating a first beacon only updates it the first time, but doing
+// so for a last beacon always updates.
+TEST_P(BeaconTest, UpdateTwice) {
+  beacon_.Update();
+  base::Time beacon_time(beacon_.Get());
+
+  beacon_.Update();

[gab, 2015/05/26 18:54:35]

Should there be some kind of sleep before this Update to make sure the
timestamps saved do differ and make sure this test fails rather than become
flaky if it is ever broken.

[grt (UTC plus 2), 2015/05/26 20:54:01]

Done.

+  if (beacon_type_ == TestBeacon::BeaconType::FIRST) {
+    ASSERT_EQ(beacon_time, beacon_.Get());
+  } else {
+    ASSERT_NE(beacon_time, beacon_.Get());
+  }
+}
+
+// Tests that the beacon is written into the proper location in the registry.
+TEST_P(BeaconTest, Location) {
+  beacon_.Update();
+  HKEY right_root = system_install_ ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
+  HKEY wrong_root = system_install_ ? HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE;
+  base::string16 right_key;
+  base::string16 wrong_key;
+  base::string16 value_name;
+
+  if (beacon_scope_ == TestBeacon::BeaconScope::PER_INSTALL ||
+      !system_install_) {
+    value_name = kBeaconName;
+    right_key = app_registration_data_.GetStateKey();
+    wrong_key = app_registration_data_.GetStateMediumKey();
+  } else {
+    ASSERT_TRUE(base::win::GetUserSidString(&value_name));
+    right_key =
+        app_registration_data_.GetStateMediumKey() + L"\\" + kBeaconName;
+    wrong_key = app_registration_data_.GetStateKey();
+  }
+
+  // Keys should not exist in the wrong root or in the right root but wrong key.
+  ASSERT_FALSE(base::win::RegKey(wrong_root, right_key.c_str(),
+                                 KEY_READ).Valid()) << right_key;
+  ASSERT_FALSE(base::win::RegKey(wrong_root, wrong_key.c_str(),
+                                 KEY_READ).Valid()) << wrong_key;
+  ASSERT_FALSE(base::win::RegKey(right_root, wrong_key.c_str(),
+                                 KEY_READ).Valid()) << wrong_key;
+  // The right key should exist.
+  base::win::RegKey key(right_root, right_key.c_str(), KEY_READ);
+  ASSERT_TRUE(key.Valid()) << right_key;
+  // And should have the value.
+  ASSERT_TRUE(key.HasValue(value_name.c_str())) << value_name;

[gab, 2015/05/26 18:54:35]

Should also test that we can reconstruct a reasonable timestamp from the value
(i.e. one that is <= Now() - test_timeout)?

(to confirm that our timestamp math is correct -- ideally testing the Time
returned by Beacon's logic).

[grt (UTC plus 2), 2015/05/26 20:54:01]

Done in UpdateAndGet.

+}
+
+// Run the tests for all combinations of beacon type, scope, and install level.
+INSTANTIATE_TEST_CASE_P(BeaconTest,
+                        BeaconTest,
+                        Combine(Values(TestBeacon::BeaconType::FIRST,
+                                       TestBeacon::BeaconType::LAST),
+                                Values(TestBeacon::BeaconScope::PER_USER,
+                                       TestBeacon::BeaconScope::PER_INSTALL),
+                                Bool()));
+
+enum class DistributionVariant {
+  SYSTEM_LEVEL_CHROME,
+  USER_LEVEL_CHROME,
+  CHROME_SXS,

[gab, 2015/05/26 18:54:35]

I'd remove CHROME from all of these (since enum class already enforces an
explicit namespace prefix).

[grt (UTC plus 2), 2015/05/26 20:54:01]

Done.

+};
+
+class DefaultBrowserBeaconTest
+    : public ::testing::TestWithParam<DistributionVariant> {
+ protected:
+  using Super = ::testing::TestWithParam<DistributionVariant>;
+
+  DefaultBrowserBeaconTest()
+      : system_install_(GetParam() == DistributionVariant::SYSTEM_LEVEL_CHROME),
+        chrome_sxs_(GetParam() == DistributionVariant::CHROME_SXS),

[gab, 2015/05/26 18:54:35]

Curious, why is Chrome SxS different than regular user-level Chrome for the
Beacon code? It should behave the same, no? I guess you're confirming that it
does behave the same given it's in practice not all the same code in practice?

[grt (UTC plus 2), 2015/05/26 20:54:01]

It should be the same, yes.

+        chrome_exe_(GetChromePath()),
+        distribution_(nullptr) {}
+
+  void SetUp() override {
+    Super::SetUp();
+
+    // Override FILE_EXE so that various InstallUtil functions will consider
+    // this to be a user/system Chrome or Chrome SxS.
+    path_overrides_.push_back(new base::ScopedPathOverride(
+        base::FILE_EXE, chrome_exe_, true /* is_absolute */,
+        false /* !create */));
+
+    // Override these paths with their own values so that they can be found
+    // after the registry override manager is in place. Getting them would
+    // otherwise fail since the underlying calls to the OS need to see the real
+    // contents of the registry.
+    static const int kPathKeys[] = {
+        base::DIR_PROGRAM_FILES,
+        base::DIR_PROGRAM_FILESX86,
+        base::DIR_LOCAL_APP_DATA,
+    };
+    for (int key : kPathKeys) {
+      base::FilePath temp;
+      PathService::Get(key, &temp);
+      path_overrides_.push_back(new base::ScopedPathOverride(key, temp));
+    }
+
+    // Override the registry so that tests can freely push state to it.
+    registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER);
+    registry_override_manager_.OverrideRegistry(HKEY_LOCAL_MACHINE);
+
+    distribution_ = BrowserDistribution::GetDistribution();
+  }
+
+  base::FilePath GetChromePath() const {

[gab, 2015/05/26 18:54:35]

Move to private section.

And I'd call this GetChromePathForParams() or something (at first I thought this
was an install_util method per its generic name and was slightly confused)

[grt (UTC plus 2), 2015/05/26 20:54:01]

Done.

+    base::FilePath chrome_exe;
+    int dir_key = base::DIR_LOCAL_APP_DATA;
+
+    if (system_install_) {
+#if defined(_WIN64)
+      static const int kSystemKey = base::DIR_PROGRAM_FILESX86;
+#else
+      static const int kSystemKey = base::DIR_PROGRAM_FILES;
+#endif
+      dir_key = kSystemKey;
+    }
+    PathService::Get(dir_key, &chrome_exe);
+#if defined(GOOGLE_CHROME_BUILD)
+    chrome_exe = chrome_exe.Append(installer::kGoogleChromeInstallSubDir1);
+    if (chrome_sxs_) {
+      chrome_exe = chrome_exe.Append(
+          base::string16(installer::kGoogleChromeInstallSubDir2) +
+          installer::kSxSSuffix);
+    } else {
+      chrome_exe = chrome_exe.Append(installer::kGoogleChromeInstallSubDir2);
+    }
+#else
+    chrome_exe = chrome_exe.AppendASCII("Chromium");
+#endif
+    chrome_exe = chrome_exe.Append(installer::kInstallBinaryDir);
+    return chrome_exe.Append(installer::kChromeExe);
+  }
+
+  bool system_install_;
+  bool chrome_sxs_;
+  base::FilePath chrome_exe_;
+  ScopedVector<base::ScopedPathOverride> path_overrides_;
+  registry_util::RegistryOverrideManager registry_override_manager_;

[gab, 2015/05/26 18:54:35]

Move these two member to private:

[grt (UTC plus 2), 2015/05/26 20:54:02]

Done.

+  BrowserDistribution* distribution_;
+};
+
+// Tests that the default browser beacons work as expected.
+TEST_P(DefaultBrowserBeaconTest, Test) {

[gab, 2015/05/26 18:54:35]

s/Test/All ? (feels weird to have a test named "Test"...)

[grt (UTC plus 2), 2015/05/26 20:54:02]

"All" is more descriptive? Done.

[gab, 2015/05/29 13:37:36]

I find so, i.e. it tests "everything" rather than just it "tests" (something?).

Perhaps, "ConfirmBeaconsAsDesiredWhenToggling" is even more descriptive?!

[grt (UTC plus 2), 2015/05/29 15:31:10]

I'm okay with "All".

+  scoped_ptr<Beacon> last_was_default(Beacon::LastWasDefault(
+      system_install_, distribution_->GetAppRegistrationData()));
+  scoped_ptr<Beacon> first_not_default(Beacon::FirstNotDefault(
+      system_install_, distribution_->GetAppRegistrationData()));
+
+  ASSERT_TRUE(last_was_default->Get().is_null());
+  ASSERT_TRUE(first_not_default->Get().is_null());
+
+  // Chrome is not default.
+  UpdateDefaultBrowserBeacon(chrome_exe_, distribution_,
+                             ShellUtil::NOT_DEFAULT);
+  ASSERT_TRUE(last_was_default->Get().is_null());
+  ASSERT_FALSE(first_not_default->Get().is_null());
+
+  // Then it is.
+  UpdateDefaultBrowserBeacon(chrome_exe_, distribution_, ShellUtil::IS_DEFAULT);
+  ASSERT_FALSE(last_was_default->Get().is_null());
+  ASSERT_TRUE(first_not_default->Get().is_null());
+
+  // It still is.
+  UpdateDefaultBrowserBeacon(chrome_exe_, distribution_, ShellUtil::IS_DEFAULT);
+  ASSERT_FALSE(last_was_default->Get().is_null());
+  ASSERT_TRUE(first_not_default->Get().is_null());
+
+  // Now it's not again.
+  UpdateDefaultBrowserBeacon(chrome_exe_, distribution_,
+                             ShellUtil::NOT_DEFAULT);
+  ASSERT_FALSE(last_was_default->Get().is_null());
+  ASSERT_FALSE(first_not_default->Get().is_null());
+
+  // And it still isn't.
+  UpdateDefaultBrowserBeacon(chrome_exe_, distribution_,
+                             ShellUtil::NOT_DEFAULT);
+  ASSERT_FALSE(last_was_default->Get().is_null());
+  ASSERT_FALSE(first_not_default->Get().is_null());

[gab, 2015/05/26 18:54:35]

Shouldn't this test also check that the timestamps are as expected? (e.g.,
bounded between two timestamps taken in this test's logic perhaps?)

[grt (UTC plus 2), 2015/05/26 20:54:01]

BeaconTest::UpdateAndGet and UpdateTwice cover everything that testing the
timestamps here would cover.

+}
+
+INSTANTIATE_TEST_CASE_P(SystemLevelChrome,
+                        DefaultBrowserBeaconTest,
+                        Values(DistributionVariant::SYSTEM_LEVEL_CHROME));
+INSTANTIATE_TEST_CASE_P(UserLevelChrome,

[gab, 2015/05/26 18:54:35]

Oh cool, didn't know we could split various parametrized test instances like
that (now I see why the macro takes an "instance name" parameter!)

[grt (UTC plus 2), 2015/05/26 20:54:01]

Acknowledged.

+                        DefaultBrowserBeaconTest,
+                        Values(DistributionVariant::USER_LEVEL_CHROME));
+#if 0 && defined(GOOGLE_CHROME_BUILD)
+// Disabled for now since InstallUtil::IsChromeSxSProcess makes this impossible.

[gab, 2015/05/26 18:54:35]

Why? Can't we override DIR_EXE to fake this? i.e. with chrome_exe_.DirName():
IsChromeSxSProcess() seems to look at DIR_EXE rather than FILE_EXE

Or even override the command-line with --chrome-sxs (although this switch is
really only meant for the installer)

[grt (UTC plus 2), 2015/05/26 20:54:01]

That was what I hoped, except that it doesn't work due to the local static in
IsChromeSxSProcess. I do not wish to refactor that for testing today.

+INSTANTIATE_TEST_CASE_P(ChromeSxS, DefaultBrowserBeaconTest,
+                        Values(DistributionVariant::CHROME_SXS));
+#endif