Added retry policy

chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc

+// Copyright (c) 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 "base/command_line.h"
+#include "base/lazy_instance.h"

[xiyuan, 2017/02/01 23:01:51]

nit: unused ?

[kumarniranjan, 2017/02/03 22:53:20]

It was unused. I deleted it. Thanks for catching this!

+#include "base/logging.h"

[xiyuan, 2017/02/01 23:01:51]

nit: unused ?

[kumarniranjan, 2017/02/03 22:53:21]

Deleted

+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"

[xiyuan, 2017/02/01 23:01:51]

nit: unused ?

[kumarniranjan, 2017/02/03 22:53:20]

Deleted

+#include "base/test/scoped_mock_time_message_loop_task_runner.h"
+#include "chrome/browser/chromeos/login/enrollment/enrollment_screen.h"
+#include "chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h"
+#include "chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_mock.h"
+#include "chrome/browser/chromeos/login/enrollment/mock_enrollment_screen.h"
+#include "chrome/browser/chromeos/login/screens/mock_base_screen_delegate.h"
+#include "chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h"
+#include "chrome/browser/chromeos/policy/enrollment_config.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "components/pairing/fake_controller_pairing_controller.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::AnyNumber;
+using testing::Invoke;

[xiyuan, 2017/02/01 23:01:51]

Are these used?

[kumarniranjan, 2017/02/03 22:53:20]

Yes

+
+namespace chromeos {
+
+class EnrollmentScreenUnitTest : public testing::Test {
+ public:
+  EnrollmentScreenUnitTest() : fake_controller("") {
+    enrollment_config.mode = policy::EnrollmentConfig::MODE_ATTESTATION_FORCED;
+    enrollment_config.auth_mechanism =
+        policy::EnrollmentConfig::AUTH_MECHANISM_ATTESTATION;
+  }
+
+  static EnterpriseEnrollmentHelper* mock_enrollment_helper_creator(

[xiyuan, 2017/02/01 23:01:51]

Move into anonymous namespace for static method when possible.

Use CamelCase for function names.
i.e.
  mock_enrollment_helper_creator -> MockEnrollmentHelperCreator

[kumarniranjan, 2017/02/03 22:53:20]

Done.

+      EnterpriseEnrollmentHelper::EnrollmentStatusConsumer* status_consumer,
+      const policy::EnrollmentConfig& enrollment_config,
+      const std::string& enrolling_user_domain) {
+    return new EnterpriseEnrollmentHelperMock(status_consumer);
+  }
+
+  void SetUp() override {

[xiyuan, 2017/02/01 23:01:51]

nit: Add a comment for overridden interface/class methods.

e.g.

  // testing::Test:
  void SetUp() override {...}
  void TearDown() override {...}

[kumarniranjan, 2017/02/03 22:53:20]

Done.

+    DBusThreadManager::Initialize();
+    EnterpriseEnrollmentHelper::SetupEnrollmentHelperMock(
+        &EnrollmentScreenUnitTest::mock_enrollment_helper_creator);
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kEnterpriseEnableZeroTouchEnrollment, "hands-off");
+  }
+
+  void TearDown() override { DBusThreadManager::Shutdown(); }
+
+  static bool should_enroll;

[xiyuan, 2017/02/01 23:01:51]

Is this used anywhere?

[kumarniranjan, 2017/02/03 22:53:20]

No. Deleted now.

+
+  // Replace main thread's task runner with a mock for duration of test.
+  base::MessageLoop loop;
+  base::ScopedMockTimeMessageLoopTaskRunner runner;
+
+  // Objects required by EnrollmentScreen that can be re-used.
+  policy::EnrollmentConfig enrollment_config;
+  pairing_chromeos::FakeControllerPairingController fake_controller;
+  MockBaseScreenDelegate mock_delegate;
+  MockEnrollmentScreenActor mock_actor;

[xiyuan, 2017/02/01 23:01:52]

Name of a class member var should end with "_"

[kumarniranjan, 2017/02/03 22:53:20]

Done.

+};
+
+TEST_F(EnrollmentScreenUnitTest, Retries) {
+  TestingBrowserProcess::DeleteInstance();
+  TestingBrowserProcess::CreateInstance();

[xiyuan, 2017/02/01 23:01:51]

Why do we need to DeleteInstance then CreateInstance?

[kumarniranjan, 2017/02/03 22:53:20]

We want to replace the standard BrowserProcess with a TestingBrowserProcess.
Before the TestingBrowserProcess can be created, the existing BrowserProcess
must be deleted. There is a DCHECK which demands this:
https://cs.chromium.org/chromium/src/chrome/test/base/testing_browser_process...

+
+  // Create EnrollmentScreen.
+  std::unique_ptr<EnrollmentScreen> enrollment_screen(
+      new EnrollmentScreen(&mock_delegate, &mock_actor));
+  enrollment_screen->SetParameters(
+      enrollment_config,
+      static_cast<pairing_chromeos::ControllerPairingController*>(

[xiyuan, 2017/02/01 23:01:51]

Is static_cast necessary?

[kumarniranjan, 2017/02/03 22:53:21]

Actually, it is not needed.

+          &fake_controller));
+
+  // Define behavior of EnterpriseEnrollmentHelperMock to always fail
+  // enrollment.
+  EnterpriseEnrollmentHelperMock::should_enroll = false;

[xiyuan, 2017/02/01 23:01:51]

Can we avoid using static (or any global state) for this purpose? Is it possible
to make the creator function expose the EnterpriseEnrollmentHelperMock object it
creates to allow setting per-instance state? Or defer SetupEnrollmentHelperMock
call here to set a creator that constructs a mock with desired behavior?

[kumarniranjan, 2017/02/03 22:53:20]

Yes. Done.

+
+  // Start zero-touch enrollment.
+  enrollment_screen->Show();
+
+  // Fast forward time by 1 minute.
+  runner.task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(1));
+
+  // Check that we have retried 4 to 6 times.
+  // It is not an exact number due to the 10% jitter in each retry delay.
+  // Since these delays multiply exponentially, the jitter leads to
+  // significant differences in time taken after a few retries.
+  // For instance, it takes 26.5-39.5 sec for 4 retries,
+  // 39.8-69.1 sec for 5 retries, and 57.8-118.0 sec for 6 retries.
+  // Therefore in 60 sec, we could observe 4, 5, or 6 retries.
+  EXPECT_GE(enrollment_screen->num_retries_, 4);
+  EXPECT_LE(enrollment_screen->num_retries_, 6);
+
+  // Required for proper destruction of EnrollmentScreen.
+  TestingBrowserProcess::GetGlobal()->SetShuttingDown(true);
+}
+
+TEST_F(EnrollmentScreenUnitTest, DoesNotRetryOnTopOfUser) {
+  TestingBrowserProcess::DeleteInstance();
+  TestingBrowserProcess::CreateInstance();
+
+  // Create EnrollmentScreen.
+  std::unique_ptr<EnrollmentScreen> enrollment_screen(
+      new EnrollmentScreen(&mock_delegate, &mock_actor));
+  enrollment_screen->SetParameters(
+      enrollment_config,
+      static_cast<pairing_chromeos::ControllerPairingController*>(
+          &fake_controller));

[xiyuan, 2017/02/01 23:01:52]

Line 104-113 repeats for every test case. Can they be moved into SetUp?

[kumarniranjan, 2017/02/03 22:53:20]

Yes. Done. I also need a separate function SetUpEnrollmentScreen since there is
one line in the middle (after certain revisions) that is different in each test.

+
+  // Define behavior of EnterpriseEnrollmentHelperMock to always fail
+  // enrollment.
+  EnterpriseEnrollmentHelperMock::should_enroll = false;
+
+  // Start zero-touch enrollment.
+  enrollment_screen->Show();
+
+  // Fast forward time by 45 seconds.
+  runner.task_runner()->FastForwardBy(base::TimeDelta::FromSeconds(45));
+
+  // Simulate the user clicking the retry button.
+  enrollment_screen->OnRetry();
+
+  // Fast forward time by another 15 seconds.
+  runner.task_runner()->FastForwardBy(base::TimeDelta::FromSeconds(15));
+
+  // Check that the number of retries is still 4-6.
+  EXPECT_GE(enrollment_screen->num_retries_, 4);
+  EXPECT_LE(enrollment_screen->num_retries_, 6);
+
+  // Required for proper destruction of EnrollmentScreen.
+  TestingBrowserProcess::GetGlobal()->SetShuttingDown(true);
+}
+
+TEST_F(EnrollmentScreenUnitTest, DoesNotRetryAfterSuccess) {
+  TestingBrowserProcess::DeleteInstance();
+  TestingBrowserProcess::CreateInstance();
+
+  // Create EnrollmentScreen.
+  std::unique_ptr<EnrollmentScreen> enrollment_screen(
+      new EnrollmentScreen(&mock_delegate, &mock_actor));
+  enrollment_screen->SetParameters(
+      enrollment_config,
+      static_cast<pairing_chromeos::ControllerPairingController*>(
+          &fake_controller));
+
+  // Define behavior of EnterpriseEnrollmentHelperMock to successfully enroll.
+  EnterpriseEnrollmentHelperMock::should_enroll = true;
+
+  // Start zero-touch enrollment.
+  enrollment_screen->Show();
+
+  // Fast forward time by 1 minute.
+  runner.task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(1));
+
+  // Check that we do not retry.
+  EXPECT_TRUE(enrollment_screen->num_retries_ == 0);
+
+  // Required for proper destruction of EnrollmentScreen.
+  TestingBrowserProcess::GetGlobal()->SetShuttingDown(true);
+}
+}  // namespace chromeos