[Sync] Rework unit tests for ChromeInvalidationClient

[Sync] Rework unit tests for ChromeInvalidationClient

In particular, add unit tests that would have caught bug 139424.

Dep-inject InvalidationClient into ChromeInvalidationClient.

Use the function name 'UpdateRegisteredIds' consistently.

Replace some mocks with fakes.

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

[akalin, 2012-08-02 06:52:32]

+rlarocque for review

[akalin, 2012-08-02 06:57:29]

+dcheng fyi

[akalin, 2012-08-03 17:20:49]

On 2012/08/02 06:57:29, akalin wrote:
> +dcheng fyi

ping!

[akalin, 2012-08-06 18:02:58]

On 2012/08/03 17:20:49, akalin wrote:
> On 2012/08/02 06:57:29, akalin wrote:
> > +dcheng fyi
> 
> ping!

ping?

[akalin, 2012-08-07 00:49:46]

+zea since richard is on vacay

[Nicolas Zea, 2012-08-07 17:16:34]

Richard is back, giving it back to him since he's more familiar with the bug.

[rlarocque, 2012-08-07 18:21:48]

Sorry this took so long.  I forgot to setup my vacation auto-responder, which
probably would have helped.

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
File sync/notifier/chrome_invalidation_client.h (right):

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client.h:53:
invalidation::InvalidationListener*)> CreateInvalidationClientCallback;
I'm not sure that adding a function parameter is the best way to do this.  Why
not extend ChromeInvalidationClient to override the Start() (or a related helper
function), rather than exposing this to the caller?

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
File sync/notifier/chrome_invalidation_client_unittest.cc (right):

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client_unittest.cc:525:
EXPECT_EQ(registered_ids_, GetRegisteredIds());
How sure are we that  !GetRegisteredIds().Has(TYPE_X) implies that we receive no
notifications for TYPE_X?

Maybe I'm just being paranoid, but I worry that, since
ChromeInvalidationClient::GetRegisteredIds() isn't the lowest level of code,
there might be a bug in that layer which these unit tests would not catch.  

Maybe that's not a big concern.  I'll let you make the call.

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client_unittest.cc:668:
GetNotificationsDisabledReason());
Same concern here as with GetRegisteredIds().  Do we have tests which give us
confidence that GetNotificationsDisabledReason() == NO_NOTIFICATIONS_ERROR
implies that invalidation client is receiving and forwarding notifications?

[akalin, 2012-08-08 22:46:35]

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
File sync/notifier/chrome_invalidation_client.h (right):

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client.h:53:
invalidation::InvalidationListener*)> CreateInvalidationClientCallback;
On 2012/08/07 18:21:48, rlarocque wrote:
> I'm not sure that adding a function parameter is the best way to do this.  Why
> not extend ChromeInvalidationClient to override the Start() (or a related
helper
> function), rather than exposing this to the caller?

I think we want to move away from the mark-function-as-virtual-and-override
pattern, as it's less clear (IMO) than dep-injecting some sort of delegate.  In
this case, the only delegate method needed is a state-less constructor function,
so it can be replaced with a base::Callback.

I think it's the second-best way; the best way would be to dep-inject
InvalidationListener* directly, but that's difficult since you'd have to keep
around its SystemResources* object.

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
File sync/notifier/chrome_invalidation_client_unittest.cc (right):

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client_unittest.cc:525:
EXPECT_EQ(registered_ids_, GetRegisteredIds());
On 2012/08/07 18:21:48, rlarocque wrote:
> How sure are we that  !GetRegisteredIds().Has(TYPE_X) implies that we receive
no
> notifications for TYPE_X?
> 
> Maybe I'm just being paranoid, but I worry that, since
> ChromeInvalidationClient::GetRegisteredIds() isn't the lowest level of code,
> there might be a bug in that layer which these unit tests would not catch.  
> 
> Maybe that's not a big concern.  I'll let you make the call.  

Note that in this case, GetRegisteredIds() returns the value from
FakeInvalidationClient, which overrides invalidation::InvalidationClient.  In
other words, it's not simply reading back the registered IDs from
ChromeInvalidationClient (confusing I know).  So if it made it all the way down
to the Ticl layer, I think that's a pretty good test.

Of course, ideally, we'd have integration tests that exercise the Ticl code,
too.

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client_unittest.cc:668:
GetNotificationsDisabledReason());
On 2012/08/07 18:21:48, rlarocque wrote:
> Same concern here as with GetRegisteredIds().  Do we have tests which give us
> confidence that GetNotificationsDisabledReason() == NO_NOTIFICATIONS_ERROR
> implies that invalidation client is receiving and forwarding notifications?

Not specifically.  But I think there's nothing that's specifically preventing
invalidations from going through even if we think that notifications are off. 
What did you have in mind?

[rlarocque, 2012-08-08 23:22:56]

LGTM.

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
File sync/notifier/chrome_invalidation_client.h (right):

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client.h:53:
invalidation::InvalidationListener*)> CreateInvalidationClientCallback;
On 2012/08/08 22:46:35, akalin wrote:
> On 2012/08/07 18:21:48, rlarocque wrote:
> > I'm not sure that adding a function parameter is the best way to do this. 
Why
> > not extend ChromeInvalidationClient to override the Start() (or a related
> helper
> > function), rather than exposing this to the caller?
> 
> I think we want to move away from the mark-function-as-virtual-and-override
> pattern, as it's less clear (IMO) than dep-injecting some sort of delegate. 
In
> this case, the only delegate method needed is a state-less constructor
function,
> so it can be replaced with a base::Callback.
> 
> I think it's the second-best way; the best way would be to dep-inject
> InvalidationListener* directly, but that's difficult since you'd have to keep
> around its SystemResources* object.

I agree that dependency injection is generally better.  However, in this case we
end up with an invalidation::InvalidationListener* parameter which is used to
pass a pointer to the listener back to tests.  It would be nice to avoid that.

It sounds like you've put some thought into this.  I won't hold up the review
over this point.

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
File sync/notifier/chrome_invalidation_client_unittest.cc (right):

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client_unittest.cc:525:
EXPECT_EQ(registered_ids_, GetRegisteredIds());
On 2012/08/08 22:46:35, akalin wrote:
> On 2012/08/07 18:21:48, rlarocque wrote:
> > How sure are we that  !GetRegisteredIds().Has(TYPE_X) implies that we
receive
> no
> > notifications for TYPE_X?
> > 
> > Maybe I'm just being paranoid, but I worry that, since
> > ChromeInvalidationClient::GetRegisteredIds() isn't the lowest level of code,
> > there might be a bug in that layer which these unit tests would not catch.  
> > 
> > Maybe that's not a big concern.  I'll let you make the call.  
> 
> Note that in this case, GetRegisteredIds() returns the value from
> FakeInvalidationClient, which overrides invalidation::InvalidationClient.  In
> other words, it's not simply reading back the registered IDs from
> ChromeInvalidationClient (confusing I know).  So if it made it all the way
down
> to the Ticl layer, I think that's a pretty good test.
> 
> Of course, ideally, we'd have integration tests that exercise the Ticl code,
> too.

Never mind, I got confused because I didn't bother to read the definition of
GetRegisteredIds().

http://codereview.chromium.org/10827133/diff/2001/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client_unittest.cc:668:
GetNotificationsDisabledReason());
On 2012/08/08 22:46:35, akalin wrote:
> On 2012/08/07 18:21:48, rlarocque wrote:
> > Same concern here as with GetRegisteredIds().  Do we have tests which give
us
> > confidence that GetNotificationsDisabledReason() == NO_NOTIFICATIONS_ERROR
> > implies that invalidation client is receiving and forwarding notifications?
> 
> Not specifically.  But I think there's nothing that's specifically preventing
> invalidations from going through even if we think that notifications are off. 
> What did you have in mind?

Never mind, I misread this.  I thought that GetNotificationsDisabledReason() was
hooked up to ChromeInvalidationClient::GetState().  Because that value is based
on flags stored in the ChromeInvalidationClient, there's a risk that its state
might not reflect that of the notifier, which is where all the interesting stuff
happens.  

Because you're testing the InvalidationClient directly, I withdraw this comment.

[akalin, 2012-08-08 23:27:34]

On 2012/08/08 23:22:56, rlarocque wrote:
> I agree that dependency injection is generally better.  However, in this case
we
> end up with an invalidation::InvalidationListener* parameter which is used to
> pass a pointer to the listener back to tests.  It would be nice to avoid that.
> 
> It sounds like you've put some thought into this.  I won't hold up the review
> over this point.

Yeah, that's unfortunate.  But I think it's the lesser of two evils.

> Never mind, I misread this.  I thought that GetNotificationsDisabledReason()
was
> hooked up to ChromeInvalidationClient::GetState().  Because that value is
based
> on flags stored in the ChromeInvalidationClient, there's a risk that its state
> might not reflect that of the notifier, which is where all the interesting
stuff
> happens.  
> 
> Because you're testing the InvalidationClient directly, I withdraw this
comment.

Cool.  But note that GetNotificationsDisabledReason() looks up the CIC's
listener, which is still our code.

[commit-bot: I haz the power, 2012-08-08 23:27:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10827133/15

[commit-bot: I haz the power, 2012-08-09 01:11:36]

Change committed as 150665