Tests for cloud policy UMA metrics.

Tests for cloud policy UMA metrics.

BUG=chromium-os:14002
TEST=unit_tests:EnterpriseMetricsTest,browser_tests:EnterpriseMetricsBrowserTest (chromeos only)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93383

[Joao da Silva, 2011-07-14 19:48:54]

@gfeher: please review, thanks! :-)

[Joao da Silva, 2011-07-14 19:49:37]

@gfeher: please review, thanks! :-)

[gfeher, 2011-07-15 12:21:34]

First round, mostly high level stuff.

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_unittest.cc (right):

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_unittest.cc:14: // function.
I am going to contradict one of my comments in our previous code reviews, sorry
about that. Couldn't you get rid of multi-processing the following way?
-Create a singleton class (or browser_process member, or
browser_policy_connector member, etc.) that collects metrics, more precisely,
passes calls to UMA_... macros?
-Replace this singleton/object with a mock when running in tests? I am not sure
if this can be done cleanly, though. But it would be cool to somehow simplify
these tests.
What do you think?

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_unittest.cc:120: class
MockDeviceManagementService : public DeviceManagementService {
Have you considered reusing the existing MockDeviceManagementService? I know
it's a bit different, but can't you maybe merge them shomehow?

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_unittest.cc:237: class MetricsTest {
Maybe call it MetricsTestHelper?

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_unittest.cc:313: std::vector<int>
samples_;
Call this expected_samples_?

[Joao da Silva, 2011-07-18 08:45:10]

@gfeher: please have another look, thanks!

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_unittest.cc (right):

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_unittest.cc:14: // function.
On 2011/07/15 12:21:34, gfeher wrote:
> I am going to contradict one of my comments in our previous code reviews,
sorry
> about that. Couldn't you get rid of multi-processing the following way?
> -Create a singleton class (or browser_process member, or
> browser_policy_connector member, etc.) that collects metrics, more precisely,
> passes calls to UMA_... macros?
> -Replace this singleton/object with a mock when running in tests? I am not
sure
> if this can be done cleanly, though. But it would be cool to somehow simplify
> these tests.
> What do you think?
As discussed offline, the out-of-process tests were rewritten as unit_tests
linked to browser_tests, so that they run on their own process.

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_unittest.cc:120: class
MockDeviceManagementService : public DeviceManagementService {
On 2011/07/15 12:21:34, gfeher wrote:
> Have you considered reusing the existing MockDeviceManagementService? I know
> it's a bit different, but can't you maybe merge them shomehow?
They are indeed different, wdyt of the merged version?

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_unittest.cc:237: class MetricsTest {
On 2011/07/15 12:21:34, gfeher wrote:
> Maybe call it MetricsTestHelper?
Obsolete.

http://codereview.chromium.org/7345010/diff/16001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_unittest.cc:313: std::vector<int>
samples_;
On 2011/07/15 12:21:34, gfeher wrote:
> Call this expected_samples_?

Done.

[gfeher, 2011-07-18 12:06:12]

I like the direction this is going. I think utilizing
MockDeviceManagementBackend would simplify your tests further, please see my
inline comment.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/mock_signed_settings_helper.h (right):

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/mock_signed_settings_helper.h:14: class
MockSignedSettingsHelper : public chromeos::SignedSettingsHelper {
I guess it would be nicer to just put this class into the chromeos namespace.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:58: class
TestInstallAttributes {
Please add 1-2 line comment explaining what does this do.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:105: void
SetMetricName(const std::string metric_name) {
Nit: pass by constant reference.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:129: void
ExpectSample(int sample) {
Please add a comment, like "Adds a sample to the set of expected samples."

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:190: std::string
metric_name_;
Please add a comment for this.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:222:
service.set_data(data);
I suggest you to use mock_device_management_backend.h instead of this stuff.
You'll likely need to define ACTION_P macros to specify responses, instead of
the predefined ACTION macros in the file.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:340:
TEST_F(EnterpriseMetricsTest, PolicyFetch) {
I would prefer this test case to be broken up to smaller pieces, e.g.
PolicyFetchFailedRequests, PolicyFetchInvalidData, ... It would be more easy
read it, and also, more easy to see what's wrong in case of a failure.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:514:
TEST_F(EnterpriseMetricsTest, DevicePolicyStorage) {
Could you also break this into smaller tests cases?

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:699: class
EnterpriseMetricsEnrollmentTest : public WizardInProcessBrowserTest {
Please put this into its own file.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:711:
URLFetcher::set_factory(&factory_);
Are you using this anywhere?

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:732:
logging::SetLogAssertHandler(assert_handler);
Which is the particular NOTREACHED you are trying to expect? What worries me, is
that what if you bypass a NOTREACHED that shouldn't be bypassed.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_cloud_policy_data_store.h (right):

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/mock_...
chrome/browser/policy/mock_cloud_policy_data_store.h:15: class
MockTokenAvailableObserver : public CloudPolicyDataStore::Observer {
I'd prefer to call this MockCloudPolicyDataStoreObserver, or something similar.
(I see that I should have renamed this earlier...)

[Joao da Silva, 2011-07-19 09:34:17]

@gfeher: thanks for all the comments, the CL is more sane now :-) Please have
another look.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/mock_signed_settings_helper.h (right):

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/mock_signed_settings_helper.h:14: class
MockSignedSettingsHelper : public chromeos::SignedSettingsHelper {
On 2011/07/18 12:06:12, gfeher wrote:
> I guess it would be nicer to just put this class into the chromeos namespace.

Done.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:58: class
TestInstallAttributes {
On 2011/07/18 12:06:12, gfeher wrote:
> Please add 1-2 line comment explaining what does this do.

Done.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:105: void
SetMetricName(const std::string metric_name) {
On 2011/07/18 12:06:12, gfeher wrote:
> Nit: pass by constant reference.

Done.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:129: void
ExpectSample(int sample) {
On 2011/07/18 12:06:12, gfeher wrote:
> Please add a comment, like "Adds a sample to the set of expected samples."

Done.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:190: std::string
metric_name_;
On 2011/07/18 12:06:12, gfeher wrote:
> Please add a comment for this.

Done.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:222:
service.set_data(data);
On 2011/07/18 12:06:12, gfeher wrote:
> I suggest you to use mock_device_management_backend.h instead of this stuff.
> You'll likely need to define ACTION_P macros to specify responses, instead of
> the predefined ACTION macros in the file.
As discussed, this test is actually exercising the real
DeviceManagementBackendImpl, and using a mock wouldn't record any samples.

The MockDeviceManagementService has been changed to look more like a real mock
though :-) Instead of the setters, it uses MOCK_ methods and custom ACTIONs.
wdyt?

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:340:
TEST_F(EnterpriseMetricsTest, PolicyFetch) {
On 2011/07/18 12:06:12, gfeher wrote:
> I would prefer this test case to be broken up to smaller pieces, e.g.
> PolicyFetchFailedRequests, PolicyFetchInvalidData, ... It would be more easy
> read it, and also, more easy to see what's wrong in case of a failure.

Done.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:514:
TEST_F(EnterpriseMetricsTest, DevicePolicyStorage) {
On 2011/07/18 12:06:12, gfeher wrote:
> Could you also break this into smaller tests cases?

Done.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:699: class
EnterpriseMetricsEnrollmentTest : public WizardInProcessBrowserTest {
On 2011/07/18 12:06:12, gfeher wrote:
> Please put this into its own file.

Done.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:711:
URLFetcher::set_factory(&factory_);
On 2011/07/18 12:06:12, gfeher wrote:
> Are you using this anywhere?
Yes, in the call to screen_->Authenticate(). It will create and start a
GaiaAuthFetcher, and this makes the Start() of the inner URLFetcher a nop.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:732:
logging::SetLogAssertHandler(assert_handler);
On 2011/07/18 12:06:12, gfeher wrote:
> Which is the particular NOTREACHED you are trying to expect? What worries me,
is
> that what if you bypass a NOTREACHED that shouldn't be bypassed.
Some code paths within EnterpriseEnrollmentScreen are not expected, and have
NOTREACHED there. For example, OnIssueAuthTokenSuccess() verifies if the token
was issued for the DM service, and it reaches a NOTREACHED if it was issue for
another service. This is something we don't expect to happen, and is recorded as
an "other" type of failure. There's a test checking for that behavior, and it
breaks when it reaches the NOTREACHED, as expected; therefore this method.

The NOTREACHED are correct IMO, but we would still like to know if these
unexpected cases are being triggered. I've extended the comment above to clarify
this.

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_cloud_policy_data_store.h (right):

http://codereview.chromium.org/7345010/diff/23001/chrome/browser/policy/mock_...
chrome/browser/policy/mock_cloud_policy_data_store.h:15: class
MockTokenAvailableObserver : public CloudPolicyDataStore::Observer {
On 2011/07/18 12:06:12, gfeher wrote:
> I'd prefer to call this MockCloudPolicyDataStoreObserver, or something
similar.
> (I see that I should have renamed this earlier...)

Done.

[gfeher, 2011-07-19 13:45:33]

Getting closer.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:56:
EXPECT_CALL(service_, CreateBackend())
Please try ON_CALL here.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_service.h (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_service.h:77: virtual void
StartJob(DeviceManagementJob* job);
Maybe you can make this private now?

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:39: EXPECT_CALL(service_,
CreateBackend())
I think this may be simplified using ON_CALL instead of EXPECT_CALL.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:24: #include
"chrome/test/ui_test_utils.h"
Is this used?

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:55: explicit
DeviceManagementBackendTestHelper(MessageLoop& loop)
Please add const modifier to loop (per style guide), or make it a pointer if
necessary.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:95: status_,
response_code_, cookies_, data_));
The 4-space indent should be counted from the beginning of the previous line,
the '.' in this case.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:103: MessageLoop& loop_;
Are you actually using this? (Note: I didn't find anything about it in the
style-guide, but I don't remember any examples of reference class members in the
Chromium code base.)

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:110: std::string data_;
I don't see the reason for storing these.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:143: // Run pending
tasks, and check that no unexpected samples were recorded.
Please move this comment before the method's head, because it seems like it
documents both lines at once.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:143: // Run pending
tasks, and check that no unexpected samples were recorded.
Aren't you also checking that all the expected samples were recorded?

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:152: MessageLoop& loop()
{
Please return pointer. (No style-guide for this, but it seems more
self-documenting and more in line with using pointers as variable-arguments.)

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:171: bool CheckSamples()
{
How about returning void, and always triggering an EXPECT_ failure in case of no
match?

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:193: &histogram);
Please deduplicate the above function call.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:624: Return(true)));
Are you sure you want these methods to be called exactly once during your test?
If that's a constraint you want to enforce, then okay. Otherwise I suggest
switching to ON_CALL.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc:63: // Other
unit tests should check the same code with the NOTREACHED enabled,
I really like this expanded explanation!
Just a nit: There should be other unit tests that check the same code

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc:96:
screen_->Authenticate(empty, empty, empty, empty);
Why not Authenticate("", "", "", "")?

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_device_management_service.h (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.h:80: // Use this to make
StartJob send a response to the job immediately.
Nit: send a faked response

[Joao da Silva, 2011-07-19 15:19:38]

@gfeher: thanks for your comments, please see inline.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:56:
EXPECT_CALL(service_, CreateBackend())
On 2011/07/19 13:45:33, gfeher wrote:
> Please try ON_CALL here.

Tried, but didn't work. ON_CALL unfortunately only allows setting the default
action, but not how many times it will be called. If the number of expected
calls isn't specified, a warning will be generated. I'd leave as is, wdyt?

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_service.h (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/devic...
chrome/browser/policy/device_management_service.h:77: virtual void
StartJob(DeviceManagementJob* job);
On 2011/07/19 13:45:33, gfeher wrote:
> Maybe you can make this private now?

Not really, since the Mock needs to override this method, and sometimes calls it
directly (StartJobNotMocked). That's why it's now protected and virtual.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:39: EXPECT_CALL(service_,
CreateBackend())
On 2011/07/19 13:45:33, gfeher wrote:
> I think this may be simplified using ON_CALL instead of EXPECT_CALL.

Same as above.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:24: #include
"chrome/test/ui_test_utils.h"
On 2011/07/19 13:45:33, gfeher wrote:
> Is this used?

Not really, removed.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:55: explicit
DeviceManagementBackendTestHelper(MessageLoop& loop)
On 2011/07/19 13:45:33, gfeher wrote:
> Please add const modifier to loop (per style guide), or make it a pointer if
> necessary.

Done.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:95: status_,
response_code_, cookies_, data_));
On 2011/07/19 13:45:33, gfeher wrote:
> The 4-space indent should be counted from the beginning of the previous line,
> the '.' in this case.

Done.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:103: MessageLoop& loop_;
On 2011/07/19 13:45:33, gfeher wrote:
> Are you actually using this? (Note: I didn't find anything about it in the
> style-guide, but I don't remember any examples of reference class members in
the
> Chromium code base.)

Nice catch, removed.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:110: std::string data_;
On 2011/07/19 13:45:33, gfeher wrote:
> I don't see the reason for storing these.

Indeed, removed.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:143: // Run pending
tasks, and check that no unexpected samples were recorded.
On 2011/07/19 13:45:33, gfeher wrote:
> Please move this comment before the method's head, because it seems like it
> documents both lines at once.

Done.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:143: // Run pending
tasks, and check that no unexpected samples were recorded.
On 2011/07/19 13:45:33, gfeher wrote:
> Aren't you also checking that all the expected samples were recorded?
Yes, but at the end of the test we just want to make sure that no other
unexpected samples were recorded. This is used to detect recording the same
event twice, for example (which would skew the metrics).

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:152: MessageLoop& loop()
{
On 2011/07/19 13:45:33, gfeher wrote:
> Please return pointer. (No style-guide for this, but it seems more
> self-documenting and more in line with using pointers as variable-arguments.)

Done.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:171: bool CheckSamples()
{
On 2011/07/19 13:45:33, gfeher wrote:
> How about returning void, and always triggering an EXPECT_ failure in case of
no
> match?

While running these tests I noticed that when they fail, it is nice to get the
line within the test that failed; that's why this returns a bool and is checked
in the test. Otherwise, we would see that an EXPECT_ failed in line 172, but we
wouldn't know which test caused it.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:193: &histogram);
On 2011/07/19 13:45:33, gfeher wrote:
> Please deduplicate the above function call.

Done.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:624: Return(true)));
On 2011/07/19 13:45:33, gfeher wrote:
> Are you sure you want these methods to be called exactly once during your
test?
> If that's a constraint you want to enforce, then okay. Otherwise I suggest
> switching to ON_CALL.
Yes, they are all called exactly once.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc:63: // Other
unit tests should check the same code with the NOTREACHED enabled,
On 2011/07/19 13:45:33, gfeher wrote:
> I really like this expanded explanation!
> Just a nit: There should be other unit tests that check the same code

Done.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc:96:
screen_->Authenticate(empty, empty, empty, empty);
On 2011/07/19 13:45:33, gfeher wrote:
> Why not Authenticate("", "", "", "")?

Done.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_device_management_service.h (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/mock_...
chrome/browser/policy/mock_device_management_service.h:80: // Use this to make
StartJob send a response to the job immediately.
On 2011/07/19 13:45:33, gfeher wrote:
> Nit: send a faked response

Done.

[gfeher, 2011-07-20 11:56:22]

It's very close. I only have some nits.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:56:
EXPECT_CALL(service_, CreateBackend())
Okay, I see. So we need an expectation to avoid "uninteresting call" warnings.
Then this is fine.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:171: bool CheckSamples()
{
Okay, I think it would worth documenting this fact in the head-comment. Not
necessarily your reasoning, just that this method has two ways of signaling the
same thing.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:624: Return(true)));
My question was more like, do you really want to enforce that these functions
must not be called more then once? If your answer is yes, then the current code
is fine.

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/26001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc:63: // Other
unit tests should check the same code with the NOTREACHED enabled,
I wanted to say:
"There should be other unit tests that check the same code with the NOTREACHED
enabled,"

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:176: &histogram);
What do you think about the following?
found = found && histogram != NULL;
if (found != expects_samples) ...
EXPECT_EQ(expects_samples, found);
This would print less error-messages, though, but maybe that's still enough.
Your call.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:194: size_t sum = 0;
Using signed ints is preferred, unless you really need unsigned. You could get
rid of some casts that way.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:196:
EXPECT_EQ(expected_samples_[i], samples.counts(i)) << "i is " << i;
Please either print a bit more verbose error message, or don't print anything.
E.g. sthg like "mismatched expectations at sample i"

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:208:
base::StatisticsRecorder statistics_recorder_;
I don't see you using this. Is it something that does something in its ctor?
Please add a short comment then.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:608:
.WillOnce(DoAll(SetArgumentPointee<1>(owned_),
Nit: SetArgumentPointee seems to be deprected, please use SetArgPointee.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:612:
.WillOnce(DoAll(SetArgumentPointee<1>(user_),
Same here.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:692:
data_store_->SetupForTesting("", "id", "user", "token", false);
Über-nit: "token" -> "gaia_token"
And at all the similar places.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc:69: void
ExpectSample(int sample) {
The seems more like "CheckSample". Please rename if you agree.

[Joao da Silva, 2011-07-20 15:23:22]

@gfeher: reviewed, please have another look. Thanks!

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:176: &histogram);
On 2011/07/20 11:56:22, gfeher wrote:
> What do you think about the following?
> found = found && histogram != NULL;
> if (found != expects_samples) ...
> EXPECT_EQ(expects_samples, found);
> This would print less error-messages, though, but maybe that's still enough.
> Your call.

Less code is better code! Done.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:194: size_t sum = 0;
On 2011/07/20 11:56:22, gfeher wrote:
> Using signed ints is preferred, unless you really need unsigned. You could get
> rid of some casts that way.

Done.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:196:
EXPECT_EQ(expected_samples_[i], samples.counts(i)) << "i is " << i;
On 2011/07/20 11:56:22, gfeher wrote:
> Please either print a bit more verbose error message, or don't print anything.
> E.g. sthg like "mismatched expectations at sample i"

Done.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:208:
base::StatisticsRecorder statistics_recorder_;
On 2011/07/20 11:56:22, gfeher wrote:
> I don't see you using this. Is it something that does something in its ctor?
> Please add a short comment then.

Yes, the ctor initializes the internal static members, which are required when
initializing all the UMA counters. A comment explaining this has been added.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:608:
.WillOnce(DoAll(SetArgumentPointee<1>(owned_),
On 2011/07/20 11:56:22, gfeher wrote:
> Nit: SetArgumentPointee seems to be deprected, please use SetArgPointee.

Didn't notice in the docs, good catch. Done.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:612:
.WillOnce(DoAll(SetArgumentPointee<1>(user_),
On 2011/07/20 11:56:22, gfeher wrote:
> Same here.

Done.

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_browsertest.cc:692:
data_store_->SetupForTesting("", "id", "user", "token", false);
On 2011/07/20 11:56:22, gfeher wrote:
> Über-nit: "token" -> "gaia_token"
> And at all the similar places.

Über-done!

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc (right):

http://codereview.chromium.org/7345010/diff/36001/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_metrics_enrollment_browsertest.cc:69: void
ExpectSample(int sample) {
On 2011/07/20 11:56:22, gfeher wrote:
> The seems more like "CheckSample". Please rename if you agree.

Done.

[gfeher, 2011-07-21 09:46:37]

LGTM!

http://codereview.chromium.org/7345010/diff/40001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/mock_signed_settings_helper.h (right):

http://codereview.chromium.org/7345010/diff/40001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/mock_signed_settings_helper.h:11: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
Maybe you can remove this because you are only using PolicyFetchResponse as a
reference?

[commit-bot: I haz the power, 2011-07-21 13:28:32]

Change committed as 93383