Re-enabled UserCloudPolicyManagerChromeOS unit tests

Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122
Committed: https://crrev.com/786755daa476c2374428a8950377f58f4d709349
Cr-Commit-Position: refs/heads/master@{#435257}

[Andrew T Wilson (Slow), 2016-11-28 17:24:09]

The CQ bit was checked by atwilson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 17:24:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Andrew T Wilson (Slow), 2016-11-28 18:32:48]

Description was changed from

==========
Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122
==========

to

==========
Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122
==========

[Andrew T Wilson (Slow), 2016-11-28 18:32:48]

Description was changed from

==========
Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122
==========

to

==========
Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122
==========

[Andrew T Wilson (Slow), 2016-11-28 18:32:48]

atwilson@chromium.org changed reviewers:
+ emaxx@chromium.org, pmarko@chromium.org

[Andrew T Wilson (Slow), 2016-11-28 18:32:49]

atwilson@chromium.org changed reviewers:
+ emaxx@chromium.org, pmarko@chromium.org

[commit-bot: I haz the power, 2016-11-28 19:31:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 19:31:02]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:123:
policy_fetch_timeout_.Stop();
nit: I think stopping the timer would not be strictly necessary because
OnBlockingFetchTimeout has to stop the timer anyway.

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:570:
EXPECT_CALL(observer_, OnUpdatePolicy(manager_.get()));
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?

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud/cloud_policy_test_utils.cc (right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/cloud_policy_test_utils.cc:17:
SetEnterpriseUsersDefaults(policy_map);
Two small questions:
- Is this not defeating the purpose of testing the defaults, or was re-stating
them in the test just found to be redundant?
- There is a small behavior change to the old version: This puts the default
policy values into policy_map only if the keys are missing. I'd suggest we
either document that this method is not guaranteed to replace existing keys or
actually clear policy_map at the beginning of the method (and document *that*).
Current callers seem to be passing empty PolicyMap instances only, so it should
not be a real problem at the moment.

[emaxx, 2016-11-29 16:21:16]

I'm fine with this change, assuming that the
GeneratePolicySource.SetEnterpriseDefaults unit test is
enough for testing that the SetEnterpriseUsersDefaults
function is doing something sensible.

LGTM

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:124:
this->OnBlockingFetchTimeout();
nit: Is "this->" really needed here?

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:499:
ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
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.)

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud/cloud_policy_test_utils.cc (right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/cloud_policy_test_utils.cc:13:
policy_map->Set(key::kNTPContentSuggestionsEnabled, POLICY_LEVEL_MANDATORY,
nit: include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/cloud_policy_test_utils.cc:15: base::WrapUnique(new
base::FundamentalValue(false)), nullptr);
nit: include "base/values.h"

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud/cloud_policy_test_utils.h (right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/cloud_policy_test_utils.h:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
nit: Please remove "(c)" according to the style guide:
"There is no (c) after Copyright."
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

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

Addressed review comments and also added a test to explicitly test that
enterprise-default policies are getting injected properly.

Pavol: PTAL

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:123:
policy_fetch_timeout_.Stop();
On 2016/11/29 14:03:34, pmarko wrote:
> nit: I think stopping the timer would not be strictly necessary because
> OnBlockingFetchTimeout has to stop the timer anyway.

Agreed, but I'm trying to mimic the behavior of a real timer firing (so the
timer would have been stopped by the time the timeout callback was invoked).
I've added a comment.

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc:124:
this->OnBlockingFetchTimeout();
On 2016/11/29 16:21:15, emaxx wrote:
> nit: Is "this->" really needed here?

No, you are just seeing the artifacts of a rusty manager trying to write code.

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:499:
ASSERT_NO_FATAL_FAILURE(CreateManager(true, 1000));
On 2016/11/29 16:21:15, emaxx wrote:
> 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.)

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.

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:570:
EXPECT_CALL(observer_, OnUpdatePolicy(manager_.get()));
On 2016/11/29 14:03:34, pmarko wrote:
> 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?

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.

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud/cloud_policy_test_utils.cc (right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/cloud_policy_test_utils.cc:13:
policy_map->Set(key::kNTPContentSuggestionsEnabled, POLICY_LEVEL_MANDATORY,
On 2016/11/29 16:21:15, emaxx wrote:
> nit: include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/cloud_policy_test_utils.cc:15: base::WrapUnique(new
base::FundamentalValue(false)), nullptr);
On 2016/11/29 16:21:15, emaxx wrote:
> nit: include "base/values.h"

Done.

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/cloud_policy_test_utils.cc:17:
SetEnterpriseUsersDefaults(policy_map);
On 2016/11/29 14:03:34, pmarko wrote:
> Two small questions:
> - Is this not defeating the purpose of testing the defaults, or was re-stating
> them in the test just found to be redundant?

This was turning out to be a maintenance problem, because when someone wanted to
add the |default_for_enterprise_users| attribute to a policy, they had to update
multiple files (policy_templates.json, cloud_policy_browsertests.cc,
user_cloud_policy_manager_chromeos_unittest.cc). I just want people to be able
to add the attribute to policy_templates.json and be done with it.

That said, there was some value in the old approach as it ensured that the code
that injected the defaults continued to work. I've added a new test to
selectively check that one well-known default policy is getting injected - that
should be enough test coverage for that code path without increasing the
maintenance burden.

> - There is a small behavior change to the old version: This puts the default
> policy values into policy_map only if the keys are missing. I'd suggest we
> either document that this method is not guaranteed to replace existing keys or
> actually clear policy_map at the beginning of the method (and document
*that*).
> Current callers seem to be passing empty PolicyMap instances only, so it
should
> not be a real problem at the moment.

I'm going to keep the current behavior in the CL, and I've documented it.

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud/cloud_policy_test_utils.h (right):

https://codereview.chromium.org/2534983002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/cloud_policy_test_utils.h:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
On 2016/11/29 16:21:15, emaxx wrote:
> nit: Please remove "(c)" according to the style guide:
> "There is no (c) after Copyright."
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

[pmarko, 2016-11-30 10:56:26]

Looks good!

[Andrew T Wilson (Slow), 2016-11-30 13:25:44]

On 2016/11/30 10:56:26, pmarko wrote:
> Looks good!

You actually have to put "LGTM" somewhere in your reply as otherwise the
codereview tool doesn't recognize that you've LGTM'd it.

[Andrew T Wilson (Slow), 2016-11-30 13:25:50]

The CQ bit was checked by atwilson@chromium.org

[Andrew T Wilson (Slow), 2016-11-30 13:25:50]

The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2534983002/#ps20001
(title: "review feedback")

[commit-bot: I haz the power, 2016-11-30 13:26:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pmarko, 2016-11-30 13:41:00]

On 2016/11/30 13:25:44, Andrew T Wilson (Slow) wrote:
> On 2016/11/30 10:56:26, pmarko wrote:
> > Looks good!
> 
> You actually have to put "LGTM" somewhere in your reply as otherwise the
> codereview tool doesn't recognize that you've LGTM'd it.

Good to know, thanks. LGTM.

[commit-bot: I haz the power, 2016-11-30 14:36:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480512350303790,
"parent_rev": "2dc97110849625ac80084adbc5dfb163ffcf87d4", "commit_rev":
"e951fd9585998e62f0da53d53a55a3ab822b7c73"}

[commit-bot: I haz the power, 2016-11-30 14:37:05]

Description was changed from

==========
Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122
==========

to

==========
Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122
==========

[commit-bot: I haz the power, 2016-11-30 14:37:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-30 14:40:28]

Description was changed from

==========
Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122
==========

to

==========
Re-enabled UserCloudPolicyManagerChromeOS unit tests

Changed test code to no longer maintain a parallel list of default
policies, but instead to use the auto-generated list for easier
maintenance.

BUG=666122

Committed: https://crrev.com/786755daa476c2374428a8950377f58f4d709349
Cr-Commit-Position: refs/heads/master@{#435257}
==========

[commit-bot: I haz the power, 2016-11-30 14:40:29]

Patchset 2 (id:??) landed as
https://crrev.com/786755daa476c2374428a8950377f58f4d709349
Cr-Commit-Position: refs/heads/master@{#435257}