Fix a bunch of mojo_public_*_unittests with the new EDK.

Fix a bunch of mojo_public_*_unittests with the new EDK.

The unittests had baked in assumptions about implementation timings of the EDK.

This makes the following tests pass: mojo_public_bindings_unittests, mojo_public_environment_unittests, mojo_public_system_unittests and mojo_public_utility_unittests.

BUG=561803
Committed: https://crrev.com/0413ae3065a484364fbb8fb1b96f98e4bcc0a7c4
Cr-Commit-Position: refs/heads/master@{#367421}

[jam, 2016-01-01 21:23:54]

Description was changed from

==========
Fix a bunch of mojo_public_*_unittests with the new EDK.

The unittests had baked in assumptions about implementation timings of the EDK.

This makes the following tests pass: mojo_public_bindings_unittests,
mojo_public_environment_unittests, mojo_public_system_unittests and
mojo_public_utility_unittests.

BUG=561803
==========

to

==========
Fix a bunch of mojo_public_*_unittests with the new EDK.

The unittests had baked in assumptions about implementation timings of the EDK.

This makes the following tests pass: mojo_public_bindings_unittests,
mojo_public_environment_unittests, mojo_public_system_unittests and
mojo_public_utility_unittests.

BUG=561803
==========

[jam, 2016-01-01 21:23:54]

jam@chromium.org changed reviewers:
+ yzshen@chromium.org

[jam, 2016-01-04 15:20:35]

Patchset #3 (id:40001) has been deleted

[jam, 2016-01-04 15:21:00]

Patchset #2 (id:20001) has been deleted

[jam, 2016-01-04 15:21:12]

Patchset #1 (id:1) has been deleted

[yzshen1, 2016-01-04 18:54:26]

https://codereview.chromium.org/1552983003/diff/60001/mojo/message_pump/messa...
File mojo/message_pump/message_pump_mojo_unittest.cc (right):

https://codereview.chromium.org/1552983003/diff/60001/mojo/message_pump/messa...
mojo/message_pump/message_pump_mojo_unittest.cc:77: run_loop.RunUntilIdle();
Even when we wait till the handle is readable, it doesn't guarantee that the
second message (line 70) is available when RunUntilIdle() is called, right?
Theoretically the run loop can become idle again before the second message
arrives at the loop.

That makes me think that RunUntilIdle() is not very useful anymore, because
writing messages to the pipe doesn't make it "not idle" immediately nor
guarantee that it will stay "non idle" until all messages are consumed. (I don't
think this is the new EDK's fault, though.) Do you have good idea to fix this?
Or maybe we should avoid using this method?

Thanks!

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/c/system/te...
File mojo/public/c/system/tests/core_unittest.cc (right):

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/c/system/te...
mojo/public/c/system/tests/core_unittest.cc:255: // |hc| should still be
readable.
nit: Maybe we could consider keeping the wait for READABLE call (and adding a
wait for PEER_CLOSED as you did), or update this comment?

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/binding_callback_unittest.cc (left):

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_unittest.cc:124: void
PumpMessages() { loop_.RunUntilIdle(); }
I think this method can be removed (here and elsewhere).

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/request_response_unittest.cc (right):

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/request_response_unittest.cc:62: void Run(const
String& a) const { *buf_ = a; closure_.Run(); }
nit: please put the statements on separate lines. (here and below)

[jam, 2016-01-04 21:00:06]

https://codereview.chromium.org/1552983003/diff/60001/mojo/message_pump/messa...
File mojo/message_pump/message_pump_mojo_unittest.cc (right):

https://codereview.chromium.org/1552983003/diff/60001/mojo/message_pump/messa...
mojo/message_pump/message_pump_mojo_unittest.cc:77: run_loop.RunUntilIdle();
On 2016/01/04 18:54:25, yzshen1 wrote:
> Even when we wait till the handle is readable, it doesn't guarantee that the
> second message (line 70) is available when RunUntilIdle() is called, right?
> Theoretically the run loop can become idle again before the second message
> arrives at the loop.
> 
> That makes me think that RunUntilIdle() is not very useful anymore, because
> writing messages to the pipe doesn't make it "not idle" immediately nor
> guarantee that it will stay "non idle" until all messages are consumed. (I
don't
> think this is the new EDK's fault, though.) Do you have good idea to fix this?
> Or maybe we should avoid using this method?
> 
> Thanks!

good point. I've switched these tests to use callbacks as well.

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/c/system/te...
File mojo/public/c/system/tests/core_unittest.cc (right):

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/c/system/te...
mojo/public/c/system/tests/core_unittest.cc:255: // |hc| should still be
readable.
On 2016/01/04 18:54:25, yzshen1 wrote:
> nit: Maybe we could consider keeping the wait for READABLE call (and adding a
> wait for PEER_CLOSED as you did), or update this comment?

well, we still do that check for readable in line 260.

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/binding_callback_unittest.cc (left):

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/binding_callback_unittest.cc:124: void
PumpMessages() { loop_.RunUntilIdle(); }
On 2016/01/04 18:54:25, yzshen1 wrote:
> I think this method can be removed (here and elsewhere).

this is called below in one more place where i couldn't hook onto any callback

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/request_response_unittest.cc (right):

https://codereview.chromium.org/1552983003/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/request_response_unittest.cc:62: void Run(const
String& a) const { *buf_ = a; closure_.Run(); }
On 2016/01/04 18:54:25, yzshen1 wrote:
> nit: please put the statements on separate lines. (here and below)

Done.

[yzshen1, 2016-01-04 21:07:40]

lgtm

[jam, 2016-01-04 21:47:06]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-01-04 21:47:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1552983003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1552983003/100001

[commit-bot: I haz the power, 2016-01-05 00:06:18]

Description was changed from

==========
Fix a bunch of mojo_public_*_unittests with the new EDK.

The unittests had baked in assumptions about implementation timings of the EDK.

This makes the following tests pass: mojo_public_bindings_unittests,
mojo_public_environment_unittests, mojo_public_system_unittests and
mojo_public_utility_unittests.

BUG=561803
==========

to

==========
Fix a bunch of mojo_public_*_unittests with the new EDK.

The unittests had baked in assumptions about implementation timings of the EDK.

This makes the following tests pass: mojo_public_bindings_unittests,
mojo_public_environment_unittests, mojo_public_system_unittests and
mojo_public_utility_unittests.

BUG=561803
==========

[commit-bot: I haz the power, 2016-01-05 00:06:18]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-01-05 00:11:12]

Description was changed from

==========
Fix a bunch of mojo_public_*_unittests with the new EDK.

The unittests had baked in assumptions about implementation timings of the EDK.

This makes the following tests pass: mojo_public_bindings_unittests,
mojo_public_environment_unittests, mojo_public_system_unittests and
mojo_public_utility_unittests.

BUG=561803
==========

to

==========
Fix a bunch of mojo_public_*_unittests with the new EDK.

The unittests had baked in assumptions about implementation timings of the EDK.

This makes the following tests pass: mojo_public_bindings_unittests,
mojo_public_environment_unittests, mojo_public_system_unittests and
mojo_public_utility_unittests.

BUG=561803

Committed: https://crrev.com/0413ae3065a484364fbb8fb1b96f98e4bcc0a7c4
Cr-Commit-Position: refs/heads/master@{#367421}
==========

[commit-bot: I haz the power, 2016-01-05 00:11:15]

Patchset 3 (id:??) landed as
https://crrev.com/0413ae3065a484364fbb8fb1b96f98e4bcc0a7c4
Cr-Commit-Position: refs/heads/master@{#367421}