Extend TwoClientExtensionSettingsSyncTest to test app settings.

Extend TwoClientExtensionSettingsSyncTest to test app settings.

Doing so found a rather fundamental bug; the protobuf representation for apps and extensions settings are different, yet they are being treated the same. This patch fixes that, too.


BUG=114974
TEST=unit_tests, browser_tests, sync_integration_tests (all --gtest_filter=*ExtensionSetting*)

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

[not at google - send to devlin, 2012-02-22 21:10:46]

+rsimha

[akalin, 2012-02-22 22:47:07]

On 2012/02/22 21:10:46, kalman wrote:
> +rsimha

High-level comment: I think there could be a better way for these tests to be
shared between the two data types than what you have here (where you have stub
tests that call functions in the test fixture class).

Have you looked at
http://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_Tests
(also, type-parametrized tests)?  I'm not sure how they interact with our
homebrew browsertest macros, but if you could get that or something like that to
work, I think that would be cleaner.

[not at google - send to devlin, 2012-02-22 22:49:56]

Yep, I know about parameterised tests. They're pretty painful to set up --
probably worth it if you have a lot of interfaces to test, but not if there's
only 2. And it also doesn't work if tests are extending some other class (i.e.
SyncTest).

[Raghu Simha, 2012-02-23 08:22:52]

On 2012/02/22 22:47:07, akalin wrote:
> High-level comment: I think there could be a better way for these tests to be
> shared between the two data types than what you have here (where you have stub
> tests that call functions in the test fixture class).

I agree with what Fred says. This way of sharing test code between extensions
and apps restricts the use of the infrastructure to just this test class, and
doesn't allow for us to write more complex tests, for example,
ManyClientExtensionSettingsSyncTest.

Also, if I want to write a test for the interaction between modifying an app's
settings and changing a preference or adding a bookmark, I can't do it outside
this file.

Ideally, InstallExtensionForAllProfiles should live in extensions_helper.h,
InstallAppForAllProfiles in apps_helper.h, and MutateSomeSettings in
{app|extension}_settings_helper.h. If it makes sense, MutateSomeSettings could
take parameters so that it can be used by test classes in different ways as more
manual test cases are automated using this framework.

[not at google - send to devlin, 2012-02-23 11:20:54]

On 2012/02/23 08:22:52, rsimha wrote:
> On 2012/02/22 22:47:07, akalin wrote:
> > High-level comment: I think there could be a better way for these tests to
be
> > shared between the two data types than what you have here (where you have
stub
> > tests that call functions in the test fixture class).
> 
> I agree with what Fred says. This way of sharing test code between extensions
> and apps restricts the use of the infrastructure to just this test class, and
> doesn't allow for us to write more complex tests, for example,
> ManyClientExtensionSettingsSyncTest.

I think that two clients is all the tests we will need. Any more will just be
the same test, but will take longer to run.

> 
> Also, if I want to write a test for the interaction between modifying an app's
> settings and changing a preference or adding a bookmark, I can't do it outside
> this file.

I think we should just cross that bridge if it comes around.

> 
> Ideally, InstallExtensionForAllProfiles should live in extensions_helper.h,
> InstallAppForAllProfiles in apps_helper.h, and MutateSomeSettings in
> {app|extension}_settings_helper.h. If it makes sense, MutateSomeSettings could
> take parameters so that it can be used by test classes in different ways as
more
> manual test cases are automated using this framework.

I'll move Install{App,Extension}ForAllProfiles to their respective header files.
Even besides the future case where other tests might want to use them, it just
seems to be the expected place you'd see methods like that.

I'm not so sure about MutateSomeSettings, though. It's a highly specialised
procedure that only makes sense in the context of those particular tests. What
kind of scenarios do you think it could be reused? I think that in general
there's a cost in having util functions in separate files: it's more code,
people reading the code need to read across two files, people modifying the code
need to change across two files, people looking at the util function need to
find callers to understand why it exists, and when you "publicise" a util
function you need to be more careful about expecting inputs, etc. I think in
this case it isn't worth that effort.

[Raghu Simha, 2012-02-23 21:48:49]

On 2012/02/23 11:20:54, kalman wrote:
 
> I think that two clients is all the tests we will need. Any more will just be
> the same test, but will take longer to run.

I was referring to manual tests that might be automated in future. This has
happened for other datatypes in the past. But for now, I agree that two client
tests seem sufficient.

> I'll move Install{App,Extension}ForAllProfiles to their respective header
files.
> Even besides the future case where other tests might want to use them, it just
> seems to be the expected place you'd see methods like that.

SGTM.

> I'm not so sure about MutateSomeSettings, though. It's a highly specialised
> procedure that only makes sense in the context of those particular tests. What
> kind of scenarios do you think it could be reused?

Ok, I initially assumed it was a general purpose method for apps / extensions,
similar to say, bookmarks_helper::AddURL(), and so, there might be some benefit
from putting it in a util class. Based on what you say about it being highly
specialized, I agree that it can remain in this file.

And thanks a lot for adding these tests. I realise it might seem like a fair bit
of work from your end at first, but it's much appreciated, particularly when we
see this stuff run on the trybots and waterfall and catch / prevent real bugs.

[not at google - send to devlin, 2012-02-24 04:08:12]

Move done. Also rebased.

[akalin, 2012-02-24 20:28:01]

On 2012/02/24 04:08:12, kalman wrote:
> Move done. Also rebased.

Did you see my comment re. value-parametrized or type-parametrized tests? 
Having to use 'return AssertionFailure()' instead of the ASSERT_, etc. is kind
of ugly.  It may even be better to just duplicate the tests, like what we do
with apps/extensions (since if app settings and extension settings do diverge,
it'll be easier to split apart then.)

[not at google - send to devlin, 2012-02-25 01:58:21]

> Did you see my comment re. value-parametrized or type-parametrized tests? 

Yes, and I responded. As I said, those tests require extending a different
generic base class so aren't compatible with the SyncTest infrastructure. It's
also a bit of a pain setting them up; you need to add several more files, funky
indirection, and you impose upon readers to understand the magic that is
parameterized tests.  I think this is worth it if you need to test a lot of
different interfaces, but here it's only 2 and I don't think worth the effort.

> Having to use 'return AssertionFailure()' instead of the ASSERT_, etc. is kind
> of ugly.  It may even be better to just duplicate the tests, like what we do
> with apps/extensions (since if app settings and extension settings do diverge,
> it'll be easier to split apart then.)

I actually don't find the "return AssertionFailure()" stuff particularly ugly;
definitely uglier than just flat tests (with only EXPECT_EQ, as it was before),
but I'd really like to avoid copy-pasting code. It's harder to add more tests,
and harder for readers to immediately recognise that they're essentially the
same tests, and it's just more code that's easily avoidable.

If apps/extensions do diverge such that it's no longer possible to share those
tests, we should think about splitting it apart then. In fact, if we do need to
test some apps/extensions behaviour that's independent, we can hopefully just
add more tests that don't call common methods, leaving the common-tests clearly
marked by virtue of explicitly being shared.

In any case -- this code review seems to have stalled a bit; the time difference
is quite unfortunate :( but perhaps we can find time next week to chat about
this over gtalk, and resolve things without further round trips.

[akalin, 2012-02-25 02:02:53]

On 2012/02/25 01:58:21, kalman wrote:
> In any case -- this code review seems to have stalled a bit; the time
difference
> is quite unfortunate :( but perhaps we can find time next week to chat about
> this over gtalk, and resolve things without further round trips.

Yeah, sorry about that. :/ Okay, why don't you schedule something on Monday with
Raghu and I and we can try and hammer it out in a meeting/chat?

[akalin, 2012-02-25 02:03:23]

And I see your reply now.  I must have missed it, sorry.

[akalin, 2012-02-27 23:04:53]

http://codereview.chromium.org/9427001/diff/16001/chrome/browser/sync/test/in...
File
chrome/browser/sync/test/integration/two_client_extension_settings_sync_test.cc
(right):

http://codereview.chromium.org/9427001/diff/16001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_extension_settings_sync_test.cc:204:
ASSERT_PRED3(StartWithSameSettingsTest,
Same -> Different

[not at google - send to devlin, 2012-02-27 23:38:17]

As discussed, renaming the file to
two_client_extension_settings_and_app_settings_test.cc.

http://codereview.chromium.org/9427001/diff/16001/chrome/browser/sync/test/in...
File
chrome/browser/sync/test/integration/two_client_extension_settings_sync_test.cc
(right):

http://codereview.chromium.org/9427001/diff/16001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_extension_settings_sync_test.cc:204:
ASSERT_PRED3(StartWithSameSettingsTest,
On 2012/02/27 23:04:54, akalin wrote:
> Same -> Different

Ah.

[akalin, 2012-02-27 23:42:10]

couple more

http://codereview.chromium.org/9427001/diff/23001/chrome/browser/extensions/s...
File chrome/browser/extensions/settings/setting_sync_data.cc (right):

http://codereview.chromium.org/9427001/diff/23001/chrome/browser/extensions/s...
chrome/browser/extensions/settings/setting_sync_data.cc:30: // The data can only
be either extension or app specfics. Assert this in
can only be either -> must be either
specfics -> specifics
no need to say you're asserting it in debug mode; code is already pretty
obvious.

http://codereview.chromium.org/9427001/diff/23001/chrome/browser/extensions/s...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

http://codereview.chromium.org/9427001/diff/23001/chrome/browser/extensions/s...
chrome/browser/extensions/settings/settings_apitest.cc:28: syncable::ModelType
kModelType = syncable::EXTENSION_SETTINGS;
const

[not at google - send to devlin, 2012-02-27 23:45:46]

move done

http://codereview.chromium.org/9427001/diff/23001/chrome/browser/extensions/s...
File chrome/browser/extensions/settings/setting_sync_data.cc (right):

http://codereview.chromium.org/9427001/diff/23001/chrome/browser/extensions/s...
chrome/browser/extensions/settings/setting_sync_data.cc:30: // The data can only
be either extension or app specfics. Assert this in
On 2012/02/27 23:42:10, akalin wrote:
> can only be either -> must be either
> specfics -> specifics
> no need to say you're asserting it in debug mode; code is already pretty
> obvious.

Done.

http://codereview.chromium.org/9427001/diff/23001/chrome/browser/extensions/s...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

http://codereview.chromium.org/9427001/diff/23001/chrome/browser/extensions/s...
chrome/browser/extensions/settings/settings_apitest.cc:28: syncable::ModelType
kModelType = syncable::EXTENSION_SETTINGS;
On 2012/02/27 23:42:10, akalin wrote:
> const

Done.

[akalin, 2012-02-28 00:41:59]

LGTM

[not at google - send to devlin, 2012-02-28 00:43:08]

Thanks Fred.

Raghu, anything else you can think of?

[Raghu Simha, 2012-02-28 01:24:01]

LGTM!

Thanks for writing these tests, and sorry about the long review this ended up
being.

[commit-bot: I haz the power, 2012-02-28 01:34:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9427001/23002

[commit-bot: I haz the power, 2012-02-28 01:34:37]

Can't apply patch for file
chrome/browser/extensions/settings/settings_apitest.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/settings/settings_apitest.cc
Hunk #2 FAILED at 74.
Hunk #3 FAILED at 86.
Hunk #4 succeeded at 135 (offset -6 lines).
Hunk #5 succeeded at 287 (offset -6 lines).
Hunk #6 succeeded at 297 (offset -6 lines).
Hunk #7 succeeded at 335 (offset -6 lines).
Hunk #8 succeeded at 343 (offset -6 lines).
2 out of 8 hunks FAILED -- saving rejects to file
chrome/browser/extensions/settings/settings_apitest.cc.rej

[commit-bot: I haz the power, 2012-02-28 03:24:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9427001/25002

[commit-bot: I haz the power, 2012-02-28 07:22:42]

Change committed as 123929