Re-enabled UserCloudPolicyManagerChromeOS unit tests

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc

Index: chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
diff --git a/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc b/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
index 2263d6dfa40a27ef4e2e63e2ca7362566576ccb9..293b6d470910c67739762f6a96a7669e46ad6da1 100644
--- a/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
+++ b/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
@@ -19,6 +19,7 @@
 #include "chrome/browser/chromeos/login/users/scoped_user_manager_enabler.h"
 #include "chrome/browser/chromeos/policy/user_cloud_policy_token_forwarder.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/policy/cloud/cloud_policy_test_utils.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
@@ -113,24 +114,11 @@ class UserCloudPolicyManagerChromeOSTest : public testing::Test {
     chrome::RegisterLocalState(prefs_.registry());
 
     // Set up a policy map for testing.
+    GetExpectedDefaultPolicy(&policy_map_);
     policy_map_.Set(key::kHomepageLocation, POLICY_LEVEL_MANDATORY,
                     POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
                     base::MakeUnique<base::StringValue>("http://chromium.org"),
                     nullptr);
-    policy_map_.Set(
-        key::kChromeOsMultiProfileUserBehavior, POLICY_LEVEL_MANDATORY,
-        POLICY_SCOPE_USER, POLICY_SOURCE_ENTERPRISE_DEFAULT,
-        base::MakeUnique<base::StringValue>("primary-only"), nullptr);
-    policy_map_.Set(key::kEasyUnlockAllowed, POLICY_LEVEL_MANDATORY,
-                    POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
-                    base::MakeUnique<base::FundamentalValue>(false), nullptr);
-    policy_map_.Set(key::kCaptivePortalAuthenticationIgnoresProxy,
-                    POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
-                    POLICY_SOURCE_CLOUD,
-                    base::MakeUnique<base::FundamentalValue>(false), nullptr);
-    policy_map_.Set(key::kAllowDinosaurEasterEgg, POLICY_LEVEL_MANDATORY,
-                    POLICY_SCOPE_USER, POLICY_SOURCE_ENTERPRISE_DEFAULT,
-                    base::MakeUnique<base::FundamentalValue>(false), nullptr);
     expected_bundle_.Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()))
         .CopyFrom(policy_map_);
 
@@ -146,6 +134,7 @@ class UserCloudPolicyManagerChromeOSTest : public testing::Test {
     policy_data_.set_policy_type(dm_protocol::kChromeUserPolicyType);
     policy_data_.set_request_token("dmtoken123");
     policy_data_.set_device_id("id987");
+    policy_data_.set_username("user@example.com");
     em::PolicyFetchResponse* policy_response =
         policy_blob_.mutable_policy_response()->add_response();
     ASSERT_TRUE(policy_data_.SerializeToString(
@@ -375,8 +364,7 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFirstFetch) {
                          DM_STATUS_SUCCESS, register_blob_));
 }
 
-// Test disabled. See crbug.com/534733.
-TEST_F(UserCloudPolicyManagerChromeOSTest, DISABLED_BlockingRefreshFetch) {
+TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingRefreshFetch) {
   // Tests the initialization of a manager whose Profile is waiting for the
   // initial fetch, when a previously cached policy and DMToken already exist.
   ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
@@ -507,10 +495,8 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchPolicyFetchError) {
   EXPECT_TRUE(PolicyBundle().Equals(manager_->policies()));
 }
 
-// Test disabled. See crbug.com/534733.
-TEST_F(UserCloudPolicyManagerChromeOSTest, DISABLED_BlockingFetchTimeout) {
-  // The blocking fetch should be abandoned after the timeout.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 0));
+TEST_F(UserCloudPolicyManagerChromeOSTest, BlockingFetchTimeout) {
+  ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));

[emaxx, 2016/11/29 16:21:15]

It's not clear to me why the timeouts have been changed in several tests to
opposite values (1000 <=> 0). (Just asking to double check that these changes
were not actually temporary - for debug purposes.)

[Andrew T Wilson (Slow), 2016/11/30 10:02:34]

The tests bitrotted while they were disabled - I added code in
https://codereview.chromium.org/1735863002 to ensure that if you say
"wait_for_fetch=true" you must pass in a non-zero timeout (doesn't make sense to
pass a zero timeout when doing a blocking policy fetch, except in tests).

So I had to change this to have a non-zero timeout, and then force a timeout
below - it's a cleaner solution than relying on a zero timeout and running the
RunLoop anyway.

 
   // Initialize the CloudPolicyService without any stored data.
   EXPECT_FALSE(manager_->core()->service()->IsInitializationComplete());
@@ -518,19 +504,18 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, DISABLED_BlockingFetchTimeout) {
   EXPECT_TRUE(manager_->core()->service()->IsInitializationComplete());
   EXPECT_FALSE(manager_->core()->client()->is_registered());
 
-  // Running the message loop should trigger the timeout.
+  // Triggering the timeout should invoke our callback.
   EXPECT_CALL(observer_, OnUpdatePolicy(manager_.get())).Times(AtLeast(1));
   EXPECT_FALSE(manager_->IsInitializationComplete(POLICY_DOMAIN_CHROME));
-  base::RunLoop().RunUntilIdle();
+  manager_->ForceTimeoutForTest();
   Mock::VerifyAndClearExpectations(&observer_);
   EXPECT_TRUE(manager_->IsInitializationComplete(POLICY_DOMAIN_CHROME));
   EXPECT_TRUE(PolicyBundle().Equals(manager_->policies()));
 }
 
-// Test disabled. See crbug.com/534733.
-TEST_F(UserCloudPolicyManagerChromeOSTest, DISABLED_NonBlockingFirstFetch) {
+TEST_F(UserCloudPolicyManagerChromeOSTest, NonBlockingFirstFetch) {
   // Tests the first policy fetch request by a Profile that isn't managed.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(false, 1000));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(false, 0));
 
   // Initialize the CloudPolicyService without any stored data. Since the
   // manager is not waiting for the initial fetch, it will become initialized
@@ -573,17 +558,17 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, DISABLED_NonBlockingFirstFetch) {
       base::Bind(&base::TestSimpleTaskRunner::RunUntilIdle, task_runner_));
 }
 
-// Test disabled. See crbug.com/534733.
-TEST_F(UserCloudPolicyManagerChromeOSTest, DISABLED_NonBlockingRefreshFetch) {
+TEST_F(UserCloudPolicyManagerChromeOSTest, NonBlockingRefreshFetch) {
   // Tests a non-blocking initial policy fetch for a Profile that already has
   // a cached DMToken.
-  ASSERT_NO_FATAL_FAILURE(CreateManager(false, 1000));
+  ASSERT_NO_FATAL_FAILURE(CreateManager(false, 0));
 
   // Set the initially cached data and initialize the CloudPolicyService.
   // The initial policy fetch is issued using the cached DMToken.
   EXPECT_FALSE(manager_->core()->service()->IsInitializationComplete());
   EXPECT_FALSE(manager_->IsInitializationComplete(POLICY_DOMAIN_CHROME));
   EXPECT_CALL(observer_, OnUpdatePolicy(manager_.get()));

[pmarko, 2016/11/29 14:03:34]

I don't really understand why is this being done here - isn't it just testing
that the SetUp method has set a username? Specifically, why is it being asserted
in NonBlockingRefreshFetch?

[Andrew T Wilson (Slow), 2016/11/30 10:02:34]

I think I added this as a temporary check while debugging a problem - agreed
that it doesn't make sense so I've removed it.

+  ASSERT_TRUE(policy_data_.has_username());
   store_->policy_.reset(new em::PolicyData(policy_data_));
   store_->NotifyStoreLoaded();
   Mock::VerifyAndClearExpectations(&observer_);