Introduce UpdateActiveSetupVersionWorkItem

chrome/installer/util/update_active_setup_version_work_item_unittest.cc

+// 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/update_active_setup_version_work_item.h"
+
+#include <windows.h>
+
+#include "base/macros.h"
+#include "base/strings/string16.h"
+#include "base/test/test_reg_util_win.h"
+#include "base/win/registry.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::ValuesIn;
+
+namespace {
+
+const HKEY kActiveSetupRoot = HKEY_LOCAL_MACHINE;
+const base::char16 kActiveSetupPath[] = L"Active Setup\\test";
+
+struct UpdateActiveSetupVersionWorkItemTestCase {
+  // The initial value to be set in the registry prior to executing the
+  // UpdateActiveSetupVersionWorkItem. No value will be set if this null.
+  const wchar_t* initial_value;
+
+  // Whether to ask the UpdateActiveSetupVersionWorkItem to bump the OS_UPGRADES
+  // component of the Active Setup version.
+  bool bump_os_upgrades_component;
+
+  // The expected value after executing the UpdateActiveSetupVersionWorkItem.
+  const wchar_t* expected_result;
+};
+
+// Implements PrintTo in order for gtest to be able to print the problematic
+// UpdateActiveSetupVersionWorkItemTestCase on failure.
+void PrintTo(const UpdateActiveSetupVersionWorkItemTestCase& test_case,
+             ::std::ostream* os) {
+  *os << "Initial value: "

[grt (UTC plus 2), 2015/07/09 14:23:54]

#include <ostream>

[gab, 2015/07/09 20:36:38]

Done.

+      << (test_case.initial_value ? test_case.initial_value : L"(empty)")
+      << ", bump_os_upgrades_component: "
+      << test_case.bump_os_upgrades_component
+      << ", expected result: " << test_case.expected_result;
+}
+
+class UpdateActiveSetupVersionWorkItemTest

[grt (UTC plus 2), 2015/07/09 14:23:54]

i don't see an advantage to having this in an unnamed namespace. it makes it
harder to set breakpoints in the fixture in windbg, and makes it impossible to
friend the fixture as mentioned in the recent thread here:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/wzmsu....
although it isn't mandated, i have a strong preference for Kent's style
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/dY_73...)
in which the test fixture and individual tests are not in an unnamed namespace.
so, unless you have a compelling reason to leave it here, please take it out.

[gab, 2015/07/09 20:36:38]

Thanks for the reference, makes sense, I had always previously followed this
paradigm with only the tests outside the anonymous namespace, but I now see why
it's desired to have the fixture there as well.

+    : public testing::TestWithParam<UpdateActiveSetupVersionWorkItemTestCase> {
+ public:
+  UpdateActiveSetupVersionWorkItemTest() {}
+
+  void SetUp() override {
+    registry_override_manager_.OverrideRegistry(kActiveSetupRoot);
+  }
+
+ private:
+  registry_util::RegistryOverrideManager registry_override_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(UpdateActiveSetupVersionWorkItemTest);
+};
+
+}  // namespace
+
+TEST_P(UpdateActiveSetupVersionWorkItemTest, Execute) {

[grt (UTC plus 2), 2015/07/09 14:23:54]

this test is beautiful!

[gab, 2015/07/09 20:36:38]

:-)

+  // Get the parametrized |test_case| which defines 5 steps:
+  //   1) Maybe set an initial Active Setup version in the registry according to
+  //      |test_case.initial_value|.
+  //   2) Declare the work to be done by the UpdateActiveSetupVersionWorkItem
+  //      based on |test_case.bump_os_upgrades_component|.
+  //   3) Unconditionally execute the Active Setup work items.
+  //   4) Verify that the updated Active Setup version is as expected by
+  //      |test_case.expected_result|.
+  //   5) Rollback and verify that |test_case.initial_value| is back.
+  const UpdateActiveSetupVersionWorkItemTestCase& test_case = GetParam();
+
+  base::win::RegKey test_key;
+
+  EXPECT_EQ(ERROR_FILE_NOT_FOUND,

[grt (UTC plus 2), 2015/07/09 14:23:54]

should this be ASSERT_EQ? is there a reason to continue the test if this fails?

[gab, 2015/07/09 20:36:38]

Agreed, anything after a failure here is meaningless.

+            test_key.Open(kActiveSetupRoot, kActiveSetupPath, KEY_READ));
+
+  UpdateActiveSetupVersionWorkItem active_setup_work_item(
+      kActiveSetupPath, test_case.bump_os_upgrades_component
+                            ? UpdateActiveSetupVersionWorkItem::
+                                  UPDATE_AND_BUMP_OS_UPGRADES_COMPONENT
+                            : UpdateActiveSetupVersionWorkItem::UPDATE);
+
+  // Create the key and set the |initial_value| *after* the WorkItem to confirm
+  // that all of the work is done when executing the item, not when creating it.
+  EXPECT_EQ(ERROR_SUCCESS,

[grt (UTC plus 2), 2015/07/09 14:23:53]

ASSERT_EQ?

[gab, 2015/07/09 20:36:38]

Agreed, anything after a failure here is meaningless.

+            test_key.Create(kActiveSetupRoot, kActiveSetupPath, KEY_SET_VALUE));
+  if (test_case.initial_value) {
+    EXPECT_EQ(ERROR_SUCCESS,

[grt (UTC plus 2), 2015/07/09 14:23:54]

ASSERT_EQ

[gab, 2015/07/09 20:36:38]

Agreed, anything after a failure here is meaningless.

+              test_key.WriteValue(L"Version", test_case.initial_value));
+  }
+
+  active_setup_work_item.Do();

[grt (UTC plus 2), 2015/07/09 14:23:54]

EXPECT_TRUE?

[gab, 2015/07/09 20:36:38]

Done.

+
+  {
+    base::string16 version_out;
+    EXPECT_EQ(ERROR_SUCCESS, test_key.Open(kActiveSetupRoot, kActiveSetupPath,
+                                           KEY_QUERY_VALUE));
+    EXPECT_EQ(ERROR_SUCCESS, test_key.ReadValue(L"Version", &version_out));
+    EXPECT_EQ(test_case.expected_result, version_out);
+  }
+
+  active_setup_work_item.Rollback();
+
+  {
+    EXPECT_EQ(ERROR_SUCCESS, test_key.Open(kActiveSetupRoot, kActiveSetupPath,
+                                           KEY_QUERY_VALUE));
+
+    base::string16 version_out;
+    LONG read_result = test_key.ReadValue(L"Version", &version_out);
+    if (test_case.initial_value) {
+      EXPECT_EQ(ERROR_SUCCESS, read_result);
+      EXPECT_EQ(test_case.initial_value, version_out);
+    } else {
+      EXPECT_EQ(ERROR_FILE_NOT_FOUND, read_result);
+    }
+  }
+}
+
+const UpdateActiveSetupVersionWorkItemTestCase

[grt (UTC plus 2), 2015/07/09 14:23:54]

can you put this up with the struct definition? it'd make it easier to see what
the values mean.

[gab, 2015/07/09 20:36:38]

Done.

+    kUpdateActiveSetupVersionWorkItemTestCases[] = {
+        // Initial install.
+        {nullptr, false, L"43,0,0,0"},
+        // No-op update.
+        {L"43.0.0.0", false, L"43,0,0,0"},
+        // Update only major component.
+        {L"24,1,2,3", false, L"43,1,2,3"},
+        // Reset from bogus value.
+        {L"zzz", false, L"43,0,0,0"},
+        // Reset from invalid version (too few components).
+        {L"1,2", false, L"43,0,0,0"},
+        // Reset from invalid version (too many components).
+        {L"43,1,2,3,10", false, L"43,0,0,0"},
+        // Reset from empty string.
+        {L"", false, L"43,0,0,0"},
+
+        // Same tests with an OS_UPGRADES component bump.
+        {nullptr, true, L"43,0,1,0"},
+        {L"43.0.0.0", true, L"43,0,1,0"},
+        {L"24,1,2,3", true, L"43,1,3,3"},
+        {L"zzz", true, L"43,0,1,0"},
+        {L"1,2", true, L"43,0,1,0"},
+        {L"43,1,2,3,10", true, L"43,0,1,0"},
+        {L"", true, L"43,0,1,0"},
+        // Bumping a negative OS upgrade component should result in it being
+        // reset and subsequently bumped to 1 as usual.
+        {L"43,11,-123,33", true, L"43,11,1,33"},
+};
+
+INSTANTIATE_TEST_CASE_P(UpdateActiveSetupVersionWorkItemTestInstance,
+                        UpdateActiveSetupVersionWorkItemTest,
+                        ValuesIn(kUpdateActiveSetupVersionWorkItemTestCases));