Run dart2js tests unmodified in browser.

Run dart2js tests unmodified in browser.

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

[ahe, 2012-09-05 15:36:15]

Seems to work for

./tools/test.py --checked -cdart2js -rdrt language -mrelease

[kasperl, 2012-09-06 10:29:29]

Comments:

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/js_backend/emitter.dart:1218: if (typeof
dartMainRunner == 'function') {
Somehow it feels shaky to not wait until the document has loaded if the
dartMainRunner hook has been set. Would it be possible to simply wrap the
mainCall in dartMainRunner if it is set instead but keep the document.readyState
logic?

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/pkg/unittest/test...
File dart/pkg/unittest/test_controller.js (right):

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/pkg/unittest/test...
dart/pkg/unittest/test_controller.js:85: main();
We have had issues with uncaught JavaScript exceptions falling out of main which
leads to tests that time out. Maybe this would be a great time to wrap the
main() call in try-catch?

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/tools/testing/dar...
File dart/tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/tools/testing/dar...
dart/tools/testing/dart/test_suite.dart:707: * The [StandardTestSuite] has
support for tests that
Javascript -> JavaScript (a few times)

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/tools/testing/dar...
dart/tools/testing/dart/test_suite.dart:717: void
enqueueBrowserTestNoFuss(TestInformation info,
It's hard for me to see what the difference between this and enqueueBrowserTest
is exactly. Would a boolean flag be better? Do you think we'll continue to need
both kinds?

At least it would be nice to be able to share more of the code through some
helpers.

[ahe, 2012-09-08 13:32:32]

PTAL. All tests pass now.

dart2js/drt tests are twice as fast on my mac.

Note: I have no intention of submitting this before I have talked to gram@.

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/js_backend/emitter.dart:1218: if (typeof
dartMainRunner == 'function') {
On 2012/09/06 10:29:29, kasperl wrote:
> Somehow it feels shaky to not wait until the document has loaded if the
> dartMainRunner hook has been set. Would it be possible to simply wrap the
> mainCall in dartMainRunner if it is set instead but keep the
document.readyState
> logic?

Done.

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/pkg/unittest/test...
File dart/pkg/unittest/test_controller.js (right):

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/pkg/unittest/test...
dart/pkg/unittest/test_controller.js:85: main();
On 2012/09/06 10:29:29, kasperl wrote:
> We have had issues with uncaught JavaScript exceptions falling out of main
which
> leads to tests that time out. Maybe this would be a great time to wrap the
> main() call in try-catch?

I think that is handled by 'window.addEventListener("error",...' above.

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/tools/testing/dar...
File dart/tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/1/dart/tools/testing/dar...
dart/tools/testing/dart/test_suite.dart:717: void
enqueueBrowserTestNoFuss(TestInformation info,
On 2012/09/06 10:29:29, kasperl wrote:
> It's hard for me to see what the difference between this and
enqueueBrowserTest
> is exactly. Would a boolean flag be better? Do you think we'll continue to
need
> both kinds?
> 
> At least it would be nice to be able to share more of the code through some
> helpers.

I'll refactor this before submitting.

[kasperl, 2012-09-10 07:37:03]

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
File dart/lib/compiler/implementation/lib/isolate_patch.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
dart/lib/compiler/implementation/lib/isolate_patch.dart:1238: void
clearTimeout(int _handle) native;
Why does the parameter name start with an underscore?

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
File dart/lib/compiler/implementation/lib/mock.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
dart/lib/compiler/implementation/lib/mock.dart:20: void
$$postGlobalMessage(String message) {
Add a comment here that explains why you need this forwarder with the slightly
weird name?

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/pkg/unittest/t...
File dart/pkg/unittest/test_controller.js (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/pkg/unittest/t...
dart/pkg/unittest/test_controller.js:100: main();
Should the done post message be handling in a finally block to make sure we get
it?

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/tools/testing/...
File dart/tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/tools/testing/...
dart/tools/testing/dart/test_suite.dart:717: void
enqueueBrowserTestNoFuss(TestInformation info,
Can this share more with the code in enqueueBrowserTest? I would expect the
comment for this method to highlight the differences between this and the
standard way of enqueuing a brower test. Maybe rename to
enqueueBrowserTestNoWrapping?

[ahe, 2012-09-10 08:31:13]

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
File dart/lib/compiler/implementation/lib/isolate_patch.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
dart/lib/compiler/implementation/lib/isolate_patch.dart:1238: void
clearTimeout(int _handle) native;
On 2012/09/10 07:37:03, kasperl wrote:
> Why does the parameter name start with an underscore?

Cut-and-paste error. Will fix.

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
File dart/lib/compiler/implementation/lib/mock.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
dart/lib/compiler/implementation/lib/mock.dart:20: void
$$postGlobalMessage(String message) {
On 2012/09/10 07:37:03, kasperl wrote:
> Add a comment here that explains why you need this forwarder with the slightly
> weird name?

I should not commit this as it will break all usages of unit tests on the VM.

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/pkg/unittest/t...
File dart/pkg/unittest/test_controller.js (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/pkg/unittest/t...
dart/pkg/unittest/test_controller.js:100: main();
On 2012/09/10 07:37:03, kasperl wrote:
> Should the done post message be handling in a finally block to make sure we
get
> it?

That is effectively what line 70 does.

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/tools/testing/...
File dart/tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/tools/testing/...
dart/tools/testing/dart/test_suite.dart:717: void
enqueueBrowserTestNoFuss(TestInformation info,
On 2012/09/10 07:37:03, kasperl wrote:
> Can this share more with the code in enqueueBrowserTest? I would expect the
> comment for this method to highlight the differences between this and the
> standard way of enqueuing a brower test. Maybe rename to
> enqueueBrowserTestNoWrapping?

As I pointed out in my previous email, I'll refactor this code before
submitting.

[ngeoffray, 2012-09-10 08:51:14]

http://codereview.chromium.org/10919098/diff/5001/dart/pkg/unittest/config.dart
File dart/pkg/unittest/config.dart (right):

http://codereview.chromium.org/10919098/diff/5001/dart/pkg/unittest/config.da...
dart/pkg/unittest/config.dart:41:
$$postGlobalMessage('unittest-suite-wait-for-done');
As discussed, it would be nice to be able to use print here instead.

[ahe, 2012-09-10 10:35:59]

PTAL

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
File dart/lib/compiler/implementation/lib/isolate_patch.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
dart/lib/compiler/implementation/lib/isolate_patch.dart:1238: void
clearTimeout(int _handle) native;
On 2012/09/10 08:31:14, ahe wrote:
> On 2012/09/10 07:37:03, kasperl wrote:
> > Why does the parameter name start with an underscore?
> 
> Cut-and-paste error. Will fix.

Done.

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
File dart/lib/compiler/implementation/lib/mock.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/lib/compiler/i...
dart/lib/compiler/implementation/lib/mock.dart:20: void
$$postGlobalMessage(String message) {
On 2012/09/10 08:31:14, ahe wrote:
> On 2012/09/10 07:37:03, kasperl wrote:
> > Add a comment here that explains why you need this forwarder with the
slightly
> > weird name?
> 
> I should not commit this as it will break all usages of unit tests on the VM.

Removed. Using print now.

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/pkg/unittest/c...
File dart/pkg/unittest/config.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/5001/dart/pkg/unittest/c...
dart/pkg/unittest/config.dart:41:
$$postGlobalMessage('unittest-suite-wait-for-done');
On 2012/09/10 08:51:14, ngeoffray wrote:
> As discussed, it would be nice to be able to use print here instead.

Done.

[kasperl, 2012-09-10 10:46:30]

https://chromiumcodereview.appspot.com/10919098/diff/1006/dart/pkg/unittest/c...
File dart/pkg/unittest/config.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/1006/dart/pkg/unittest/c...
dart/pkg/unittest/config.dart:149: print(message);
Would it make sense to add a prefix to these messages here? Somehow that would
make it easier to grok that the postMessage implementation here does its trick
and encodes the secret messages instead of letting all users of postMessage deal
with it?

   print("POSTMESSAGE: $message");

https://chromiumcodereview.appspot.com/10919098/diff/1006/dart/pkg/unittest/t...
File dart/pkg/unittest/test_controller.js (right):

https://chromiumcodereview.appspot.com/10919098/diff/1006/dart/pkg/unittest/t...
dart/pkg/unittest/test_controller.js:93: if ((msg === 'unittest-suite-success')
Maybe this could be based on a message prefix like POSTMESSAGE or something like
that?

[ahe, 2012-09-10 10:48:05]

https://chromiumcodereview.appspot.com/10919098/diff/1006/dart/pkg/unittest/c...
File dart/pkg/unittest/config.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/1006/dart/pkg/unittest/c...
dart/pkg/unittest/config.dart:149: print(message);
On 2012/09/10 10:46:30, kasperl wrote:
> Would it make sense to add a prefix to these messages here? Somehow that would
> make it easier to grok that the postMessage implementation here does its trick
> and encodes the secret messages instead of letting all users of postMessage
deal
> with it?
> 
>    print("POSTMESSAGE: $message");
> 

There is already a prefix, "unittest-". I'm not sure there is much value to
adding additional prefixes.

[gram, 2012-09-10 16:25:20]

As I'm not familiar with the details of dart2js, I find it very hard to
understand what the implications are of the changes to emitter.dart in this CL.
It would be great if some time when you guys are under less pressure if you
could add some comments to the code there to explain what is going on (not only
the new changes but the rest of the code too).

https://chromiumcodereview.appspot.com/10919098/diff/1006/dart/tools/testing/...
File dart/tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/10919098/diff/1006/dart/tools/testing/...
dart/tools/testing/dart/test_suite.dart:712: * two sequential steps to be run by
the [ProcessQueue when] the test is
Is this a typo? [ProcessQueue when] vs [ProcessQueue] when? Or is when a method?

[ahe, 2012-09-10 16:49:57]

On 2012/09/10 16:25:20, gram wrote:
> As I'm not familiar with the details of dart2js, I find it very hard to
> understand what the implications are of the changes to emitter.dart in this
CL.
> It would be great if some time when you guys are under less pressure if you
> could add some comments to the code there to explain what is going on (not
only
> the new changes but the rest of the code too).

Could you send me a list of questions? I'll add comments for what you concretely
think is confusing.

Cheers,
Peter

[ahe, 2012-09-10 20:26:16]

Hi Graham,

Thank you for the suggestions about more comments in the emitter. I know it's
just a drop in the ocean, but I hope the comments I added makes this CL easier
to understand.

Cheers,
Peter

[ahe, 2012-09-10 20:51:41]

The code is commented and I have cleaned up enqueueBrowserTests.

As far as I'm concerned, this is ready to submit.

[gram, 2012-09-10 21:08:19]

On 2012/09/10 20:51:41, ahe wrote:
> The code is commented and I have cleaned up enqueueBrowserTests.
> 
> As far as I'm concerned, this is ready to submit.

Thanks Peter, now I understand at least that this is related to the way print
gets tweaked to do message posting.

One day I will take the time to understand the compiler in more detail and will
tell you what I struggle with and where more comments could help.

[kasperl, 2012-09-11 06:29:00]

LGTM.