All isolate tests running on vm

All isolate tests running on vm

[Lasse Reichstein Nielsen, 2013-10-23 10:08:21]

Ready for comments or to be imported into your patch.

[floitsch, 2013-10-23 13:33:24]

LGTM with lots of minor nits.
More important:
 - unresolved_ports_test looks broken.
 - illegal_msg_test.dart seems to have dead code and not really test everything.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/count_test.dart
File tests/isolate/count_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/count_test.dart...
tests/isolate/count_test.dart:14: print("iso: $message");
debug print.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/count_test.dart...
tests/isolate/count_test.dart:19: print("iso-closed");
ditto

https://codereview.chromium.org/36933002/diff/1/tests/isolate/count_test.dart...
tests/isolate/count_test.dart:36: print("Main: ${msg[0]}");
debug print.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/cross_isolate_m...
File tests/isolate/cross_isolate_message_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/cross_isolate_m...
tests/isolate/cross_isolate_message_test.dart:30: local.close();
you can drop the close. "first" already closes.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/cross_isolate_m...
tests/isolate/cross_isolate_message_test.dart:44: print(msg[0]);
debug print.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/illegal_msg_tes...
File tests/isolate/illegal_msg_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/illegal_msg_tes...
tests/isolate/illegal_msg_test.dart:27: asyncEnd();
Looks like the second part is never executed.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/illegal_msg_tes...
tests/isolate/illegal_msg_test.dart:37: asyncStart();
This would yield an error.
you are not allowed to do:
start, end, start end.
they must overlap.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/isolate_complex...
File tests/isolate/isolate_complex_messages_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/isolate_complex...
tests/isolate/isolate_complex_messages_test.dart:30: local.close();
we should make sure that we reach 'done'.
var done = expectAsync0(() {});
...

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mandel_isolate_...
File tests/isolate/mandel_isolate_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mandel_isolate_...
tests/isolate/mandel_isolate_test.dart:99: Future isolateStart =
Isolate.spawn(processLines, reply.sendPort);
No need for temporary variable.

Isolate.spawn(processLines, reply.sendPort)
  .then((_) => reply.first)
  .then((port) => new LineProcessorClient(state, id, port));

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mandel_isolate_...
tests/isolate/mandel_isolate_test.dart:109: _port.send([y, reply.sendPort]);
I prefer the "right" order.

ReceivePort reply = ...
_port.send([y, reply.sendPort]);
reply.first.then(...)

https://codereview.chromium.org/36933002/diff/1/tests/isolate/message2_test.dart
File tests/isolate/message2_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/message2_test.d...
tests/isolate/message2_test.dart:69: remote.send([m, replyPort.sendPort]);
You could have send this as initial message, but no need to change now.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mint_maker_test...
File tests/isolate/mint_maker_test.dart (left):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mint_maker_test...
tests/isolate/mint_maker_test.dart:5: // Things that should be "auto-generated"
are between AUTO START and
I take it we only kept the test as is, but removed the generator?

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mint_maker_test...
File tests/isolate/mint_maker_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mint_maker_test...
tests/isolate/mint_maker_test.dart:48: _mint.send([balance, reply.sendPort]);
Switch order of lines.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mint_maker_test...
tests/isolate/mint_maker_test.dart:111: ReceivePort reply = new ReceivePort();
Create a sendReceive helper method.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/nested_spawn2_t...
File tests/isolate/nested_spawn2_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/nested_spawn2_t...
tests/isolate/nested_spawn2_test.dart:20: init.send(port.sendPort);
move send before port.first.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/nested_spawn2_t...
tests/isolate/nested_spawn2_test.dart:34: print("-$msg");
debug print.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/request_reply_t...
File tests/isolate/request_reply_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/request_reply_t...
tests/isolate/request_reply_test.dart:24: Isolate.spawn(entry, init.sendPort);
You could have sent "99" as initial message.
No need to change now.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/spawn_function_...
File tests/isolate/spawn_function_custom_class_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/spawn_function_...
tests/isolate/spawn_function_custom_class_test.dart:34: Isolate.spawn(child,
['hi', port.sendPort]);
I prefer the sending before the listening.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/spawn_function_...
File tests/isolate/spawn_function_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/spawn_function_...
tests/isolate/spawn_function_test.dart:24: Isolate.spawn(child, ['hi',
port.sendPort]);
move before listen call.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/unresolved_port...
File tests/isolate/unresolved_ports_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/unresolved_port...
tests/isolate/unresolved_ports_test.dart:27:
spawnFunction(bobIsolate).then((bob) {
Still uses spawnFunction.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/unresolved_port...
tests/isolate/unresolved_ports_test.dart:64:
spawnFunction(timIsolate).then((tim) {
ditto.

[Lasse Reichstein Nielsen, 2013-10-24 10:26:01]

Addressed some comments. Didn't check unresolved ports test.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/count_test.dart
File tests/isolate/count_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/count_test.dart...
tests/isolate/count_test.dart:14: print("iso: $message");
All removed.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/cross_isolate_m...
File tests/isolate/cross_isolate_message_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/cross_isolate_m...
tests/isolate/cross_isolate_message_test.dart:30: local.close();
On 2013/10/23 13:33:24, floitsch wrote:
> you can drop the close. "first" already closes.

Done.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/cross_isolate_m...
tests/isolate/cross_isolate_message_test.dart:44: print(msg[0]);
On 2013/10/23 13:33:24, floitsch wrote:
> debug print.

Done.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/illegal_msg_tes...
File tests/isolate/illegal_msg_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/illegal_msg_tes...
tests/isolate/illegal_msg_test.dart:15: port.list((msg) {
This is a typo, so this code doesn't run either (which is expected, sending the
message should fail in "snd.send" below, so we never reach the isolate).

https://codereview.chromium.org/36933002/diff/1/tests/isolate/illegal_msg_tes...
tests/isolate/illegal_msg_test.dart:27: asyncEnd();
It never is if the test succeeds. The expected behavior is to catch the
exception. Only if we successfully sends the bad message will the isolate be
used at all, ditto the else-branch below.

Moved asyncEnd to end of function, so async ranges overlap.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/illegal_msg_tes...
tests/isolate/illegal_msg_test.dart:37: asyncStart();
Now overlaps.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/isolate_complex...
File tests/isolate/isolate_complex_messages_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/isolate_complex...
tests/isolate/isolate_complex_messages_test.dart:30: local.close();
Done.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mandel_isolate_...
File tests/isolate/mandel_isolate_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mandel_isolate_...
tests/isolate/mandel_isolate_test.dart:99: Future isolateStart =
Isolate.spawn(processLines, reply.sendPort);
I just thought the line-breaks looked better this way.
It looks artificial to split up reply.first.then by returning reply.first.

I've tried a different rewrite.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mandel_isolate_...
tests/isolate/mandel_isolate_test.dart:109: _port.send([y, reply.sendPort]);
I actually consider that the wrong order.
The safest order should be:
- Create port 
- set up listeners on port 
- reveal port to someone else.

Revealing the port before setting up the listeners will work because the stream
is buffering, but it still feels a little wrong.

I can understand the send-request/handle-reply sequencing as well, and I've not
been consistent since both versions work, so I'll change it.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mint_maker_test...
File tests/isolate/mint_maker_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mint_maker_test...
tests/isolate/mint_maker_test.dart:48: _mint.send([balance, reply.sendPort]);
Ok, but I don't necessarily agree that it's better.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/mint_maker_test...
tests/isolate/mint_maker_test.dart:111: ReceivePort reply = new ReceivePort();
On 2013/10/23 13:33:24, floitsch wrote:
> Create a sendReceive helper method.

Done.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/nested_spawn2_t...
File tests/isolate/nested_spawn2_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/nested_spawn2_t...
tests/isolate/nested_spawn2_test.dart:20: init.send(port.sendPort);
On 2013/10/23 13:33:24, floitsch wrote:
> move send before port.first.

Done.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/nested_spawn2_t...
tests/isolate/nested_spawn2_test.dart:34: print("-$msg");
On 2013/10/23 13:33:24, floitsch wrote:
> debug print.

Done.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/request_reply_t...
File tests/isolate/request_reply_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/request_reply_t...
tests/isolate/request_reply_test.dart:24: Isolate.spawn(entry, init.sendPort);
The test name suggested a two-way message communication, so I wanted messages
sent in both directions.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/spawn_function_...
File tests/isolate/spawn_function_custom_class_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/spawn_function_...
tests/isolate/spawn_function_custom_class_test.dart:34: Isolate.spawn(child,
['hi', port.sendPort]);
On 2013/10/23 13:33:24, floitsch wrote:
> I prefer the sending before the listening.

Done.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/spawn_function_...
File tests/isolate/spawn_function_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/spawn_function_...
tests/isolate/spawn_function_test.dart:24: Isolate.spawn(child, ['hi',
port.sendPort]);
If I could put it on one line, I'd have done:
  var port = new ReceivePort()..listen(() {
    ...
  });
  Isolate.spawn(...)

But done.

https://codereview.chromium.org/36933002/diff/1/tests/isolate/unresolved_port...
File tests/isolate/unresolved_ports_test.dart (right):

https://codereview.chromium.org/36933002/diff/1/tests/isolate/unresolved_port...
tests/isolate/unresolved_ports_test.dart:27:
spawnFunction(bobIsolate).then((bob) {
Confusingly, the spawnFunction is a new function I declared below.
I works similar to the original spawnFunction in that it returns a SendPort (but
in a Future).