Implement testing support for "--compiler=none --runtime=dartium"

Implement testing support for "--compiler=none --runtime=dartium"

This is implemented without the broken unittest-based wrapping of tests.

R=kasperl@google.com

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

[kustermann, 2013-10-01 12:55:05]

While reading the dartium bindings code (because of the "Failed to load a file
..." issue) I discovered that anton implemented the 'print()' call interception.

Now we can get rid of the unittest-based wrapping in dartium (and probably in
content_shell -- not implemented in this CL).

Eventually we also want to get rid of the small wrapper (i.e. the two print()
statements around the real main).

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:883: }
I don't know if I should submit this, but we'd get valuable information on the
buildbot, why double reporting tests are broken.

[kustermann, 2013-10-01 13:09:35]

In case you're curious, here's antons CL:
https://codereview.chromium.org/12477005/

[kasperl, 2013-10-01 13:15:55]

LGTM.

https://codereview.chromium.org/25514002/diff/1001/tests/utils/utils.status
File tests/utils/utils.status (right):

https://codereview.chromium.org/25514002/diff/1001/tests/utils/utils.status#n...
tests/utils/utils.status:27: dart2js_test: Skip
Add comments that explains why you're skipping these.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:324: String _binary;
final? You'll need to initialize it in the initializer list.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:332: // This is extracted to a
function since we may need to support several
So this is just planning for future changes?

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:731:
doubleReportingTests.add(testId);
Isn't the (linked) key set of the doubleReportingOutputs enough to keep track of
the doubleReportingTests? Why have two data structures for this?

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:883: }
On 2013/10/01 12:55:06, kustermann wrote:
> I don't know if I should submit this, but we'd get valuable information on the
> buildbot, why double reporting tests are broken.

I'd submit it.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
File tools/testing/dart/browser_test.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_test.dart:83: import '$libraryPathComponent' as Test;
We tend to user lowercase for library prefixes.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:922: dartLibraryFilename,
Add a type annotation for this?

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1043: // FIXME(kustermann): This is broken,
we can't wrap tests in
Maybe move this FIXME comment to the implementation of _createWrapperFile
instead?

[kustermann, 2013-10-01 15:41:59]

Thank you.

https://codereview.chromium.org/25514002/diff/1001/tests/utils/utils.status
File tests/utils/utils.status (right):

https://codereview.chromium.org/25514002/diff/1001/tests/utils/utils.status#n...
tests/utils/utils.status:27: dart2js_test: Skip
On 2013/10/01 13:15:55, kasperl wrote:
> Add comments that explains why you're skipping these.

Done.

The layout tests are hopefully going away soonish (if I get green light from
sigmund).

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:324: String _binary;
On 2013/10/01 13:15:55, kasperl wrote:
> final? You'll need to initialize it in the initializer list.

I've tried:

  final String _binary;
  ...
  Chrome() : _binary = _getBinary();
  String _getBinary() { ... }

Results in

'file:///...../dart/tools/testing/dart/browser_controller.dart': Error: line 327
pos 24: illegal implicit access to receiver 'this'
  Chrome() : _binary = _getBinary();

I guess this only works if the method is static.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:332: // This is extracted to a
function since we may need to support several
On 2013/10/01 13:15:55, kasperl wrote:
> So this is just planning for future changes?

Yeah, rico put this in.

It's not really necessary at the moment. I'll remove the function.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:731:
doubleReportingTests.add(testId);
On 2013/10/01 13:15:55, kasperl wrote:
> Isn't the (linked) key set of the doubleReportingOutputs enough to keep track
of
> the doubleReportingTests? Why have two data structures for this?

The keys in doubleReportingOutputs are enough, that's correct.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
File tools/testing/dart/browser_test.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_test.dart:83: import '$libraryPathComponent' as Test;
On 2013/10/01 13:15:55, kasperl wrote:
> We tend to user lowercase for library prefixes.

Done.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:922: dartLibraryFilename,
On 2013/10/01 13:15:55, kasperl wrote:
> Add a type annotation for this?

Done.

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1043: // FIXME(kustermann): This is broken,
we can't wrap tests in
On 2013/10/01 13:15:55, kasperl wrote:
> Maybe move this FIXME comment to the implementation of _createWrapperFile
> instead?

Done.

[kustermann, 2013-10-02 07:38:10]

Committed patchset #3 manually as r28141 (presubmit successful).

[ricow1, 2013-10-02 18:17:47]

LGTM, awesome!

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:872: print("Double reporting tests");
Do we want to remove the double reporting here when we print it to the debug log
below?

https://codereview.chromium.org/25514002/diff/12001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/25514002/diff/12001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1046: _createWrapperFile(
is this basically drt now?

[kustermann, 2013-10-03 11:06:01]

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/25514002/diff/1001/tools/testing/dart/browser...
tools/testing/dart/browser_controller.dart:872: print("Double reporting tests");
On 2013/10/02 18:17:47, ricow1 wrote:
> Do we want to remove the double reporting here when we print it to the debug
log
> below?

For now I'd leave it, if you pass '--write-debug-log' you'll never see any
warning. (and if you don't pass it, you'll get tons of output -- nobody wants
that)

https://codereview.chromium.org/25514002/diff/12001/tools/testing/dart/test_s...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/25514002/diff/12001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1046: _createWrapperFile(
On 2013/10/02 18:17:47, ricow1 wrote:
> is this basically drt now?

Yes it is -- or rather content_shell.

I'll remove this unittest-based wrapping here as well (if the print forwarding
works on content_shell as well)