Simplify WorkItemListTest.

chrome/installer/util/work_item_list_unittest.cc

Index: chrome/installer/util/work_item_list_unittest.cc
diff --git a/chrome/installer/util/work_item_list_unittest.cc b/chrome/installer/util/work_item_list_unittest.cc
index 1a2eb3d3060e9b7316061070c476ff7cdfc9884e..d074434a353fbefd331ab4192f6905e4137737ae 100644
--- a/chrome/installer/util/work_item_list_unittest.cc
+++ b/chrome/installer/util/work_item_list_unittest.cc
@@ -2,284 +2,203 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <windows.h>
+#include "chrome/installer/util/work_item_list.h"
+
+#include <algorithm>
+#include <memory>
+#include <vector>
 
-#include "base/base_paths.h"
-#include "base/files/file_util.h"
+#include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/strings/string_util.h"
-#include "base/test/test_reg_util_win.h"
-#include "base/win/registry.h"
+#include "base/macros.h"
 #include "chrome/installer/util/conditional_work_item_list.h"
 #include "chrome/installer/util/work_item.h"
-#include "chrome/installer/util/work_item_list.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using base::win::RegKey;
-
 namespace {
 
-const wchar_t kTestRoot[] = L"ListList";
-const wchar_t kDataStr[] = L"data_111";
-const wchar_t kName[] = L"name";
-
-class WorkItemListTest : public testing::Test {
- protected:
-  void SetUp() override {
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-    registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER);
+class DummyWorkItem : public WorkItem {
+ public:
+  DummyWorkItem(bool do_return_value,
+                std::vector<WorkItem*>* do_list,
+                std::vector<WorkItem*>* rollback_list)
+      : do_return_value_(do_return_value),
+        do_list_(do_list),
+        rollback_list_(rollback_list) {}
+
+  bool Do() override {
+    do_list_->push_back(this);
+    return do_return_value_;
   }
 
-  void TearDown() override { logging::CloseLogFile(); }
+  void Rollback() override { rollback_list_->push_back(this); }
 
-  base::ScopedTempDir temp_dir_;
-  registry_util::RegistryOverrideManager registry_override_manager_;
-};
+ private:
+  // Value returned by Do().
+  const bool do_return_value_;
 
-}  // namespace
+  // List to which this WorkItem is added when Do() runs.
+  std::vector<WorkItem*>* const do_list_;
 
-// Execute a WorkItem list successfully and then rollback.
-TEST_F(WorkItemListTest, ExecutionSuccess) {
-  scoped_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());
-  scoped_ptr<WorkItem> work_item;
-
-  base::FilePath top_dir_to_create(temp_dir_.path());
-  top_dir_to_create = top_dir_to_create.AppendASCII("a");
-  base::FilePath dir_to_create(top_dir_to_create);
-  dir_to_create = dir_to_create.AppendASCII("b");
-  ASSERT_FALSE(base::PathExists(dir_to_create));
-
-  work_item.reset(reinterpret_cast<WorkItem*>(
-      WorkItem::CreateCreateDirWorkItem(dir_to_create)));
-  work_item_list->AddWorkItem(work_item.release());
+  // List to which this WorkItem is added when RollbackImpl() runs.
+  std::vector<WorkItem*>* const rollback_list_;
 
-  std::wstring key_to_create(kTestRoot);
-  key_to_create.push_back(base::FilePath::kSeparators[0]);
-  key_to_create.append(L"ExecutionSuccess");
+  DISALLOW_COPY_AND_ASSIGN(DummyWorkItem);
+};
 
-  work_item.reset(
-      reinterpret_cast<WorkItem*>(WorkItem::CreateCreateRegKeyWorkItem(
-          HKEY_CURRENT_USER, key_to_create, WorkItem::kWow64Default)));
-  work_item_list->AddWorkItem(work_item.release());
+std::vector<WorkItem*> ReverseVector(const std::vector<WorkItem*>& in) {
+  std::vector<WorkItem*> out(in.size());

[grt (UTC plus 2), 2016/04/12 19:47:06]

how about:
  return std::vector<WorkItem*>(in.rbegin(), in.rend());

[fdoray, 2016/04/13 15:07:09]

n/a

+  std::reverse_copy(in.begin(), in.end(), out.begin());
+  return out;
+}
 
-  std::wstring name(kName);
-  std::wstring data(kDataStr);
-  work_item.reset(reinterpret_cast<WorkItem*>(
-      WorkItem::CreateSetRegValueWorkItem(HKEY_CURRENT_USER,
-                                          key_to_create,
-                                          WorkItem::kWow64Default,
-                                          name,
-                                          data,
-                                          false)));
-  work_item_list->AddWorkItem(work_item.release());
+}  // namespace
 
-  EXPECT_TRUE(work_item_list->Do());
+// Execute a WorkItem list successfully and then rollback.
+TEST(WorkItemListTest, ExecutionSuccess) {
+  std::unique_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());

[grt (UTC plus 2), 2016/04/12 19:47:06]

this seems like a perfect use of GoogleMock. i think the tests and the
expectations can be laid out very cleanly without loops and holding intermediate
state. what do you think?

{
  std::unique_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());

  // Create the mock work items.
  std::unique_ptr<MockWorkItem> item1(new MockWorkItem);
  std::unique_ptr<MockWorkItem> item2(new MockWorkItem);
  std::unique_ptr<MockWorkItem> item3(new MockWorkItem);

  {
    // Expect all three items to be done in order then undone.
    InSequence s;

    EXPECT_CALL(*item1, Do()).WillOnce(Return(true));
    EXPECT_CALL(*item2, Do()).WillOnce(Return(true));
    EXPECT_CALL(*item3, Do()).WillOnce(Return(true));
    EXPECT_CALL(*item3, Rollback());
    EXPECT_CALL(*item2, Rollback());
    EXPECT_CALL(*item1, Rollback());
  }

  // Add the items to the list.
  list->AddWorkItem(item1.release());
  list->AddWorkItem(item2.release());
  list->AddWorkItem(item3.release());

  // Do it!
  EXPECT_TRUE(work_item_list->Do());
  work_item_list->Rollback();
}

[fdoray, 2016/04/13 15:07:09]

Done.

+
+  std::vector<WorkItem*> do_list;
+  std::vector<WorkItem*> rollback_list;
+  std::vector<WorkItem*> expected_do_list;
+
+  // Work items that succeed.
+  for (int i = 0; i < 3; ++i) {
+    std::unique_ptr<WorkItem> work_item(
+        new DummyWorkItem(true, &do_list, &rollback_list));
+    expected_do_list.push_back(work_item.get());
+    work_item_list->AddWorkItem(work_item.release());
+  }
 
-  // Verify all WorkItems have been executed.
-  RegKey key;
-  EXPECT_EQ(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, key_to_create.c_str(), KEY_READ));
-  std::wstring read_out;
-  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(name.c_str(), &read_out));
-  EXPECT_EQ(0, read_out.compare(kDataStr));
-  key.Close();
-  EXPECT_TRUE(base::PathExists(dir_to_create));
+  EXPECT_TRUE(work_item_list->Do());
+  EXPECT_EQ(expected_do_list, do_list);
 
   work_item_list->Rollback();
 
-  // Verify everything is rolled back.
-  // The value must have been deleted first in roll back otherwise the key
-  // can not be deleted.
-  EXPECT_NE(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, key_to_create.c_str(), KEY_READ));
-  EXPECT_FALSE(base::PathExists(top_dir_to_create));
+  EXPECT_EQ(ReverseVector(expected_do_list), rollback_list);
 }
 
 // Execute a WorkItem list. Fail in the middle. Rollback what has been done.
-TEST_F(WorkItemListTest, ExecutionFailAndRollback) {
-  scoped_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());
-  scoped_ptr<WorkItem> work_item;
-
-  base::FilePath top_dir_to_create(temp_dir_.path());
-  top_dir_to_create = top_dir_to_create.AppendASCII("a");
-  base::FilePath dir_to_create(top_dir_to_create);
-  dir_to_create = dir_to_create.AppendASCII("b");
-  ASSERT_FALSE(base::PathExists(dir_to_create));
-
-  work_item.reset(reinterpret_cast<WorkItem*>(
-      WorkItem::CreateCreateDirWorkItem(dir_to_create)));
-  work_item_list->AddWorkItem(work_item.release());
-
-  std::wstring key_to_create(kTestRoot);
-  key_to_create.push_back(base::FilePath::kSeparators[0]);
-  key_to_create.append(L"ExecutionFail");
-
-  work_item.reset(
-      reinterpret_cast<WorkItem*>(WorkItem::CreateCreateRegKeyWorkItem(
-          HKEY_CURRENT_USER, key_to_create, WorkItem::kWow64Default)));
-  work_item_list->AddWorkItem(work_item.release());
+TEST(WorkItemListTest, ExecutionFailAndRollback) {
+  std::unique_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());
+
+  std::vector<WorkItem*> do_list;
+  std::vector<WorkItem*> rollback_list;
+  std::vector<WorkItem*> expected_do_list;
+
+  // Work items that succeed.
+  for (int i = 0; i < 3; ++i) {
+    std::unique_ptr<WorkItem> work_item(
+        new DummyWorkItem(true, &do_list, &rollback_list));
+    expected_do_list.push_back(work_item.get());
+    work_item_list->AddWorkItem(work_item.release());
+  }
 
-  std::wstring not_created_key(kTestRoot);
-  not_created_key.push_back(base::FilePath::kSeparators[0]);
-  not_created_key.append(L"NotCreated");
-  std::wstring name(kName);
-  std::wstring data(kDataStr);
-  work_item.reset(reinterpret_cast<WorkItem*>(
-      WorkItem::CreateSetRegValueWorkItem(HKEY_CURRENT_USER,
-                                          not_created_key,
-                                          WorkItem::kWow64Default,
-                                          name,
-                                          data,
-                                          false)));
+  // Work item that fails.
+  std::unique_ptr<WorkItem> work_item(
+      new DummyWorkItem(false, &do_list, &rollback_list));
+  expected_do_list.push_back(work_item.get());
   work_item_list->AddWorkItem(work_item.release());
 
-  // This one will not be executed because we will fail early.
-  work_item.reset(
-      reinterpret_cast<WorkItem*>(WorkItem::CreateCreateRegKeyWorkItem(
-          HKEY_CURRENT_USER, not_created_key, WorkItem::kWow64Default)));
-  work_item_list->AddWorkItem(work_item.release());
+  // More work items that fail.
+  for (int i = 0; i < 3; ++i) {
+    work_item_list->AddWorkItem(
+        new DummyWorkItem(false, &do_list, &rollback_list));
+  }
 
   EXPECT_FALSE(work_item_list->Do());
-
-  // Verify the first 2 WorkItems have been executed.
-  RegKey key;
-  EXPECT_EQ(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, key_to_create.c_str(), KEY_READ));
-  key.Close();
-  EXPECT_TRUE(base::PathExists(dir_to_create));
-  // The last one should not be there.
-  EXPECT_NE(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, not_created_key.c_str(), KEY_READ));
+  EXPECT_EQ(expected_do_list, do_list);
 
   work_item_list->Rollback();
-
-  // Verify everything is rolled back.
-  EXPECT_NE(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, key_to_create.c_str(), KEY_READ));
-  EXPECT_FALSE(base::PathExists(top_dir_to_create));
+  EXPECT_EQ(ReverseVector(expected_do_list), rollback_list);
 }
 
-TEST_F(WorkItemListTest, ConditionalExecutionSuccess) {
-  scoped_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());
-  scoped_ptr<WorkItem> work_item;
-
-  base::FilePath top_dir_to_create(temp_dir_.path());
-  top_dir_to_create = top_dir_to_create.AppendASCII("a");
-  base::FilePath dir_to_create(top_dir_to_create);
-  dir_to_create = dir_to_create.AppendASCII("b");
-  ASSERT_FALSE(base::PathExists(dir_to_create));
-
-  work_item.reset(reinterpret_cast<WorkItem*>(
-      WorkItem::CreateCreateDirWorkItem(dir_to_create)));
-  work_item_list->AddWorkItem(work_item.release());
+TEST(WorkItemListTest, ConditionalExecutionSuccess) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
-  scoped_ptr<WorkItemList> conditional_work_item_list(
+  std::unique_ptr<WorkItemList> conditional_work_item_list(
       WorkItem::CreateConditionalWorkItemList(
-          new ConditionRunIfFileExists(dir_to_create)));
-
-  std::wstring key_to_create(kTestRoot);
-  key_to_create.push_back(base::FilePath::kSeparators[0]);
-  key_to_create.append(L"ExecutionSuccess");
-  work_item.reset(
-      reinterpret_cast<WorkItem*>(WorkItem::CreateCreateRegKeyWorkItem(
-          HKEY_CURRENT_USER, key_to_create, WorkItem::kWow64Default)));
-  conditional_work_item_list->AddWorkItem(work_item.release());
-
-  std::wstring name(kName);
-  std::wstring data(kDataStr);
-  work_item.reset(reinterpret_cast<WorkItem*>(
-      WorkItem::CreateSetRegValueWorkItem(HKEY_CURRENT_USER,
-                                          key_to_create,
-                                          WorkItem::kWow64Default,
-                                          name,
-                                          data,
-                                          false)));
-  conditional_work_item_list->AddWorkItem(work_item.release());
-
-  work_item_list->AddWorkItem(conditional_work_item_list.release());
-
-  EXPECT_TRUE(work_item_list->Do());
-
-  // Verify all WorkItems have been executed.
-  RegKey key;
-  EXPECT_EQ(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, key_to_create.c_str(), KEY_READ));
-  std::wstring read_out;
-  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(name.c_str(), &read_out));
-  EXPECT_EQ(0, read_out.compare(kDataStr));
-  key.Close();
-  EXPECT_TRUE(base::PathExists(dir_to_create));
+          new ConditionRunIfFileExists(temp_dir.path())));
+
+  std::vector<WorkItem*> do_list;
+  std::vector<WorkItem*> rollback_list;
+  std::vector<WorkItem*> expected_do_list;
+
+  // Work items that succeed.
+  for (int i = 0; i < 3; ++i) {
+    std::unique_ptr<WorkItem> work_item(
+        new DummyWorkItem(true, &do_list, &rollback_list));
+    expected_do_list.push_back(work_item.get());
+    conditional_work_item_list->AddWorkItem(work_item.release());
+  }
 
-  work_item_list->Rollback();
+  EXPECT_TRUE(conditional_work_item_list->Do());
+  EXPECT_EQ(expected_do_list, do_list);
 
-  // Verify everything is rolled back.
-  // The value must have been deleted first in roll back otherwise the key
-  // can not be deleted.
-  EXPECT_NE(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, key_to_create.c_str(), KEY_READ));
-  EXPECT_FALSE(base::PathExists(top_dir_to_create));
+  conditional_work_item_list->Rollback();
+  EXPECT_EQ(ReverseVector(expected_do_list), rollback_list);
 }
 
-TEST_F(WorkItemListTest, ConditionalExecutionConditionFailure) {
-  scoped_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());
-  scoped_ptr<WorkItem> work_item;
+TEST(WorkItemListTest, ConditionalExecutionFailAndRollback) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
-  base::FilePath top_dir_to_create(temp_dir_.path());
-  top_dir_to_create = top_dir_to_create.AppendASCII("a");
-  base::FilePath dir_to_create(top_dir_to_create);
-  dir_to_create = dir_to_create.AppendASCII("b");
-  ASSERT_FALSE(base::PathExists(dir_to_create));
-
-  work_item.reset(reinterpret_cast<WorkItem*>(
-      WorkItem::CreateCreateDirWorkItem(dir_to_create)));
-  work_item_list->AddWorkItem(work_item.release());
-
-  scoped_ptr<WorkItemList> conditional_work_item_list(
+  std::unique_ptr<WorkItemList> conditional_work_item_list(
       WorkItem::CreateConditionalWorkItemList(
-          new ConditionRunIfFileExists(dir_to_create.AppendASCII("c"))));
-
-  std::wstring key_to_create(kTestRoot);
-  key_to_create.push_back(base::FilePath::kSeparators[0]);
-  key_to_create.append(L"ExecutionSuccess");
-  work_item.reset(
-      reinterpret_cast<WorkItem*>(WorkItem::CreateCreateRegKeyWorkItem(
-          HKEY_CURRENT_USER, key_to_create, WorkItem::kWow64Default)));
-  conditional_work_item_list->AddWorkItem(work_item.release());
+          new ConditionRunIfFileExists(temp_dir.path())));
+
+  std::vector<WorkItem*> do_list;
+  std::vector<WorkItem*> rollback_list;
+  std::vector<WorkItem*> expected_do_list;
+
+  // Work items that succeed.
+  for (int i = 0; i < 3; ++i) {
+    std::unique_ptr<WorkItem> work_item(
+        new DummyWorkItem(true, &do_list, &rollback_list));
+    expected_do_list.push_back(work_item.get());
+    conditional_work_item_list->AddWorkItem(work_item.release());
+  }
 
-  std::wstring name(kName);
-  std::wstring data(kDataStr);
-  work_item.reset(reinterpret_cast<WorkItem*>(
-      WorkItem::CreateSetRegValueWorkItem(HKEY_CURRENT_USER,
-                                          key_to_create,
-                                          WorkItem::kWow64Default,
-                                          name,
-                                          data,
-                                          false)));
+  // Work item that fails.
+  std::unique_ptr<WorkItem> work_item(
+      new DummyWorkItem(false, &do_list, &rollback_list));
+  expected_do_list.push_back(work_item.get());
   conditional_work_item_list->AddWorkItem(work_item.release());
 
-  work_item_list->AddWorkItem(conditional_work_item_list.release());
+  // More work items that fail.
+  for (int i = 0; i < 3; ++i) {
+    conditional_work_item_list->AddWorkItem(
+        new DummyWorkItem(false, &do_list, &rollback_list));
+  }
 
-  EXPECT_TRUE(work_item_list->Do());
+  EXPECT_FALSE(conditional_work_item_list->Do());
+  EXPECT_EQ(expected_do_list, do_list);
 
-  // Verify that the WorkItems added as part of the conditional list have NOT
-  // been executed.
-  RegKey key;
-  EXPECT_NE(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, key_to_create.c_str(), KEY_READ));
-  std::wstring read_out;
-  EXPECT_NE(ERROR_SUCCESS, key.ReadValue(name.c_str(), &read_out));
-  key.Close();
+  conditional_work_item_list->Rollback();
+  EXPECT_EQ(ReverseVector(expected_do_list), rollback_list);
+}
 
-  // Verify that the other work item was executed.
-  EXPECT_TRUE(base::PathExists(dir_to_create));
+TEST(WorkItemListTest, ConditionalExecutionConditionFailure) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
-  work_item_list->Rollback();
+  std::unique_ptr<WorkItemList> conditional_work_item_list(
+      WorkItem::CreateConditionalWorkItemList(new ConditionRunIfFileExists(
+          temp_dir.path().Append(FILE_PATH_LITERAL("DoesNotExist")))));
+
+  std::vector<WorkItem*> do_list;
+  std::vector<WorkItem*> rollback_list;
+
+  // Work items that shouldn't run because the condition isn't met.
+  for (int i = 0; i < 3; ++i) {
+    conditional_work_item_list->AddWorkItem(
+        new DummyWorkItem(true, &do_list, &rollback_list));
+  }
+
+  EXPECT_TRUE(conditional_work_item_list->Do());
+  EXPECT_TRUE(do_list.empty());
 
-  // Verify everything is rolled back.
-  // The value must have been deleted first in roll back otherwise the key
-  // can not be deleted.
-  EXPECT_NE(ERROR_SUCCESS,
-      key.Open(HKEY_CURRENT_USER, key_to_create.c_str(), KEY_READ));
-  EXPECT_FALSE(base::PathExists(top_dir_to_create));
+  conditional_work_item_list->Rollback();
+  EXPECT_TRUE(rollback_list.empty());
 }