Simplify ExtensionInstallChecker into a single-use class

chrome/browser/extensions/extension_install_checker_unittest.cc

Index: chrome/browser/extensions/extension_install_checker_unittest.cc
diff --git a/chrome/browser/extensions/extension_install_checker_unittest.cc b/chrome/browser/extensions/extension_install_checker_unittest.cc
index f70d9ddce34242be4c5b38bee2b6ca13b7b94b3f..26c0c044985ca6996425e45161d44b25ad1b7fc0 100644
--- a/chrome/browser/extensions/extension_install_checker_unittest.cc
+++ b/chrome/browser/extensions/extension_install_checker_unittest.cc
@@ -18,10 +18,6 @@ const BlacklistState kBlacklistStateError = BLACKLISTED_MALWARE;
 const char kDummyRequirementsError[] = "Requirements error";
 const char kDummyPolicyError[] = "Cannot install extension";
 
-const char kDummyPolicyError2[] = "Another policy error";
-const char kDummyRequirementsError2[] = "Another requirements error";
-const BlacklistState kBlacklistState2 = BLACKLISTED_SECURITY_VULNERABILITY;
-
 }  // namespace
 
 // Stubs most of the checks since we are interested in validating the logic in
@@ -30,7 +26,7 @@ const BlacklistState kBlacklistState2 = BLACKLISTED_SECURITY_VULNERABILITY;
 class ExtensionInstallCheckerForTest : public ExtensionInstallChecker {
  public:
   ExtensionInstallCheckerForTest()
-      : ExtensionInstallChecker(NULL),
+      : ExtensionInstallChecker(nullptr, nullptr),
         requirements_check_called_(false),
         blacklist_check_called_(false),
         policy_check_called_(false),
@@ -50,21 +46,21 @@ class ExtensionInstallCheckerForTest : public ExtensionInstallChecker {
   bool blacklist_check_called() const { return blacklist_check_called_; }
   bool policy_check_called() const { return policy_check_called_; }
 
-  void MockCheckRequirements(int sequence_number) {
+  void MockCheckRequirements() {
     std::vector<std::string> errors;
     if (!requirements_error_.empty())
       errors.push_back(requirements_error_);
-    OnRequirementsCheckDone(sequence_number, errors);
+    OnRequirementsCheckDone(errors);
   }
 
-  void MockCheckBlacklistState(int sequence_number) {
-    OnBlacklistStateCheckDone(sequence_number, blacklist_state_);
+  void MockCheckBlacklistState() {
+    OnBlacklistStateCheckDone(blacklist_state_);
   }
 
  protected:
   void CheckRequirements() override {
     requirements_check_called_ = true;
-    MockCheckRequirements(current_sequence_number());
+    MockCheckRequirements();
   }
 
   void CheckManagementPolicy() override {
@@ -75,15 +71,7 @@ class ExtensionInstallCheckerForTest : public ExtensionInstallChecker {
 
   void CheckBlacklistState() override {
     blacklist_check_called_ = true;
-    MockCheckBlacklistState(current_sequence_number());
-  }
-
-  void ResetResults() override {
-    ExtensionInstallChecker::ResetResults();
-
-    requirements_check_called_ = false;
-    blacklist_check_called_ = false;
-    policy_check_called_ = false;
+    MockCheckBlacklistState();
   }
 
   bool requirements_check_called_;
@@ -106,7 +94,7 @@ class ExtensionInstallCheckerAsync : public ExtensionInstallCheckerForTest {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(&ExtensionInstallCheckerForTest::MockCheckRequirements,
-                   base::Unretained(this), current_sequence_number()));
+                   base::Unretained(this)));
   }
 
   void CheckBlacklistState() override {
@@ -115,7 +103,7 @@ class ExtensionInstallCheckerAsync : public ExtensionInstallCheckerForTest {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(&ExtensionInstallCheckerForTest::MockCheckBlacklistState,
-                   base::Unretained(this), current_sequence_number()));
+                   base::Unretained(this)));
   }
 };
 
@@ -148,40 +136,6 @@ class ExtensionInstallCheckerTest : public testing::Test {
   ExtensionInstallCheckerTest() {}
   ~ExtensionInstallCheckerTest() override {}
 
-  void RunSecondInvocation(ExtensionInstallCheckerForTest* checker,
-                           int checks_failed) {
-    EXPECT_GT(checks_failed, 0);
-    EXPECT_FALSE(checker->is_running());
-    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
-
-    // Set up different return values.
-    checker->set_blacklist_state(kBlacklistState2);
-    checker->set_policy_check_error(kDummyPolicyError2);
-    checker->set_requirements_error(kDummyRequirementsError2);
-
-    // Run the install checker again and ensure the second set of return values
-    // is received.
-    checker->Start(
-        ExtensionInstallChecker::CHECK_ALL,
-        false /* fail fast */,
-        base::Bind(&ExtensionInstallCheckerTest::ValidateSecondInvocation,
-                   base::Unretained(this),
-                   checker));
-  }
-
-  void ValidateSecondInvocation(ExtensionInstallCheckerForTest* checker,
-                                int checks_failed) {
-    EXPECT_FALSE(checker->is_running());
-    EXPECT_EQ(ExtensionInstallChecker::CHECK_REQUIREMENTS |
-                  ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY,
-              checks_failed);
-    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
-
-    EXPECT_EQ(kBlacklistState2, checker->blacklist_state());
-    ExpectPolicyError(kDummyPolicyError2, *checker);
-    ExpectRequirementsError(kDummyRequirementsError2, *checker);
-  }
-
  protected:
   void SetAllErrors(ExtensionInstallCheckerForTest* checker) {
     checker->set_blacklist_state(kBlacklistStateError);
@@ -284,35 +238,32 @@ class ExtensionInstallCheckerTest : public testing::Test {
     ExpectBlacklistError(*checker);
   }
 
-  void DoRunSubsetOfChecks(ExtensionInstallCheckerForTest* checker) {
-    // Test check set 1.
-    int tests_to_run = ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY |
-                       ExtensionInstallChecker::CHECK_REQUIREMENTS;
-    SetAllErrors(checker);
-    RunChecker(checker, false, tests_to_run, tests_to_run, tests_to_run);
-
-    ExpectRequirementsError(*checker);
-    ExpectPolicyError(*checker);
-    ExpectBlacklistPass(*checker);
-
-    // Test check set 2.
-    tests_to_run = ExtensionInstallChecker::CHECK_BLACKLIST |
-                   ExtensionInstallChecker::CHECK_REQUIREMENTS;
-    SetAllErrors(checker);
-    RunChecker(checker, false, tests_to_run, tests_to_run, tests_to_run);
-
-    ExpectRequirementsError(*checker);
-    ExpectPolicyPass(*checker);
-    ExpectBlacklistError(*checker);
-
-    // Test a single check.
-    tests_to_run = ExtensionInstallChecker::CHECK_BLACKLIST;
-    SetAllErrors(checker);
-    RunChecker(checker, false, tests_to_run, tests_to_run, tests_to_run);
-
-    ExpectRequirementsPass(*checker);
-    ExpectPolicyPass(*checker);
-    ExpectBlacklistError(*checker);
+  void DoRunSubsetOfChecks(int checks_to_run) {
+    ExtensionInstallCheckerForTest sync_checker;
+    ExtensionInstallCheckerAsync async_checker;
+    ExtensionInstallCheckerForTest* checkers[] = {
+        &sync_checker, &async_checker,
+    };
+
+    for (ExtensionInstallCheckerForTest* checker : checkers) {

[Devlin, 2017/03/16 19:29:41]

optional nit: just inline checkers

[michaelpg, 2017/03/17 04:25:23]

IMO the polymorphism is a bit more obvious being explicit like this

Also, seems like that would require a static_cast to EICFT* for async_checker
:-\

I made the range-declaration "auto*" tho.

+      SetAllErrors(checker);
+      RunChecker(checker, false, checks_to_run, checks_to_run, checks_to_run);
+
+      if (checks_to_run & ExtensionInstallChecker::CHECK_REQUIREMENTS)
+        ExpectRequirementsError(*checker);
+      else
+        ExpectRequirementsPass(*checker);
+
+      if (checks_to_run & ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY)
+        ExpectPolicyError(*checker);
+      else
+        ExpectPolicyPass(*checker);
+
+      if (checks_to_run & ExtensionInstallChecker::CHECK_BLACKLIST)
+        ExpectBlacklistError(*checker);
+      else
+        ExpectBlacklistPass(*checker);
+    }
   }
 
  private:
@@ -320,53 +271,6 @@ class ExtensionInstallCheckerTest : public testing::Test {
   base::MessageLoop message_loop;
 };
 
-class ExtensionInstallCheckerMultipleInvocationTest
-    : public ExtensionInstallCheckerTest {
- public:
-  ExtensionInstallCheckerMultipleInvocationTest() : callback_count_(0) {}
-  ~ExtensionInstallCheckerMultipleInvocationTest() override {}
-
-  void RunSecondInvocation(ExtensionInstallCheckerForTest* checker,
-                           int checks_failed) {
-    ASSERT_EQ(0, callback_count_);
-    ++callback_count_;
-    EXPECT_FALSE(checker->is_running());
-    EXPECT_GT(checks_failed, 0);
-    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
-
-    // Set up different return values.
-    checker->set_blacklist_state(kBlacklistState2);
-    checker->set_policy_check_error(kDummyPolicyError2);
-    checker->set_requirements_error(kDummyRequirementsError2);
-
-    // Run the install checker again and ensure the second set of return values
-    // is received.
-    checker->Start(ExtensionInstallChecker::CHECK_ALL,
-                   false /* fail fast */,
-                   base::Bind(&ExtensionInstallCheckerMultipleInvocationTest::
-                                  ValidateSecondInvocation,
-                              base::Unretained(this),
-                              checker));
-  }
-
-  void ValidateSecondInvocation(ExtensionInstallCheckerForTest* checker,
-                                int checks_failed) {
-    ASSERT_EQ(1, callback_count_);
-    EXPECT_FALSE(checker->is_running());
-    EXPECT_EQ(ExtensionInstallChecker::CHECK_REQUIREMENTS |
-                  ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY,
-              checks_failed);
-    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
-
-    EXPECT_EQ(kBlacklistState2, checker->blacklist_state());
-    ExpectPolicyError(kDummyPolicyError2, *checker);
-    ExpectRequirementsError(kDummyRequirementsError2, *checker);
-  }
-
- private:
-  int callback_count_;
-};
-
 // Test the case where all tests pass.
 TEST_F(ExtensionInstallCheckerTest, AllSucceeded) {
   ExtensionInstallCheckerForTest sync_checker;
@@ -387,41 +291,43 @@ TEST_F(ExtensionInstallCheckerTest, AllFailed) {
 
 // Test running only a subset of tests.
 TEST_F(ExtensionInstallCheckerTest, RunSubsetOfChecks) {
-  ExtensionInstallCheckerForTest sync_checker;
-  ExtensionInstallCheckerAsync async_checker;
-  DoRunSubsetOfChecks(&sync_checker);
-  DoRunSubsetOfChecks(&async_checker);
+  DoRunSubsetOfChecks(ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY |
+                      ExtensionInstallChecker::CHECK_REQUIREMENTS);
+  DoRunSubsetOfChecks(ExtensionInstallChecker::CHECK_BLACKLIST |
+                      ExtensionInstallChecker::CHECK_REQUIREMENTS);
+  DoRunSubsetOfChecks(ExtensionInstallChecker::CHECK_BLACKLIST);
 }
 
 // Test fail fast with synchronous callbacks.
 TEST_F(ExtensionInstallCheckerTest, FailFastSync) {
   // This test assumes some internal knowledge of the implementation - that
   // the policy check runs first.
-  ExtensionInstallCheckerForTest checker;
-  SetAllErrors(&checker);
-  RunChecker(&checker,
-             true /* fail fast */,
-             ExtensionInstallChecker::CHECK_ALL,
-             ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY,
-             ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY);
-
-  ExpectRequirementsPass(checker);
-  ExpectPolicyError(checker);
-  ExpectBlacklistPass(checker);
+  {
+    ExtensionInstallCheckerForTest checker;
+    SetAllErrors(&checker);
+    RunChecker(&checker, true /* fail fast */,
+               ExtensionInstallChecker::CHECK_ALL,
+               ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY,
+               ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY);
 
-  // This test assumes some internal knowledge of the implementation - that
-  // the requirements check runs before the blacklist check.
-  SetAllErrors(&checker);
-  RunChecker(&checker,
-             true /* fail fast */,
-             ExtensionInstallChecker::CHECK_REQUIREMENTS |
-                 ExtensionInstallChecker::CHECK_BLACKLIST,
-             ExtensionInstallChecker::CHECK_REQUIREMENTS,
-             ExtensionInstallChecker::CHECK_REQUIREMENTS);
+    ExpectRequirementsPass(checker);
+    ExpectPolicyError(checker);
+    ExpectBlacklistPass(checker);
+  }
 
-  ExpectRequirementsError(checker);
-  ExpectPolicyPass(checker);
-  ExpectBlacklistPass(checker);
+  {
+    ExtensionInstallCheckerForTest checker;
+    SetAllErrors(&checker);
+    RunChecker(&checker, true /* fail fast */,
+               ExtensionInstallChecker::CHECK_REQUIREMENTS |
+                   ExtensionInstallChecker::CHECK_BLACKLIST,
+               ExtensionInstallChecker::CHECK_REQUIREMENTS,
+               ExtensionInstallChecker::CHECK_REQUIREMENTS);
+
+    ExpectRequirementsError(checker);
+    ExpectPolicyPass(checker);
+    ExpectBlacklistPass(checker);
+  }
 }
 
 // Test fail fast with asynchronous callbacks.
@@ -448,41 +354,4 @@ TEST_F(ExtensionInstallCheckerTest, FailFastAsync) {
   ExpectBlacklistPass(checker);
 }
 
-// Test multiple invocations of the install checker. Wait for all checks to
-// complete.
-TEST_F(ExtensionInstallCheckerMultipleInvocationTest, CompleteAll) {
-  ExtensionInstallCheckerAsync checker;
-  SetAllErrors(&checker);
-
-  // Start the second check as soon as the callback of the first run is invoked.
-  checker.Start(
-      ExtensionInstallChecker::CHECK_ALL,
-      false /* fail fast */,
-      base::Bind(
-          &ExtensionInstallCheckerMultipleInvocationTest::RunSecondInvocation,
-          base::Unretained(this),
-          &checker));
-  base::RunLoop().RunUntilIdle();
-}
-
-// Test multiple invocations of the install checker and fail fast.
-TEST_F(ExtensionInstallCheckerMultipleInvocationTest, FailFast) {
-  ExtensionInstallCheckerAsync checker;
-  SetAllErrors(&checker);
-
-  // The policy check is synchronous and needs to pass for the other tests to
-  // run.
-  checker.set_policy_check_error(std::string());
-
-  // Start the second check as soon as the callback of the first run is invoked.
-  checker.Start(
-      ExtensionInstallChecker::CHECK_ALL,
-      true /* fail fast */,
-      base::Bind(
-          &ExtensionInstallCheckerMultipleInvocationTest::RunSecondInvocation,
-          base::Unretained(this),
-          &checker));
-  base::RunLoop().RunUntilIdle();
-}
-
 }  // namespace extensions