Make isolate tests work on drt (and hopefully dartium).

Make isolate tests work on drt (and hopefully dartium).

The current isolate tests don't work in drt/dartium because the
DOM isolate can't use Isolate.spawnFunction.

To avoid this problem, this change makes the affected tests start out
by doing an Isolate.spawnUri with their own main file, therevy running the test
in a non-DOM isolate. The test results are propagated back to the original
isolate.

I haven't been able to test dartium yet. I might be missing Selenium.

BUG= http://dartbug.com/14626
R=floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30610

[Lasse Reichstein Nielsen, 2013-11-14 16:22:10]

Can any of you see a problems with this approach?

This way of running tests always runs the tests in a new isolate (except for
dart2js code with no web-workers, where it falls back to running them directly,
because it can't do spawnUri then - we assume there is no case where both spawn
and spawnUri fails :).

Should we find a way to only introduce the new isolate when necessary (in a DOM
isolate, and how would we detect that?).

Is there a better way to find the URI of the current library than using mirrors?

[siva, 2013-11-14 17:21:08]

https://codereview.chromium.org/70103014/diff/40001/tests/isolate/remote_unit...
File tests/isolate/remote_unittest_helper.dart (right):

https://codereview.chromium.org/70103014/diff/40001/tests/isolate/remote_unit...
tests/isolate/remote_unittest_helper.dart:21: */
I find this a little tricky, you probably did it this way to minimize changes to
the tests.

Why not instead go with this scheme for all isolate tests:

existing main becomes testMain
new main would be:
void main([args, port]) {
  return testInNewIsolate(testMain, [args, port]);
}

Your current testRemote could be renamed to testInNewIsolate with the signature
also accepting 'args'.

the catch part in LocalTests.run would have to be changed to:
} catch (e) {
  // ....
  // ....
  return Function.apply(main, [args, port]);
}

[siva, 2013-11-14 17:21:39]

Thanks for taking this up.

[ricow1, 2013-11-15 10:45:14]

"I haven't been able to test dartium yet. I might be missing Selenium."
No more selenium - it has been retired, to test in dartium do:
tools/test.py -mrelease -rdartium -cnone
this will automatically fetch a dartium and run it in there

[Lasse Reichstein Nielsen, 2013-11-15 14:57:45]

https://codereview.chromium.org/70103014/diff/40001/tests/isolate/remote_unit...
File tests/isolate/remote_unittest_helper.dart (right):

https://codereview.chromium.org/70103014/diff/40001/tests/isolate/remote_unit...
tests/isolate/remote_unittest_helper.dart:21: */
Minimizing changes was a goal, yes :) Didn't succeed as well as I had hoped,
since I had to convert some tests to using the unittest package.

I could pass testMain instead of main to the function, and have the function
call testMain directly instead of returning a bool. I'm not sure it's much
simpler.

I use "main" because it guarantees that its library is the correct library to
rerun the "main" function of (by spawnUri). If I used "testMain" as argument,
I'd have to document that it had to be in the same library as "main" (which it
always would be for these fairly simple tests, but still, it's more fragile).

[siva, 2013-11-15 23:10:57]

I find the 'try it one way, if that fails then try it another way' a little
weird.

regarding your comment on 'testMain', it is being passed as a closure to
'testInNewIsolate', it is not required to be called testMain, it could be any
top level function in the test.

[Lasse Reichstein Nielsen, 2013-11-18 07:32:54]

On 2013/11/15 23:10:57, siva wrote:
> I find the 'try it one way, if that fails then try it another way' a little
> weird.Agree.

The rationale is:
We are testing Isolate.spawn. It doesn't work in a DOM isolate, so in those
cases we must run the test in a new isolate (using Isolate.spawnUri).
The only generic way to detect that case is to use Isolate.spawn and see it 
fail. Since we are testing that function, I don't want the test harness to
depend on its behavior in any way.
But Isolate.spawnUri doesn't always work either - in dart2js code run on d8,
you cant do spawnUri. Detecting that by catching is ok, it's not the function
we are testing. 
So, there are three cases:

Running in a DOM Isolate: 
    The tests must be run in a new isolate (spawn dosen't work).
Running dart2js standalone: 
    The tests must be run directly (spawnUri doesn't work).
All other cases: 
    The tests could/should be run directly, but I can't detect these cases,
    so I run the test in a new isolate anyway, which is safe.

So, yes, all but two of the cases are running in a new isolate unnecessarily, 
and if I could avoid it, I would. Any suggestions on detecting a DOM Isolate 
consistently across runtime environments will be appreciated :)

> regarding your comment on 'testMain', it is being passed as a closure to
> 'testInNewIsolate', it is not required to be called testMain, it could be any
> top level function in the test.

True, but it wouldn't change my argument. I don't depend on the name, but I do
depend on the library it is in, for getting the URI to use with spawnUri.
But I could pass both main and a testMain function to the test.

[Lasse Reichstein Nielsen, 2013-11-22 14:25:48]

I kept "main" as the function I pass, also because it makes it easier to use
"@MirrorsUsed" annotation (which reduces dart2js output by a megabyte in some
cases).

Removed the helper class, and made testRemote a single method.

Instead of starting tests on port events, I collect them all first, and then
simulate all the tests in one go (which is necessary to prevent unittest from
thinking it's done earlier).

[Lasse Reichstein Nielsen, 2013-11-22 14:27:07]

PTAL

[floitsch, 2013-11-22 17:02:03]

LGTM.

https://codereview.chromium.org/70103014/diff/220001/tests/isolate/isolate.st...
File tests/isolate/isolate.status (left):

https://codereview.chromium.org/70103014/diff/220001/tests/isolate/isolate.st...
tests/isolate/isolate.status:93: [ $compiler == none && ($runtime == drt ||
$runtime == dartium) ]
I don't think all of them will work.
The problem is that currently print and scheduleImmediate/Timer doesn't work in
other isolates yet.

https://codereview.chromium.org/70103014/diff/220001/tests/isolate/remote_uni...
File tests/isolate/remote_unittest_helper.dart (right):

https://codereview.chromium.org/70103014/diff/220001/tests/isolate/remote_uni...
tests/isolate/remote_unittest_helper.dart:64: Isolate.spawnUri(library.uri,
null, receivePort.sendPort);
normally spawnUri should not throw, but return the error in the returned future.

[Lasse Reichstein Nielsen, 2013-11-25 08:19:41]

https://codereview.chromium.org/70103014/diff/220001/tests/isolate/isolate.st...
File tests/isolate/isolate.status (left):

https://codereview.chromium.org/70103014/diff/220001/tests/isolate/isolate.st...
tests/isolate/isolate.status:93: [ $compiler == none && ($runtime == drt ||
$runtime == dartium) ]
You are right, stress_test fails in dartium. I haven't touched the test, and it
seems to fail for several different reasons, so I won't touch it now.

[Lasse Reichstein Nielsen, 2013-11-25 08:20:14]

Committed patchset #7 manually as r30610 (presubmit successful).