Make SignalSenderVerificationTest more robust

Make SignalSenderVerificationTest more robust

Add more assertions and a callback to check the result of RequestOwnership.

Original test can hang when test_service2_ tries to acquire the ownership before D-Bus recognizes test_service_'s disconnection.
In that situation, test_service2_ cannot own the name and cannot send a message.

BUG=158689
TEST=unittests


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

[satorux1, 2012-11-07 01:42:25]

Thank you for fixing the flakiness.

http://codereview.chromium.org/11358111/diff/1/dbus/signal_sender_verificatio...
File dbus/signal_sender_verification_unittest.cc (right):

http://codereview.chromium.org/11358111/diff/1/dbus/signal_sender_verificatio...
dbus/signal_sender_verification_unittest.cc:187:
base::PlatformThread::Sleep(TestTimeouts::action_timeout());
Sleep should be avoided. Can we monitor the ownership change and wait until the
owner is gone?

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.cc#newcode18
dbus/test_service.cc:18: void EmptyCallback(bool b) {
b -> /* success */

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.cc#newcode106
dbus/test_service.cc:106: base::Bind(EmptyCallback)));
base::Bind(&EmptyCallback)

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h
File dbus/test_service.h (right):

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode68
dbus/test_service.h:68: // Request the ownership of a well-known name
"TestService".
Please add some comment about the new parameter.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode69
dbus/test_service.h:69: void RequestOwnership(base::Callback<void(bool)>);
parameter name is missing.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode72
dbus/test_service.h:72: bool HasOwnership();
bool has_ownership() const { return has_ownership_; }

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode85
dbus/test_service.h:85: void OnOwnership(base::Callback<void(bool)> callback,
Please add some comment about the new parameter, as well as the existing
parameters. :)

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode146
dbus/test_service.h:146: bool has_name_ownership;
The trailing _ is missing.

[Haruki Sato, 2012-11-12 04:59:43]

sorry for the delay.
Confirmed that
for f in `seq 1 100`; do ./out/Debug/dbus_unittests; done
passes in my local environment.
PTAL.

http://codereview.chromium.org/11358111/diff/1/dbus/signal_sender_verificatio...
File dbus/signal_sender_verification_unittest.cc (right):

http://codereview.chromium.org/11358111/diff/1/dbus/signal_sender_verificatio...
dbus/signal_sender_verification_unittest.cc:187:
base::PlatformThread::Sleep(TestTimeouts::action_timeout());
On 2012/11/07 01:42:25, satorux1 wrote:
> Sleep should be avoided. Can we monitor the ownership change and wait until
the
> owner is gone?
Thank you for the advice!
Replaced with NameOwnerChanged listening via ObjectProxy.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.cc#newcode18
dbus/test_service.cc:18: void EmptyCallback(bool b) {
On 2012/11/07 01:42:25, satorux1 wrote:
> b -> /* success */

Done. Thanks.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.cc#newcode106
dbus/test_service.cc:106: base::Bind(EmptyCallback)));
On 2012/11/07 01:42:25, satorux1 wrote:
> base::Bind(&EmptyCallback)

Done. Thanks.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h
File dbus/test_service.h (right):

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode68
dbus/test_service.h:68: // Request the ownership of a well-known name
"TestService".
On 2012/11/07 01:42:25, satorux1 wrote:
> Please add some comment about the new parameter.

Done.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode69
dbus/test_service.h:69: void RequestOwnership(base::Callback<void(bool)>);
On 2012/11/07 01:42:25, satorux1 wrote:
> parameter name is missing.

Done. Thanks.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode72
dbus/test_service.h:72: bool HasOwnership();
On 2012/11/07 01:42:25, satorux1 wrote:
> bool has_ownership() const { return has_ownership_; }

Done.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode85
dbus/test_service.h:85: void OnOwnership(base::Callback<void(bool)> callback,
On 2012/11/07 01:42:25, satorux1 wrote:
> Please add some comment about the new parameter, as well as the existing
> parameters. :)

Done.

http://codereview.chromium.org/11358111/diff/1/dbus/test_service.h#newcode146
dbus/test_service.h:146: bool has_name_ownership;
On 2012/11/07 01:42:25, satorux1 wrote:
> The trailing _ is missing.

Done.

[satorux1, 2012-11-12 07:56:43]

http://codereview.chromium.org/11358111/diff/8004/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/11358111/diff/8004/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:643: name_owner_changed_callback_.Run(signal);
This will crash if name_owner_changed_callback_ is null. Can we assume that the
callback is always set? Otherwise, we should check if this is non null.

http://codereview.chromium.org/11358111/diff/8004/dbus/signal_sender_verifica...
File dbus/signal_sender_verification_unittest.cc (right):

http://codereview.chromium.org/11358111/diff/8004/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:98: base::Unretained(this)));
Cannot we call QuitMessageLoop() directly here? Otherwise, please add some
comment about why we need to do this through the message loop

http://codereview.chromium.org/11358111/diff/8004/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:205:
ASSERT_TRUE(latest_name_owner_.empty());
Add a comment like:

// latest_name_owner_ becomes empty as the owner is gone.

http://codereview.chromium.org/11358111/diff/8004/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:212:
ASSERT_FALSE(latest_name_owner_.empty());
// latest_name_owner_ becomes non empty as the new owner appears.

Besides, I think we should keep:

ASSERT_TRUE(test_service2_->has_ownership());

http://codereview.chromium.org/11358111/diff/8004/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/11358111/diff/8004/dbus/test_service.cc#newcode18
dbus/test_service.cc:18: void EmptyCallback(bool success) {
/* success */ as it's unused, per the style guide.

[Haruki Sato, 2012-11-13 06:23:28]

http://codereview.chromium.org/11358111/diff/8004/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/11358111/diff/8004/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:643: name_owner_changed_callback_.Run(signal);
On 2012/11/12 07:56:43, satorux1 wrote:
> This will crash if name_owner_changed_callback_ is null. Can we assume that
the
> callback is always set? Otherwise, we should check if this is non null.
> 
>   

Done. Thanks.

http://codereview.chromium.org/11358111/diff/8004/dbus/signal_sender_verifica...
File dbus/signal_sender_verification_unittest.cc (right):

http://codereview.chromium.org/11358111/diff/8004/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:98: base::Unretained(this)));
On 2012/11/12 07:56:43, satorux1 wrote:
> Cannot we call QuitMessageLoop() directly here? Otherwise, please add some
> comment about why we need to do this through the message loop

Done.

http://codereview.chromium.org/11358111/diff/8004/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:205:
ASSERT_TRUE(latest_name_owner_.empty());
On 2012/11/12 07:56:43, satorux1 wrote:
> Add a comment like:
> 
> // latest_name_owner_ becomes empty as the owner is gone.

Done.

http://codereview.chromium.org/11358111/diff/8004/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:212:
ASSERT_FALSE(latest_name_owner_.empty());
On 2012/11/12 07:56:43, satorux1 wrote:
> // latest_name_owner_ becomes non empty as the new owner appears.
> 
> Besides, I think we should keep:
> 
> ASSERT_TRUE(test_service2_->has_ownership());
Added comment.

http://codereview.chromium.org/11358111/diff/8004/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/11358111/diff/8004/dbus/test_service.cc#newcode18
dbus/test_service.cc:18: void EmptyCallback(bool success) {
On 2012/11/12 07:56:43, satorux1 wrote:
> /* success */ as it's unused, per the style guide.

Done. I see. Thanks!

[satorux1, 2012-11-13 06:32:03]

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
File dbus/signal_sender_verification_unittest.cc (right):

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:95: // PostTask to quit the
mMessageLoop as this is called from D-Bus thread.
mMessageLoop -> MessageLoop

Thank you for adding the comment.

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:202: latest_name_owner_ =
"dummyowner";
this looks rather unclean. At this moment, test_service_ owns the name, right? 

// latest_name_owner_ is non empty, as the name is owned by test_service_ now.
ASSERT_FALSE(latest_name_owner_.empty());

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:212: // OnNameOwnerChanged will
PostTask to quit the message loop.
OnNameOwnerChanged()

Let's add () for function names in comments.

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:213: message_loop_.Run();
What about calling this twice:

message_loop_.Run();  // For OnOwnership().
message_loop_.Run();  // For OnNameOwnerChanged().

We should also add message_loop_.Quit() in OnOwnership() too.

[satorux1, 2012-11-13 07:00:39]

http://codereview.chromium.org/11358111/diff/6003/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/11358111/diff/6003/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:628: DBusHandlerResult
ObjectProxy::HandleNameOwnerChanged(Signal* signal) {
This should be scoped_ptr<Signal>

http://codereview.chromium.org/11358111/diff/6003/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:644: name_owner_changed_callback_.Run(signal);
Let's not call this here as we are now on the D-Bus thread. Please post this to
the origin thread.

http://codereview.chromium.org/11358111/diff/6003/dbus/object_proxy.h
File dbus/object_proxy.h (right):

http://codereview.chromium.org/11358111/diff/6003/dbus/object_proxy.h#newcode147
dbus/object_proxy.h:147: // |service_name_|.
I think |callback| should be called on the origin thread (likely UI thread), not
on D-Bus thread. Please also document.

[Haruki Sato, 2012-11-14 06:35:15]

As discussed offline, changed the order of service setup and added checks for
callback calls using boolean flags.

PTAL.
Thanks!

http://codereview.chromium.org/11358111/diff/6003/dbus/object_proxy.h
File dbus/object_proxy.h (right):

http://codereview.chromium.org/11358111/diff/6003/dbus/object_proxy.h#newcode147
dbus/object_proxy.h:147: // |service_name_|.
On 2012/11/13 07:00:39, satorux1 wrote:
> I think |callback| should be called on the origin thread (likely UI thread),
not
> on D-Bus thread. Please also document.

Done. Thanks.

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
File dbus/signal_sender_verification_unittest.cc (right):

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:95: // PostTask to quit the
mMessageLoop as this is called from D-Bus thread.
On 2012/11/13 06:32:03, satorux1 wrote:
> mMessageLoop -> MessageLoop
> 
> Thank you for adding the comment.

This part is changed. Thank you for the review!

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:202: latest_name_owner_ =
"dummyowner";
On 2012/11/13 06:32:03, satorux1 wrote:
> this looks rather unclean. At this moment, test_service_ owns the name, right?

> 
> // latest_name_owner_ is non empty, as the name is owned by test_service_ now.
> ASSERT_FALSE(latest_name_owner_.empty());

Done.

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:212: // OnNameOwnerChanged will
PostTask to quit the message loop.
On 2012/11/13 06:32:03, satorux1 wrote:
> OnNameOwnerChanged()
> 
> Let's add () for function names in comments.

Done.

http://codereview.chromium.org/11358111/diff/6003/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:213: message_loop_.Run();
On 2012/11/13 06:32:03, satorux1 wrote:
> What about calling this twice:
> 
> message_loop_.Run();  // For OnOwnership().
> message_loop_.Run();  // For OnNameOwnerChanged().
> 
> We should also add message_loop_.Quit() in OnOwnership() too.

As discussed offline, I added more checks for callback calls using boolean
flags.

[satorux1, 2012-11-14 07:56:10]

http://codereview.chromium.org/11358111/diff/9002/dbus/signal_sender_verifica...
File dbus/signal_sender_verification_unittest.cc (right):

http://codereview.chromium.org/11358111/diff/9002/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:169: bool on_ownership_called_;
Please initialize them to false in the constructor.

[Haruki Sato, 2012-11-14 08:25:20]

http://codereview.chromium.org/11358111/diff/9002/dbus/signal_sender_verifica...
File dbus/signal_sender_verification_unittest.cc (right):

http://codereview.chromium.org/11358111/diff/9002/dbus/signal_sender_verifica...
dbus/signal_sender_verification_unittest.cc:169: bool on_ownership_called_;
On 2012/11/14 07:56:10, satorux1 wrote:
> Please initialize them to false in the constructor.

Done. Thanks.

[satorux1, 2012-11-14 08:27:33]

LGTM

[commit-bot: I haz the power, 2012-11-14 08:53:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/11358111/3014

[commit-bot: I haz the power, 2012-11-14 11:03:00]

Change committed as 167649