Add DeviceSettingProvider unit tests.

Add DeviceSettingProvider unit tests.

BUG=none
TEST=unit_tests:DeviceSettingsProviderTest.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=128874

[pastarmovj, 2012-03-16 13:55:52]

Please review this CL which adds unit tests to DeviceSettingsProvider.

[Mattias Nissler (ping if slow), 2012-03-16 15:21:10]

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.h (right):

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.h:47: OwnershipService::Status
ownership_service);
it seems like the name |ownership_service| is slightly misleading, as you're
only passing the status. Seems like we could as well remove the status since you
can always call set_ownership_status(). And then, why not just go back to a
single constructor that also takes the signed settings helper? There's only one
call site in production code anyways, so putting the SignedSettingsHelper::Get()
incantation there shouldn't hurt.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider_unittest.cc (right):

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:14: #include
"chrome/browser/chromeos/device_settings_provider.h"
alphabetize

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:20: #include
"chrome/browser/chromeos/login/signed_settings_cache.h"
needed?

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:22: #include
"chrome/browser/policy/browser_policy_connector.h"
needed?

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:23: #include
"chrome/browser/policy/cloud_policy_data_store.h"
needed?

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:44:
notifier_called_ = true;
You're never reading |notifier_called_|...

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:47: void
GetTrustedCallback() {
Both SettingChanged and GetTrustedCallback (which are named inconsistently btw.)
could be MOCK_METHODs. If you do so, you don't have to keep flags, but you can
instead install expectations on the mocked methods.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:49: }
newline

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:128:
scoped_ptr<MockUserManager> mock_user_manager_;
Does this need the scoped_ptr wrapping?

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:133:
MetricsService* metrics_service_;
Seems there is no reason to keep this pointer in a member.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:141:
ASSERT_TRUE(provider_->GetTrusted(
EXPECT_TRUE seems to be good enough

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:147:
ASSERT_FALSE(trusted_called_);
same here.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:152:
ASSERT_FALSE(bool_value);
and here
and below

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:156: // No calls to
the SignedSettingsHelper shoud occur in this case!
s/shoud/should/
also below

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:180: // Verify the
change has not been apply.
s/apply/applied/

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:193: // Verify the
change has not been apply.
s/apply/applied/
also below.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:222: // Verify that
the cache policy blob is "trusted".
Don't you initialize with OWNERSHIP_TAKEN? Shouldn't the trusted checks fail
then?

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:232:
TEST_F(DeviceSettingsProviderTest, PolicyRetrievalFailed2) {
Can you make the naming of the test reflect the difference between the two?

[jar (doing other things), 2012-03-16 16:29:16]

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.h:144: friend class
chromeos::DeviceSettingsProviderTest;
I'm always leery about adding friendship to classes. It makes the class harder
to read, as there is always a fear that distant code will manipulate innards in
unknown ways.  When possible, it is always nicer to have a limited interface
that server the needs of this would-be new-friend. 

It is helpful that the class has the word "Test" in it, which generally assures
me that the tinkering won't happen in production, but it also means that if a
person modifies the MetricsService class, they must be aware of any/all accesses
made by a second class through undocumented interfaces :-(.

If you can clarify the limited set of interfaces and mutations you'd need for
the other class, that would be IMO much cleaner (more readable, maintainable,
etc.)

[pastarmovj, 2012-03-21 15:23:26]

PTAL at this much better contained version now. I managed to get  my tests
running without the need for the metrics service to be mocked.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.h (right):

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.h:47: OwnershipService::Status
ownership_service);
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> it seems like the name |ownership_service| is slightly misleading, as you're
> only passing the status. Seems like we could as well remove the status since
you
> can always call set_ownership_status(). And then, why not just go back to a
> single constructor that also takes the signed settings helper? There's only
one
> call site in production code anyways, so putting the
SignedSettingsHelper::Get()
> incantation there shouldn't hurt.

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider_unittest.cc (right):

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:14: #include
"chrome/browser/chromeos/device_settings_provider.h"
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> alphabetize

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:20: #include
"chrome/browser/chromeos/login/signed_settings_cache.h"
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> needed?

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:22: #include
"chrome/browser/policy/browser_policy_connector.h"
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> needed?

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:23: #include
"chrome/browser/policy/cloud_policy_data_store.h"
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> needed?

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:44:
notifier_called_ = true;
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> You're never reading |notifier_called_|...

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:47: void
GetTrustedCallback() {
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> Both SettingChanged and GetTrustedCallback (which are named inconsistently
btw.)
> could be MOCK_METHODs. If you do so, you don't have to keep flags, but you can
> instead install expectations on the mocked methods.

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:49: }
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:128:
scoped_ptr<MockUserManager> mock_user_manager_;
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> Does this need the scoped_ptr wrapping?

Yes after my UserManager CL that Nikita is reviewing lands it's responsibility
of the unit test to destroy the mock manager.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:133:
MetricsService* metrics_service_;
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> Seems there is no reason to keep this pointer in a member.

True fixed.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:141:
ASSERT_TRUE(provider_->GetTrusted(
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> EXPECT_TRUE seems to be good enough

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:147:
ASSERT_FALSE(trusted_called_);
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> same here.

Line is gone.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:152:
ASSERT_FALSE(bool_value);
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> and here
> and below

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:156: // No calls to
the SignedSettingsHelper shoud occur in this case!
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> s/shoud/should/
> also below

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:180: // Verify the
change has not been apply.
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> s/apply/applied/

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:193: // Verify the
change has not been apply.
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> s/apply/applied/
> also below.

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:222: // Verify that
the cache policy blob is "trusted".
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> Don't you initialize with OWNERSHIP_TAKEN? Shouldn't the trusted checks fail
> then?

True and this is what the code does but the comment doesn't. :)

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider_unittest.cc:232:
TEST_F(DeviceSettingsProviderTest, PolicyRetrievalFailed2) {
On 2012/03/16 15:21:11, Mattias Nissler wrote:
> Can you make the naming of the test reflect the difference between the two?

Done.

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/9703115/diff/2001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.h:144: friend class
chromeos::DeviceSettingsProviderTest;
On 2012/03/16 16:29:16, jar wrote:
> I'm always leery about adding friendship to classes. It makes the class harder
> to read, as there is always a fear that distant code will manipulate innards
in
> unknown ways.  When possible, it is always nicer to have a limited interface
> that server the needs of this would-be new-friend. 
> 
> It is helpful that the class has the word "Test" in it, which generally
assures
> me that the tinkering won't happen in production, but it also means that if a
> person modifies the MetricsService class, they must be aware of any/all
accesses
> made by a second class through undocumented interfaces :-(.
> 
> If you can clarify the limited set of interfaces and mutations you'd need for
> the other class, that would be IMO much cleaner (more readable, maintainable,
> etc.)

I decided to split this CL and get all MetricsService related changes in another
one so this is reverted. 
I was anyhow planning to do some refactoring there like adding a virtual base
class to server as a base for mocking the MetricsService in tests. Also the
utility functions that are now used to turn on and off the metrics reporting
seem to be a little chaotic and I remember there were users directly accessing
the MetricsService while other user the OptionsUtil class. So I will consolidate
those as well if possible in this other CL. Might take a while till I get to
this task though.

[Mattias Nissler (ping if slow), 2012-03-22 10:59:33]

LGTM with a nit.

https://chromiumcodereview.appspot.com/9703115/diff/6001/chrome/browser/chrom...
File chrome/browser/chromeos/device_settings_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9703115/diff/6001/chrome/browser/chrom...
chrome/browser/chromeos/device_settings_provider_unittest.cc:79:
pointer_factory_.GetWeakPtr()),
You could actually ditch the WeakPtrFactory and use base::Unretained here and
below, as the provider is guaranteed to go down before the test class instance
gets destroyed.

[pastarmovj, 2012-03-23 13:39:39]

Adding sky to review the change to chrome/browser/ui/options/options_util.cc. 

I have a plan to extend the MetricsService class soon to properly support
mocking for tests then we can replace this DCHECK back in there if required.

[sky, 2012-03-23 16:04:33]

LGTM