Tests: Make LoggedInUserMixin more user-friendly

LoggedInUserMixin is not as easy to use as it could be. This CL
maximizes the chances that LoggedInUserMixin just works out of the
box, while minimizing the chances of mysterious time outs. The test
developer can optimize for efficiency later by tweaking the options in
LoggedInUserMixin. This CL improves the browser test login tools.

Bug: None
Change-Id: Ic5e597e68ba32e7f29daa4d5331d47298ef3fbcf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2598044
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840919}

chrome/browser/chromeos/child_accounts/parent_access_code/parent_access_service_browsertest.cc

@@ -18,7 +18,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/supervised_user/logged_in_user_mixin.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
-#include "chromeos/constants/chromeos_switches.h"
 #include "components/account_id/account_id.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/test/browser_test.h"
@@ -89,18 +88,11 @@ class ParentAccessServiceTest : public MixinBasedInProcessBrowserTest {
             logged_in_user_mixin_.GetAccountId())) {}
   ~ParentAccessServiceTest() override = default;
 
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    MixinBasedInProcessBrowserTest::SetUpCommandLine(command_line);
-    command_line->AppendSwitch(switches::kOobeSkipPostLogin);
-  }
-
   void SetUpOnMainThread() override {
     ASSERT_NO_FATAL_FAILURE(GetTestAccessCodeValues(&test_values_));
     ParentAccessService::Get().AddObserver(test_observer_.get());
     MixinBasedInProcessBrowserTest::SetUpOnMainThread();
-    logged_in_user_mixin_.LogInUser(false /*issue_any_scope_token*/,
-                                    true /*wait_for_active_session*/,
-                                    true /*request_policy_update*/);
+    logged_in_user_mixin_.LogInUser();
   }
 
   void TearDownOnMainThread() override {

chrome/browser/chromeos/child_accounts/screen_time_controller_browsertest.cc

@@ -24,7 +24,6 @@
 #include "chrome/browser/supervised_user/logged_in_user_mixin.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
-#include "chromeos/constants/chromeos_switches.h"
 #include "components/prefs/pref_service.h"
 #include "components/session_manager/core/session_manager.h"
 #include "content/public/browser/notification_service.h"
@@ -65,12 +64,6 @@ class ScreenTimeControllerTest : public MixinBasedInProcessBrowserTest {
 
   ~ScreenTimeControllerTest() override = default;
 
-  // MixinBasedInProcessBrowserTest:
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    MixinBasedInProcessBrowserTest::SetUpCommandLine(command_line);
-    command_line->AppendSwitch(switches::kOobeSkipPostLogin);
-  }
-
   // MixinBasedInProcessBrowserTest:
   void SetUpInProcessBrowserTestFixture() override {
     MixinBasedInProcessBrowserTest::SetUpInProcessBrowserTestFixture();
@@ -87,9 +80,7 @@ class ScreenTimeControllerTest : public MixinBasedInProcessBrowserTest {
  protected:
   void LogInChildAndSetupClockWithTime(const char* time) {
     SetupTaskRunnerWithTime(utils::TimeFromString(time));
-    logged_in_user_mixin_.LogInUser(false /*issue_any_scope_token*/,
-                                    true /*wait_for_active_session*/,
-                                    true /*request_policy_update*/);
+    logged_in_user_mixin_.LogInUser();
     MockClockForActiveUser();
   }
 

chrome/browser/chromeos/child_accounts/time_limits/app_time_browsertest.cc

@@ -37,7 +37,6 @@
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/test/browser_test.h"
-#include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -106,10 +105,7 @@ class AppTimeTest : public MixinBasedInProcessBrowserTest {
   void SetUpOnMainThread() override {
     MixinBasedInProcessBrowserTest::SetUpOnMainThread();
     ASSERT_TRUE(embedded_test_server()->Started());
-    host_resolver()->AddIPLiteralRule("*", "127.0.0.1", "localhost");
-    logged_in_user_mixin_.LogInUser(false /*issue_any_scope_token*/,
-                                    true /*wait_for_active_session*/,
-                                    true /*request_policy_update*/);
+    logged_in_user_mixin_.LogInUser();
 
     arc::SetArcPlayStoreEnabledForProfile(browser()->profile(), true);
     arc_app_list_prefs_ = ArcAppListPrefs::Get(browser()->profile());

chrome/browser/chromeos/child_accounts/time_limits/web_time_calculation_browsertest.cc

@@ -40,7 +40,6 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
-#include "net/dns/mock_host_resolver.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/window_open_disposition.h"
 #include "url/gurl.h"
@@ -101,12 +100,7 @@ void WebTimeCalculationBrowserTest::SetUpOnMainThread() {
 
   ASSERT_TRUE(embedded_test_server()->Started());
 
-  // Resolve everything to localhost.
-  host_resolver()->AddIPLiteralRule("*", "127.0.0.1", "localhost");
-
-  logged_in_user_mixin_.LogInUser(false /*issue_any_scope_token*/,
-                                  true /*wait_for_active_session*/,
-                                  true /*request_policy_update*/);
+  logged_in_user_mixin_.LogInUser();
   profile_ = browser()->profile();
 
   // During tests, AppService doesn't notify AppActivityRegistry that chrome app

chrome/browser/chromeos/child_accounts/time_limits/web_time_limit_enforcer_browsertest.cc

@@ -38,7 +38,6 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
-#include "net/dns/mock_host_resolver.h"
 #include "ui/base/window_open_disposition.h"
 #include "url/gurl.h"
 
@@ -130,12 +129,7 @@ void WebTimeLimitEnforcerThrottleTest::SetUpOnMainThread() {
 
   ASSERT_TRUE(embedded_test_server()->Started());
 
-  // Resolve everything to localhost.
-  host_resolver()->AddIPLiteralRule("*", "127.0.0.1", "localhost");
-
-  logged_in_user_mixin_.LogInUser(false /*issue_any_scope_token*/,
-                                  true /*wait_for_active_session*/,
-                                  true /*request_policy_update*/);
+  logged_in_user_mixin_.LogInUser();
 }
 
 bool WebTimeLimitEnforcerThrottleTest::IsErrorPageBeingShownInWebContents(

chrome/browser/chromeos/extensions/login_screen/login/login_apitest.cc

@@ -401,11 +401,7 @@ class LoginApitestWithEnterpriseUser : public LoginApitest {
 
   ~LoginApitestWithEnterpriseUser() override = default;
 
-  void LoginUser() {
-    logged_in_user_mixin_.LogInUser(/*issue_any_scope_token=*/false,
-                                    /*wait_for_active_session=*/true,
-                                    /*request_policy_update=*/true);
-  }
+  void LoginUser() { logged_in_user_mixin_.LogInUser(); }
 
   void SetUpInSessionExtension() override {
     AccountId account_id = logged_in_user_mixin_.GetAccountId();

chrome/browser/chromeos/login/login_ui_browsertest.cc

@@ -397,9 +397,7 @@ class UserManagementDisclosureChildTest
 // having logged a child account into a session and having locked the screen.
 IN_PROC_BROWSER_TEST_F(UserManagementDisclosureChildTest,
                        PRE_EnterpriseIconVisibleChildUser) {
-  logged_in_user_mixin_.LogInUser(false /*issue_any_scope_token*/,
-                                  true /*wait_for_active_session*/,
-                                  true /*request_policy_update*/);
+  logged_in_user_mixin_.LogInUser();
   ScreenLockerTester screen_locker_tester;
   screen_locker_tester.Lock();
   EXPECT_FALSE(ash::LoginScreenTestApi::IsManagedIconShown(

chrome/browser/chromeos/login/screens/supervision_transition_screen_browsertest.cc

@@ -79,8 +79,7 @@ class SupervisionTransitionScreenTest
     // WaitForActiveSession() otherwise.
     logged_in_user_mixin_.LogInUser(
         false /*issue_any_scope_token*/,
-        content::IsPreTest() /*wait_for_active_session*/,
-        true /*request_policy_update*/);
+        content::IsPreTest() /*wait_for_active_session*/);
   }
 
   // The tests simulate user type changes between regular and child user.

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc

@@ -20,7 +20,6 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
-#include "chromeos/constants/chromeos_switches.h"
 #include "components/arc/arc_features.h"
 #include "components/arc/arc_prefs.h"
 #include "components/policy/core/browser/browser_policy_connector.h"
@@ -59,16 +58,12 @@ class UserCloudPolicyManagerTest
     MixinBasedInProcessBrowserTest::TearDown();
   }
 
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitch(chromeos::switches::kOobeSkipPostLogin);
-    MixinBasedInProcessBrowserTest::SetUpCommandLine(command_line);
-  }
-
   // Sets up fake GAIA for specified user login, and requests login for the user
   // (using LoggedInUserMixin).
   void StartUserLogIn(bool wait_for_active_session) {
     logged_in_user_mixin_.LogInUser(true /*issue_any_scope_token*/,
-                                    wait_for_active_session);
+                                    wait_for_active_session,
+                                    /*request_policy_update=*/false);
   }
 
  protected:

chrome/browser/supervised_user/logged_in_user_mixin.cc

@@ -6,6 +6,7 @@
 
 #include <vector>
 
+#include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "chromeos/login/auth/stub_authenticator_builder.h"
 #include "chromeos/login/auth/user_context.h"
@@ -49,7 +50,9 @@ LoggedInUserMixin::LoggedInUserMixin(
                 AccountId::FromUserEmailGaiaId(FakeGaiaMixin::kFakeUserEmail,
                                                FakeGaiaMixin::kFakeUserGaiaId)),
             ConvertUserType(type)),
-      login_manager_(mixin_host, GetInitialUsers(user_, include_initial_user)),
+      login_manager_(mixin_host,
+                     GetInitialUsers(user_, include_initial_user),
+                     &fake_gaia_),
       local_policy_server_(mixin_host),
       user_policy_(mixin_host,
                    user_.account_id,
@@ -72,6 +75,8 @@ void LoggedInUserMixin::SetUpOnMainThread() {
   // account.google.com requests would never reach fake GAIA server without
   // this.
   test_base_->host_resolver()->AddRule("*", "127.0.0.1");
+  // Ensures logging in doesn't hang on the post login Gaia screens.
+  WizardController::SkipPostLoginScreensForTesting();
 }
 
 void LoggedInUserMixin::LogInUser(bool issue_any_scope_token,
@@ -89,17 +94,14 @@ void LoggedInUserMixin::LogInUser(bool issue_any_scope_token,
                                      FakeGaiaMixin::kFakeRefreshToken);
   }
   if (request_policy_update) {
-    // Set up policy, which prevents the call to LoginAndWaitForActiveSession()
-    // below from hanging indefinitely in some test scenarios.
+    // Child users require user policy, set up an empty one so the user can get
+    // through login.
     GetUserPolicyMixin()->RequestPolicyUpdate();
   }
   if (wait_for_active_session) {
     login_manager_.LoginAndWaitForActiveSession(user_context);
-    // Set the private |browser_| member in InProcessBrowserTest.
-    // Otherwise calls to InProcessBrowserTest::browser() returns null and leads
-    // to segmentation faults.
-    // Note: |browser_| is only non-null if should_launch_browser was set to
-    // true in the constructor.
+    // If should_launch_browser was set to true, then ensures
+    // InProcessBrowserTest::browser() doesn't return nullptr.
     test_base_->SelectFirstBrowser();
   } else {
     login_manager_.AttemptLoginUsingAuthenticator(

chrome/browser/supervised_user/logged_in_user_mixin.h

@@ -5,7 +5,6 @@
 #ifndef CHROME_BROWSER_SUPERVISED_USER_LOGGED_IN_USER_MIXIN_H_
 #define CHROME_BROWSER_SUPERVISED_USER_LOGGED_IN_USER_MIXIN_H_
 
-#include "base/macros.h"
 #include "base/optional.h"
 #include "chrome/browser/chromeos/login/test/embedded_test_server_mixin.h"
 #include "chrome/browser/chromeos/login/test/fake_gaia_mixin.h"
@@ -60,15 +59,13 @@ class LoggedInUserMixin : public InProcessBrowserTestMixin {
   // that can be passed into this constructor.
   // |test_base|: just pass in a pointer to the browser test class.
   // |should_launch_browser| determines whether a browser instance is launched
-  // after successful login. Call SelectFirstBrowser() afterwards to ensure
-  // calls to browser() don't return nullptr. LogInUser() already calls
-  // SelectFirstBrowser() for convenience.
+  // after successful login.
   // |account_id| is the desired test account id for logging in. The default
   // test account already works for the majority of test cases, unless an
   // enterprise account is needed for setting up policy.
-  // |include_initial_user| determines whether the TestUserInfo should be passed
-  // to the initial users list of the LoginManagerMixin. Excluding the initial
-  // user causes the OOBE GAIA screen to show on start-up.
+  // |include_initial_user| if true, then the user already exists on the login
+  // screen. Otherwise, the user is newly added to the device and the OOBE Gaia
+  // screen will show on start-up.
   // |use_local_policy_server| determines if the LocalPolicyTestServerMixin
   // should be passed into the UserPolicyMixin.
   LoggedInUserMixin(InProcessBrowserTestMixinHost* mixin_host,
@@ -78,7 +75,10 @@ class LoggedInUserMixin : public InProcessBrowserTestMixin {
                     bool should_launch_browser = true,
                     base::Optional<AccountId> account_id = base::nullopt,
                     bool include_initial_user = true,
+                    // TODO(crbug/1112885): Remove this parameter.
                     bool use_local_policy_server = true);
+  LoggedInUserMixin(const LoggedInUserMixin&) = delete;
+  LoggedInUserMixin& operator=(const LoggedInUserMixin&) = delete;
   ~LoggedInUserMixin() override;
 
   // InProcessBrowserTestMixin:
@@ -93,7 +93,7 @@ class LoggedInUserMixin : public InProcessBrowserTestMixin {
   // * If |request_policy_update|, UserPolicyMixin will set up user policy.
   void LogInUser(bool issue_any_scope_token = false,
                  bool wait_for_active_session = true,
-                 bool request_policy_update = false);
+                 bool request_policy_update = true);
 
   LoginManagerMixin* GetLoginManagerMixin() { return &login_manager_; }
 
@@ -123,8 +123,6 @@ class LoggedInUserMixin : public InProcessBrowserTestMixin {
   FakeGaiaMixin fake_gaia_;
 
   InProcessBrowserTest* test_base_;
-
-  DISALLOW_COPY_AND_ASSIGN(LoggedInUserMixin);
 };
 
 }  // namespace chromeos

chrome/browser/supervised_user/supervised_user_navigation_throttle_browsertest.cc

@@ -37,7 +37,6 @@
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
-#include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
 using content::NavigationController;
@@ -198,8 +197,6 @@ void SupervisedUserNavigationThrottleTest::SetUp() {
 
 void SupervisedUserNavigationThrottleTest::SetUpOnMainThread() {
   MixinBasedInProcessBrowserTest::SetUpOnMainThread();
-  // Resolve everything to localhost.
-  host_resolver()->AddIPLiteralRule("*", "127.0.0.1", "localhost");
 
   ASSERT_TRUE(embedded_test_server()->Started());