Allow sync integration tests to operate on multiple datatypes: Preferences + Bookmarks

Allow sync integration tests to operate on multiple datatypes: Preferences + Bookmarks

The sync integration tests currently use a class hierarchy where the test classes for each datatype are subclasses of LiveSyncTest. While this design worked in the past, it allows tests to work with only one datatype at a time, and therefore doesn't allow us to test the interplay between datatypes.

This patch is the first in a series of patches that will move away from an inheritance model to one where test cases can operate on more than one datatype. It updates the Preferences and Bookmarks datatypes to the new model, and contains the following changes:

- LivePreferencesSyncTest no longer inherits from LiveSyncTest, but is renamed to PreferencesHelper, and contains a bunch of static methods that perform various operations related to preferences. Similarly, LiveBookmarksSyncTest becomes BookmarksHelper.
- SingleClientLivePreferencesSyncTest is renamed to SingleClientPreferencesSyncTest, and is a subclass of LiveSyncTest. It uses the methods in PreferencesHelper by including its header file and attaching itself to the helper. Similarly, SingleClientBookmarksSyncTest uses the methods in BookmarksHelper.
- The old helper class BookmarkModelVerifier is now refactored into BookmarksHelper. This eliminates a bunch of wrapper methods that were in LiveBookmarksSyncTest.
- Once more datatype helper classes are refactored out, tests can attach to more than one datatype helper and use all their methods. For example, the class MigrationErrorsTest benefits from this model by being able to use PreferencesHelper and BookmarksHelper.

BUG=88510
TEST=sync_integration_tests, sync_performance_tests

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

[Raghu Simha, 2011-07-06 07:20:57]

Tim/Nick, let me know what you think of this approach. Thanks.

[ncarter (slow), 2011-07-06 17:48:52]

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_test.h (right):

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_test.h:115: void DisableVerifier();
Best to avoid per-datatype methods in LiveSyncTest -- if allowed to mushroom,
they'll muddy the capabilities of this class.  Here's a counterproposal: an
EnableVerifier(ModelType) and DisableVerifier(ModelType) pair.  Any unsupported
modeltypes can then become test failures.  (if you really wanted, you could use
varargs to make the call site cleaner for multiple datatypes)

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/prefer...
File chrome/test/live_sync/preferences_helper.h (right):

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/prefer...
chrome/test/live_sync/preferences_helper.h:20: ~PreferencesHelper();
I'm doing pros/cons of whether this should be a non-static class with |int
index| captured by the ctor.  I think it could lead to slightly more readable
code.  E.g. the following:

// If |index| is static.
PreferencesHelper alice(0);
PreferencesHelper bob(1);
alice.changeListPref("pref1", "a");
bob.changeListPref("pref2", "b");

// If |index| is dynamic.
for (int i = 0; i < num_clients(); ++i) {
  PreferencesHelper(i).changeListPref(i, "pref1", "value");
}

[C++ is increasingly foreign to me, and the above suggestion may have a tinge of
Java in it.  Evaluate its merits.]

[Raghu Simha, 2011-07-07 02:14:03]

Nick: I have a couple of responses (and no code changes) :) Let me know what you
think.

Tim: Ping. (It's the start of a new work day in BLR now, so it'll help the code
review round-trip-time if you can get to this tonight)

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/live_s...
File chrome/test/live_sync/live_sync_test.h (right):

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/live_s...
chrome/test/live_sync/live_sync_test.h:115: void DisableVerifier();
On 2011/07/06 17:48:52, ncarter wrote:
> Best to avoid per-datatype methods in LiveSyncTest -- if allowed to mushroom,
> they'll muddy the capabilities of this class.  Here's a counterproposal: an
> EnableVerifier(ModelType) and DisableVerifier(ModelType) pair.  Any
unsupported
> modeltypes can then become test failures.  (if you really wanted, you could
use
> varargs to make the call site cleaner for multiple datatypes)

It may not be readily evident yet, but this should ideally be a sync-wide
method. Over time, I had added copies of it to a bunch of different datatypes,
but now, I'm moving it into the base class so that each datatype can simply use
the mechanism that's in here. Now, if a test wants to make (racy) changes where
the end state is non-deterministic, it simply calls
LiveSyncTest::DisableVerifier(). In upcoming patches, I'll be removing the
copies in other datatypes.

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/prefer...
File chrome/test/live_sync/preferences_helper.h (right):

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/prefer...
chrome/test/live_sync/preferences_helper.h:20: ~PreferencesHelper();
On 2011/07/06 17:48:52, ncarter wrote:
> I'm doing pros/cons of whether this should be a non-static class with |int
> index| captured by the ctor.  I think it could lead to slightly more readable
> code.  E.g. the following:
> 
> // If |index| is static.
> PreferencesHelper alice(0);
> PreferencesHelper bob(1);
> alice.changeListPref("pref1", "a");
> bob.changeListPref("pref2", "b");
> 
> // If |index| is dynamic.
> for (int i = 0; i < num_clients(); ++i) {
>   PreferencesHelper(i).changeListPref(i, "pref1", "value");
> }
> 
> [C++ is increasingly foreign to me, and the above suggestion may have a tinge
of
> Java in it.  Evaluate its merits.]

Maybe I'm missing something, but I don't see the benefit of using a separate
Helper object per profile. Test cases already use the profile index in numerous
other places, so this is a logical extension of that design.

The goal I'm working towards is that test cases can use more than one
DatatypeHelper class (for example, MigrationErrorTest can use PreferencesHelper
and BookmarksHelper).

One thing your comment made me realise is that in the model I'm currently using,
tests should never try to create instances of PreferencesHelper, and so its
constructor should probably be private.

[ncarter (slow), 2011-07-07 02:30:33]

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/prefer...
File chrome/test/live_sync/preferences_helper.h (right):

http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/prefer...
chrome/test/live_sync/preferences_helper.h:20: ~PreferencesHelper();
On 2011/07/07 02:14:04, rsimha wrote:
> On 2011/07/06 17:48:52, ncarter wrote:
> > I'm doing pros/cons of whether this should be a non-static class with |int
> > index| captured by the ctor.  I think it could lead to slightly more
readable
> > code.  E.g. the following:
> > 
> > // If |index| is static.
> > PreferencesHelper alice(0);
> > PreferencesHelper bob(1);
> > alice.changeListPref("pref1", "a");
> > bob.changeListPref("pref2", "b");
> > 
> > // If |index| is dynamic.
> > for (int i = 0; i < num_clients(); ++i) {
> >   PreferencesHelper(i).changeListPref(i, "pref1", "value");
> > }
> > 
> > [C++ is increasingly foreign to me, and the above suggestion may have a
tinge
> of
> > Java in it.  Evaluate its merits.]
> 
> Maybe I'm missing something, but I don't see the benefit of using a separate
> Helper object per profile. Test cases already use the profile index in
numerous
> other places, so this is a logical extension of that design.
> 
> The goal I'm working towards is that test cases can use more than one
> DatatypeHelper class (for example, MigrationErrorTest can use
PreferencesHelper
> and BookmarksHelper).
> 
> One thing your comment made me realise is that in the model I'm currently
using,
> tests should never try to create instances of PreferencesHelper, and so its
> constructor should probably be private.

The only difference is in readability; it allows you to drop the
"PreferencesHelper::" everywhere.  More personally, I find it easier to follow
code where boilerplate parameters that have to be passed to a method with every
invocation are kept to a minimum.  In particular, I've found it particularly
hard to keep things correct when doing cut/paste/edit of code that uses
hardcoded |int index|.

Having said that, you're the expert here, not me.  You have a valid point about
the precedent, but it also seems that this changelist is about setting a new
precedent, so I think it's worthwhile making readability improvements to the API
if possible.

[tim (not reviewing), 2011-07-07 05:28:43]

On 2011/07/07 02:30:33, ncarter wrote:
>
http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/prefer...
> File chrome/test/live_sync/preferences_helper.h (right):
> 
>
http://codereview.chromium.org/7259005/diff/2037/chrome/test/live_sync/prefer...
> chrome/test/live_sync/preferences_helper.h:20: ~PreferencesHelper();
> On 2011/07/07 02:14:04, rsimha wrote:
> > On 2011/07/06 17:48:52, ncarter wrote:
> > > I'm doing pros/cons of whether this should be a non-static class with |int
> > > index| captured by the ctor.  I think it could lead to slightly more
> readable
> > > code.  E.g. the following:
> > > 
> > > // If |index| is static.
> > > PreferencesHelper alice(0);
> > > PreferencesHelper bob(1);
> > > alice.changeListPref("pref1", "a");
> > > bob.changeListPref("pref2", "b");
> > > 
> > > // If |index| is dynamic.
> > > for (int i = 0; i < num_clients(); ++i) {
> > >   PreferencesHelper(i).changeListPref(i, "pref1", "value");
> > > }
> > > 
> > > [C++ is increasingly foreign to me, and the above suggestion may have a
> tinge
> > of
> > > Java in it.  Evaluate its merits.]
> > 
> > Maybe I'm missing something, but I don't see the benefit of using a separate
> > Helper object per profile. Test cases already use the profile index in
> numerous
> > other places, so this is a logical extension of that design.
> > 
> > The goal I'm working towards is that test cases can use more than one
> > DatatypeHelper class (for example, MigrationErrorTest can use
> PreferencesHelper
> > and BookmarksHelper).
> > 
> > One thing your comment made me realise is that in the model I'm currently
> using,
> > tests should never try to create instances of PreferencesHelper, and so its
> > constructor should probably be private.
> 
> The only difference is in readability; it allows you to drop the
> "PreferencesHelper::" everywhere.  More personally, I find it easier to follow
> code where boilerplate parameters that have to be passed to a method with
every
> invocation are kept to a minimum.  In particular, I've found it particularly
> hard to keep things correct when doing cut/paste/edit of code that uses
> hardcoded |int index|.
> 
> Having said that, you're the expert here, not me.  You have a valid point
about
> the precedent, but it also seems that this changelist is about setting a new
> precedent, so I think it's worthwhile making readability improvements to the
API
> if possible.

On the whole these changes look good to me!  Though I'm on the edge of my seat
anticipating the outcome of static vs instantiating PreferencesHelper :) 
Eliminating all those extra characters does seem nice... but I dont feel strong
enough to enforce either myself.

[Raghu Simha, 2011-07-12 13:01:17]

Tim/Nick: Please take another look.

Some notes:
- This patch now adds 2 static classes - PreferencesHelper and BookmarksHelper.
- I played around with the suggestion of instantiating a PreferencesHelper /
BookmarksHelper object and invoking it as Nick suggested (alice.ChangeListPref
instead of PreferencesHelper::ChangeListPref). From my attempts, using static
classes turned out to be cleaner. (Both approaches required prefixing the method
name -- something I haven't been able to avoid)
- There remains a major TODO in this patch: Truncating the lines to 80 chars or
less. I'll take care of it tomorrow. Meanwhile, I'm sending this out as it is so
that I can get feedback from you guys overnight.
- The fruits of this patch can be seen in MigrationErrorsTest, which now uses
helper methods for 2 datatypes. See
http://codereview.chromium.org/7259005/diff/14001/chrome/test/live_sync/migra....

Let me know what you guys think.

[Raghu Simha, 2011-07-14 10:03:06]

Ping.

This is now ready for a full review, with all TODOs taken care of. The sync
trybots seem to be happy with all the Bookmarks and Preferences tests that now
use their new helper classes.

[Raghu Simha, 2011-07-22 05:48:00]

Tim/Nick: Bumping this thread once again.

I should be able to address comments / land this on Monday.

[Raghu Simha, 2011-07-25 08:12:32]

+zea as reviewer, since Tim is OOO.

Nicolas, could you take a look at this please? It's already been through a
preliminary review by Tim/Nick. The code review site shows some of the renames
as separate adds / deletes, and there doesn't seem to be a way to avoid that. 

The trybot redness has to do with issues with mock keychain. This patch touches
only the tests that deal with bookmarks and preferences.

[Nicolas Zea, 2011-07-26 02:24:41]

On 2011/07/25 08:12:32, rsimha wrote:
> +zea as reviewer, since Tim is OOO.
> 
> Nicolas, could you take a look at this please? It's already been through a
> preliminary review by Tim/Nick. The code review site shows some of the renames
> as separate adds / deletes, and there doesn't seem to be a way to avoid that. 
> 
> The trybot redness has to do with issues with mock keychain. This patch
touches
> only the tests that deal with bookmarks and preferences.

Tim is back in office, and I didn't get around to going through this today,
leaving it to him :-)

[Raghu Simha, 2011-07-27 10:49:26]

Tim: Bumpity bump!

[tim (not reviewing), 2011-07-28 05:22:41]

this LGTM,  remind me again why we couldn't go with a non-static class member
for the helpers though?

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/bookm...
File chrome/test/live_sync/bookmarks_helper.cc (right):

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/bookm...
chrome/test/live_sync/bookmarks_helper.cc:307: if (test()->use_verifier()) {
Almost (almost) seems like a job for inheritance+factory, since you have this if
in almost every method ...

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/sync_...
File chrome/test/live_sync/sync_datatype_helper.cc (right):

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/sync_...
chrome/test/live_sync/sync_datatype_helper.cc:15: ASSERT_TRUE(test != NULL) <<
"Cannot associate with null test.";
ASSERT_NE

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/sync_...
chrome/test/live_sync/sync_datatype_helper.cc:22: EXPECT_TRUE(test_ != NULL) <<
"Must call AssociateWithTest first.";
EXPECT_EQ

[Raghu Simha, 2011-07-28 09:36:54]

Thanks for the review.

I chose the static-helper-class implementation over
non-static-member-of-LiveSyncTest due to the following reasons:

- In terms of added code, there wasn't much difference. ("alice.ChangeListPref"
vs. "PreferencesHelper::ChangeListPref")
- With static helpers, there's the notion of associating them with a test
instance (which happens automatically in the constructor of LiveSyncTest). All a
test case writer has to do in order to use a helper class in a new set of tests
is include a header file and invoke the helper method of choice.
- Meanwhile, if we were to use helpers that are non-static members of
LiveSyncTest, individual test classes (and perhaps, test cases) would need to
keep track of which helpers they're using and manage their lifetimes. 
- Also, with static helpers, adding a helper class for a new datatype involves
no changes to LiveSyncTest, while with non-static members, LiveSyncTest needs to
be updated every time a new datatype is added.

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/bookm...
File chrome/test/live_sync/bookmarks_helper.cc (right):

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/bookm...
chrome/test/live_sync/bookmarks_helper.cc:307: if (test()->use_verifier()) {
I implemented this via a dynamic check because a test case (a.k.a. an instance
of XXXClientBookmarksSyncTest) can decide to stop using the verifier midway
through the flow. Unless I'm wrong, using the inheritance model you suggest
would require tests to switch from using "BookmarksHelperWithVerifier" to a
"BookmarksHelperWithoutVerifier", which might end up being more complicated and
error prone than the check above. 

Example flow:

SetupSync();
ClientAChangesSomething(); // Same change made to verifier
ClientA->WaitForClientB();
VerifyAllClientsMatchVerifier(); // A = B = Verifier

DisableVerifier(); // Verifier is disregarded from here on
ClientAChangesSomething();
ClientBMakesAConflictingChange();
AwaitQuiescence(); //  End state is non-deterministic
VerifyAllClientsMatch(); // A = B

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/sync_...
File chrome/test/live_sync/sync_datatype_helper.cc (right):

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/sync_...
chrome/test/live_sync/sync_datatype_helper.cc:15: ASSERT_TRUE(test != NULL) <<
"Cannot associate with null test.";
On 2011/07/28 05:22:41, timsteele wrote:
> ASSERT_NE 

Tried that earlier -- it turns out that the ASSERT_* macros don't support
pointer types like LiveSyncTest* because NULL is defined as the integer 0.

error C2446: '!=' : no conversion from 'LiveSyncTest *const ' to 'const int'

http://codereview.chromium.org/7259005/diff/34001/chrome/test/live_sync/sync_...
chrome/test/live_sync/sync_datatype_helper.cc:22: EXPECT_TRUE(test_ != NULL) <<
"Must call AssociateWithTest first.";
On 2011/07/28 05:22:41, timsteele wrote:
> EXPECT_EQ

Same comment as above.