Rename test.dart component to specify compiler + runtime.

Rename test.dart component to specify compiler + runtime.
Committed: https://code.google.com/p/dart/source/detail?r=5893

[Emily Fortuna, 2012-03-23 23:14:43]

This is the dart repo version for renaming how we specify tests. Things seem to
work, but there's a chance I've messed up some test configuration I'm less
familiar with, so I'm cc'ing hopefully someone working on each group of bots to
make sure that it works with their system.

I have another CL for the VM and Dart Editor change in the chromium codebase
coming out in a minute. 

The short version is this change will change how we specify tests instead of
using the --component flag (and get rid of all the awesome names we have like
legium) and instead specify a --compiler flag (if necessary) and/or a --runtime
flag.

[Siggi Cherem (dart-lang), 2012-03-23 23:57:32]

https://chromiumcodereview.appspot.com/9838068/diff/7010/compiler/tests/dartc...
File compiler/tests/dartc/dartc.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/compiler/tests/dartc...
compiler/tests/dartc/dartc.status:20: [ $runtime == vm ]
how about removing this, but add
 || $compiler == none
below? 

This would make it so we also skip dartium.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
File frog/scripts/buildbot_annotated_steps.py (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:32: - compiler: 'dart2js', 'frog',
'frogsh', or None when the builder has an
- also 'dartc'?
- is 'none' allowed, or should we say that None is also when the builder doesn't
specify a compiler (e.g. vm/dartium)

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:34: - runtime: 'v8', 'ie', 'ff',
'safari', 'chrome', 'opera'
v8 or d8? (I've seen d8 in some status files)

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:172: # TODO(jmesserly): make
DumpRenderTree more like other browser tests, so
seems like this TODO is obsolete, but the translation chrome->drt should
continue to exist?

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:254: if runtime != 'd8' or (system ==
'linux' and runtime == 'chrome'):
d8/v8

https://chromiumcodereview.appspot.com/9838068/diff/7010/samples/tests/sample...
File samples/tests/samples/samples-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/samples/tests/sample...
samples/tests/samples/samples-leg.status:6: [ $compiler == dart2js ]
&& $runtime == d8?

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_options.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:77: ['none', 'frog', 'dart2js', 'dartc'],
seems like you still need frogsh? (saw it in status files)

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:428: return _expandHelper('runtime',
configuration);
could we add a function (or a TODO) that checks that the blend of configurations
makes sense and issue a warning/error if they don't? For example:

-c dartc -r drt would be invalid.

I prefer to issue a warning, so that if you specify many combinations together
in the command line, e.g.: '-c frog,dartc -r none,drt', then you see some
warnings but all other valid tests are run:
 warning: frog_none is not valid
 warning: dartc_drt is not valid
 running tests for frog_drt
 running tests for dartc_none

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:142: || configuration['runtime'] == 'opera';
an alternative suggestion:

bool get usesWebDriver() =>
 (const ['chrome', 'ff', 'safari', 'ie', 'opera'])
    .indexOf(configuration['runtime']) >= 0;

(I wanted to use [contains] rather than [indexOf], but it is not in the list
collection yet).

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:1180: const <String>['dartium',
dartium -> drt?

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:1185: 'ff'].some((x) => x == runtime);
:)  this is basically what I had in mind for [usesWebDriver]

https://chromiumcodereview.appspot.com/9838068/diff/7010/utils/tests/css/css....
File utils/tests/css/css.status (left):

https://chromiumcodereview.appspot.com/9838068/diff/7010/utils/tests/css/css....
utils/tests/css/css.status:11: [$component == webdriver && $browser == opera]
is opera working! :)?

[zundel, 2012-03-24 01:11:10]

Hi Emily,

I patched it in and ran the tool with:

./tools/test.py  -c dartc

This runs 'dartc_vm_debug_ia32' by default which is an invalid configuration. 
It runs for a while but then spits out lots of errors half way through the test
run.

The only valid runtime parameter with 'dartc' is 'none':

./tools/test.py -c dartc -r none

That one runs to successful completion.  Would it be possible to make the
runtime 'none' whenever 'dartc' is specified and error out on other values?

Thanks,
-Eric.

On 2012/03/23 23:57:32, sigmund wrote:
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/compiler/tests/dartc...
> File compiler/tests/dartc/dartc.status (right):
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/compiler/tests/dartc...
> compiler/tests/dartc/dartc.status:20: [ $runtime == vm ]
> how about removing this, but add
>  || $compiler == none
> below? 
> 
> This would make it so we also skip dartium.
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
> File frog/scripts/buildbot_annotated_steps.py (right):
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
> frog/scripts/buildbot_annotated_steps.py:32: - compiler: 'dart2js', 'frog',
> 'frogsh', or None when the builder has an
> - also 'dartc'?
> - is 'none' allowed, or should we say that None is also when the builder
doesn't
> specify a compiler (e.g. vm/dartium)
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
> frog/scripts/buildbot_annotated_steps.py:34: - runtime: 'v8', 'ie', 'ff',
> 'safari', 'chrome', 'opera'
> v8 or d8? (I've seen d8 in some status files)
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
> frog/scripts/buildbot_annotated_steps.py:172: # TODO(jmesserly): make
> DumpRenderTree more like other browser tests, so
> seems like this TODO is obsolete, but the translation chrome->drt should
> continue to exist?
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
> frog/scripts/buildbot_annotated_steps.py:254: if runtime != 'd8' or (system ==
> 'linux' and runtime == 'chrome'):
> d8/v8
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/samples/tests/sample...
> File samples/tests/samples/samples-leg.status (right):
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/samples/tests/sample...
> samples/tests/samples/samples-leg.status:6: [ $compiler == dart2js ]
> && $runtime == d8?
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
> File tools/testing/dart/test_options.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
> tools/testing/dart/test_options.dart:77: ['none', 'frog', 'dart2js', 'dartc'],
> seems like you still need frogsh? (saw it in status files)
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
> tools/testing/dart/test_options.dart:428: return _expandHelper('runtime',
> configuration);
> could we add a function (or a TODO) that checks that the blend of
configurations
> makes sense and issue a warning/error if they don't? For example:
> 
> -c dartc -r drt would be invalid.
> 
> I prefer to issue a warning, so that if you specify many combinations together
> in the command line, e.g.: '-c frog,dartc -r none,drt', then you see some
> warnings but all other valid tests are run:
>  warning: frog_none is not valid
>  warning: dartc_drt is not valid
>  running tests for frog_drt
>  running tests for dartc_none
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
> tools/testing/dart/test_runner.dart:142: || configuration['runtime'] ==
'opera';
> an alternative suggestion:
> 
> bool get usesWebDriver() =>
>  (const ['chrome', 'ff', 'safari', 'ie', 'opera'])
>     .indexOf(configuration['runtime']) >= 0;
> 
> (I wanted to use [contains] rather than [indexOf], but it is not in the list
> collection yet).
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
> File tools/testing/dart/test_suite.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
> tools/testing/dart/test_suite.dart:1180: const <String>['dartium',
> dartium -> drt?
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
> tools/testing/dart/test_suite.dart:1185: 'ff'].some((x) => x == runtime);
> :)  this is basically what I had in mind for [usesWebDriver]
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/utils/tests/css/css....
> File utils/tests/css/css.status (left):
> 
>
https://chromiumcodereview.appspot.com/9838068/diff/7010/utils/tests/css/css....
> utils/tests/css/css.status:11: [$component == webdriver && $browser == opera]
> is opera working! :)?

[ahe, 2012-03-24 13:56:39]

LGTM with comments.

https://chromiumcodereview.appspot.com/9838068/diff/7010/client/tests/client/...
File client/tests/client/client-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/client/tests/client/...
client/tests/client/client-leg.status:5: [ $compiler == dart2js && ($runtime ==
drt || $runtime == d8) ]
Just:

[ $compiler == dart2js ]

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
File frog/scripts/buildbot_annotated_steps.py (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:32: - compiler: 'dart2js', 'frog',
'frogsh', or None when the builder has an
On 2012/03/23 23:57:33, sigmund wrote:
> - also 'dartc'?
> - is 'none' allowed, or should we say that None is also when the builder
doesn't
> specify a compiler (e.g. vm/dartium)

dartc is not using this script.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:34: - runtime: 'v8', 'ie', 'ff',
'safari', 'chrome', 'opera'
On 2012/03/23 23:57:33, sigmund wrote:
> v8 or d8? (I've seen d8 in some status files)

d8 please.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:122: - component: either 'dart2js',
'frog', 'frogsh' (frog self-hosted)
compiler.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:239: "system: %s, option: %s" %
(system, option)
  print "compiler: %s, runtime: %s mode: %s, system: %s, option: %s" % (
    compiler, runtime, mode system, option)

See http://www.corp.google.com/eng/doc/pyguide.xml?showone=Strings#Strings

https://chromiumcodereview.appspot.com/9838068/diff/7010/samples/tests/sample...
File samples/tests/samples/samples-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/samples/tests/sample...
samples/tests/samples/samples-leg.status:6: [ $compiler == dart2js ]
On 2012/03/23 23:57:33, sigmund wrote:
> && $runtime == d8?

No.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
File tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
tests/co19/co19-leg.status:5: [ $compiler == dart2js && $runtime == d8 &&
$system == macos ]
Remove $runtime == d8

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
tests/co19/co19-leg.status:11: [ $compiler == dart2js && $runtime == d8]
ditto

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
tests/co19/co19-leg.status:27: [ $compiler == dart2js && $runtime == d8 &&
$checked ]
ditto

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
tests/co19/co19-leg.status:36: [ $component == dart2js && $runtime == d8]
$compiler and remove $runtime == d8

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/corelib/coreli...
File tests/corelib/corelib-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/corelib/coreli...
tests/corelib/corelib-leg.status:5: [ $compiler == dart2js && $runtime == d8 &&
$checked ]
Remove $runtime == d8

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/language/langu...
File tests/language/language-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/language/langu...
tests/language/language-leg.status:5: [ $compiler == dart2js && $runtime == d8
&& $checked ]
Remove $runtime == d8

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/language/langu...
tests/language/language-leg.status:235: [ $compiler == dart2js && $runtime == d8
&& $mode == release ]
Remove $runtime == d8

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/standalone/sta...
File tests/standalone/standalone-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/standalone/sta...
tests/standalone/standalone-leg.status:5: [ $compiler == dart2js && $runtime ==
d8 ]
Remove $runtime == d8

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/utils/utils.st...
File tests/utils/utils.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/utils/utils.st...
tests/utils/utils.status:5: [ $compiler == dart2js && $runtime == d8 ]
Remove $runtime == d8

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_options.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:68: none: Do not compile the Dart code (run
native Dart code on the VM).
I don't think compiler 'none' should imply running on the VM.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:77: ['none', 'frog', 'dart2js', 'dartc'],
On 2012/03/23 23:57:33, sigmund wrote:
> seems like you still need frogsh? (saw it in status files)

I don't think we run frogsh anymore on build bots. I agree with Siggi that
completely removing frogsh could be a separate CL. However, if this CL is
simpler without frogsh, perhaps it is better to remove it now.

Would it help if I prepared a CL to remove frogsh?

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:99: none: No runtime (used for dartc static
analysis tests).''',
for example.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:440: case 'none': // none = dartc static
analysis
I would prefer that none does not imply dartc.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:449: case 'd8':
It has nothing to do with d8, the problem is the compiler takes too long when
running on the debug VM or in host_checked mode.

So I think you need to look at the compiler (I would like to be able to test
dart2js with runtime == none).

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:472: * results (separated by a ','.
Missing close parens.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:142: || configuration['runtime'] == 'opera';
On 2012/03/23 23:57:33, sigmund wrote:
> (I wanted to use [contains] rather than [indexOf], but it is not in the list
> collection yet).

+1

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:430: void enqueueBrowserTest(TestInformation
info,
I had hoped this would become more general. I would like to run the compilers
without them launching a JS runtime. I think this more closely matches how we
want our compilers to behave.

But perhaps that is something to consider for a future CL.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:1077: if (executable == 'd8' || executable ==
'vm' || executable == 'dartc') {
Do we have a dartc.exe?

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:1126: configuration['runtime'] == 'drt') {
Why not just:

  if (configuration['compiler'] == 'none') {
...

[Emily Fortuna, 2012-03-26 20:52:32]

ptal.

https://chromiumcodereview.appspot.com/9838068/diff/7010/client/tests/client/...
File client/tests/client/client-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/client/tests/client/...
client/tests/client/client-leg.status:5: [ $compiler == dart2js && ($runtime ==
drt || $runtime == d8) ]
On 2012/03/24 13:56:40, ahe wrote:
> Just:
> 
> [ $compiler == dart2js ]

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/compiler/tests/dartc...
File compiler/tests/dartc/dartc.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/compiler/tests/dartc...
compiler/tests/dartc/dartc.status:20: [ $runtime == vm ]
On 2012/03/23 23:57:33, sigmund wrote:
> how about removing this, but add
>  || $compiler == none
> below? 
> 
> This would make it so we also skip dartium.

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
File frog/scripts/buildbot_annotated_steps.py (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:32: - compiler: 'dart2js', 'frog',
'frogsh', or None when the builder has an
On 2012/03/24 13:56:40, ahe wrote:
> On 2012/03/23 23:57:33, sigmund wrote:
> > - also 'dartc'?
> > - is 'none' allowed, or should we say that None is also when the builder
> doesn't
> > specify a compiler (e.g. vm/dartium)
> 
> dartc is not using this script.

Which script does dartc use? I found changes for the dart editor and the VM
elsewhere in my other CL, but nothing for dartc....

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:34: - runtime: 'v8', 'ie', 'ff',
'safari', 'chrome', 'opera'
On 2012/03/24 13:56:40, ahe wrote:
> On 2012/03/23 23:57:33, sigmund wrote:
> > v8 or d8? (I've seen d8 in some status files)
> 
> d8 please.

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:122: - component: either 'dart2js',
'frog', 'frogsh' (frog self-hosted)
On 2012/03/24 13:56:40, ahe wrote:
> compiler.

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:172: # TODO(jmesserly): make
DumpRenderTree more like other browser tests, so
On 2012/03/23 23:57:33, sigmund wrote:
> seems like this TODO is obsolete, but the translation chrome->drt should
> continue to exist?

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:239: "system: %s, option: %s" %
(system, option)
On 2012/03/24 13:56:40, ahe wrote:
>   print "compiler: %s, runtime: %s mode: %s, system: %s, option: %s" % (
>     compiler, runtime, mode system, option)
> 
> See http://www.corp.google.com/eng/doc/pyguide.xml?showone=Strings#Strings
> 

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:254: if runtime != 'd8' or (system ==
'linux' and runtime == 'chrome'):
On 2012/03/23 23:57:33, sigmund wrote:
> d8/v8

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
File tests/co19/co19-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
tests/co19/co19-leg.status:5: [ $compiler == dart2js && $runtime == d8 &&
$system == macos ]
On 2012/03/24 13:56:40, ahe wrote:
> Remove $runtime == d8

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
tests/co19/co19-leg.status:11: [ $compiler == dart2js && $runtime == d8]
On 2012/03/24 13:56:40, ahe wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
tests/co19/co19-leg.status:27: [ $compiler == dart2js && $runtime == d8 &&
$checked ]
On 2012/03/24 13:56:40, ahe wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/co19/co19-leg....
tests/co19/co19-leg.status:36: [ $component == dart2js && $runtime == d8]
On 2012/03/24 13:56:40, ahe wrote:
> $compiler and remove $runtime == d8

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/corelib/coreli...
File tests/corelib/corelib-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/corelib/coreli...
tests/corelib/corelib-leg.status:5: [ $compiler == dart2js && $runtime == d8 &&
$checked ]
On 2012/03/24 13:56:40, ahe wrote:
> Remove $runtime == d8

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/language/langu...
File tests/language/language-leg.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/language/langu...
tests/language/language-leg.status:5: [ $compiler == dart2js && $runtime == d8
&& $checked ]
On 2012/03/24 13:56:40, ahe wrote:
> Remove $runtime == d8

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/language/langu...
tests/language/language-leg.status:235: [ $compiler == dart2js && $runtime == d8
&& $mode == release ]
On 2012/03/24 13:56:40, ahe wrote:
> Remove $runtime == d8

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/utils/utils.st...
File tests/utils/utils.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/utils/utils.st...
tests/utils/utils.status:5: [ $compiler == dart2js && $runtime == d8 ]
On 2012/03/24 13:56:40, ahe wrote:
> Remove $runtime == d8

If that's the case, that means we'd be marking the test configuration formerly
known as "legium" as also failing this test. Is this intended?

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_options.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:68: none: Do not compile the Dart code (run
native Dart code on the VM).
On 2012/03/24 13:56:40, ahe wrote:
> I don't think compiler 'none' should imply running on the VM.

In what case when the compiler=none would we not be running on a dart VM?

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:77: ['none', 'frog', 'dart2js', 'dartc'],
On 2012/03/24 13:56:40, ahe wrote:
> On 2012/03/23 23:57:33, sigmund wrote:
> > seems like you still need frogsh? (saw it in status files)
> 
> I don't think we run frogsh anymore on build bots. I agree with Siggi that
> completely removing frogsh could be a separate CL. However, if this CL is
> simpler without frogsh, perhaps it is better to remove it now.
> 
> Would it help if I prepared a CL to remove frogsh?
@Peter, SGTM. In this CL I had originally removed all of frogsh, but then I saw
we had one buildbot frogsh-linux-release that seemed to be running frogsh... is
that bot just misnamed at the moment?

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:99: none: No runtime (used for dartc static
analysis tests).''',
On 2012/03/24 13:56:40, ahe wrote:
> for example.

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:428: return _expandHelper('runtime',
configuration);
On 2012/03/23 23:57:33, sigmund wrote:
> could we add a function (or a TODO) that checks that the blend of
configurations
> makes sense and issue a warning/error if they don't? For example:
> 
> -c dartc -r drt would be invalid.
> 
> I prefer to issue a warning, so that if you specify many combinations together
> in the command line, e.g.: '-c frog,dartc -r none,drt', then you see some
> warnings but all other valid tests are run:
>  warning: frog_none is not valid
>  warning: dartc_drt is not valid
>  running tests for frog_drt
>  running tests for dartc_none

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:440: case 'none': // none = dartc static
analysis
On 2012/03/24 13:56:40, ahe wrote:
> I would prefer that none does not imply dartc.

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:449: case 'd8':
On 2012/03/24 13:56:40, ahe wrote:
> It has nothing to do with d8, the problem is the compiler takes too long when
> running on the debug VM or in host_checked mode.
> 
> So I think you need to look at the compiler (I would like to be able to test
> dart2js with runtime == none).

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:472: * results (separated by a ','.
On 2012/03/24 13:56:40, ahe wrote:
> Missing close parens.

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:142: || configuration['runtime'] == 'opera';
On 2012/03/24 13:56:40, ahe wrote:
> On 2012/03/23 23:57:33, sigmund wrote:
> > (I wanted to use [contains] rather than [indexOf], but it is not in the list
> > collection yet).
> 
> +1

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_suite.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:1077: if (executable == 'd8' || executable ==
'vm' || executable == 'dartc') {
On 2012/03/24 13:56:40, ahe wrote:
> Do we have a dartc.exe?

removed.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:1126: configuration['runtime'] == 'drt') {
On 2012/03/24 13:56:40, ahe wrote:
> Why not just:
> 
>   if (configuration['compiler'] == 'none') {
> ...

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:1180: const <String>['dartium',
On 2012/03/23 23:57:33, sigmund wrote:
> dartium -> drt?

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_suite.dart:1185: 'ff'].some((x) => x == runtime);
On 2012/03/23 23:57:33, sigmund wrote:
> :)  this is basically what I had in mind for [usesWebDriver]

Done.

https://chromiumcodereview.appspot.com/9838068/diff/7010/utils/tests/css/css....
File utils/tests/css/css.status (left):

https://chromiumcodereview.appspot.com/9838068/diff/7010/utils/tests/css/css....
utils/tests/css/css.status:11: [$component == webdriver && $browser == opera]
On 2012/03/23 23:57:33, sigmund wrote:
> is opera working! :)?

Nope, not yet. :-(

[ahe, 2012-03-26 20:56:53]

A few quick comments, I'll take a closer look tomorrow.

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
File frog/scripts/buildbot_annotated_steps.py (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:32: - compiler: 'dart2js', 'frog',
'frogsh', or None when the builder has an
On 2012/03/26 20:52:32, Emily Fortuna wrote:
> On 2012/03/24 13:56:40, ahe wrote:
> > On 2012/03/23 23:57:33, sigmund wrote:
> > > - also 'dartc'?
> > > - is 'none' allowed, or should we say that None is also when the builder
> > doesn't
> > > specify a compiler (e.g. vm/dartium)
> > 
> > dartc is not using this script.
> 
> Which script does dartc use? I found changes for the dart editor and the VM
> elsewhere in my other CL, but nothing for dartc....

The VM and dartc are using a script that's checked in on the build bot. ricow@
know more.

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/utils/utils.st...
File tests/utils/utils.status (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tests/utils/utils.st...
tests/utils/utils.status:5: [ $compiler == dart2js && $runtime == d8 ]
On 2012/03/26 20:52:32, Emily Fortuna wrote:
> On 2012/03/24 13:56:40, ahe wrote:
> > Remove $runtime == d8
> 
> If that's the case, that means we'd be marking the test configuration formerly
> known as "legium" as also failing this test. Is this intended?

It's marked as skip below.

[Siggi Cherem (dart-lang), 2012-03-26 22:28:39]

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
File tools/testing/dart/test_options.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/tools/testing/dart/t...
tools/testing/dart/test_options.dart:428: return _expandHelper('runtime',
configuration);
On 2012/03/26 20:52:32, Emily Fortuna wrote:
> On 2012/03/23 23:57:33, sigmund wrote:
> > could we add a function (or a TODO) that checks that the blend of
> configurations
> > makes sense and issue a warning/error if they don't? For example:
> > 
> > -c dartc -r drt would be invalid.
> > 
> > I prefer to issue a warning, so that if you specify many combinations
together
> > in the command line, e.g.: '-c frog,dartc -r none,drt', then you see some
> > warnings but all other valid tests are run:
> >  warning: frog_none is not valid
> >  warning: dartc_drt is not valid
> >  running tests for frog_drt
> >  running tests for dartc_none
> 
> Done.

Nice, thanks!

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
File tools/testing/dart/test_options.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
tools/testing/dart/test_options.dart:78: node.js.''',
since compiler and runtime have limited combinations together, maybe we can add
to this comment (and below to the runtime comment) which are valid combinations?
For instance:

none: Do not .... 
  only valid with with the following runtimes: vm, drt

frog: Compile ...
  valid with the following runtimes: d8, drt, chrome, ...

dartc: Perform ...
  only valid with the runtime: none

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
tools/testing/dart/test_options.dart:345: * Loop through the set of
configurations that we will be testing. Return a
rewrite comment to talk about a single config? (the current comment seems
appropriate for 'filter' above where you are iterating through the configs).

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
tools/testing/dart/test_options.dart:356: 'opera',
'none']).indexOf(config['runtime']) >= 0;
I thought [none] wasn't valid here. I guess if you just want to see that it
compiles things correctly this would be ok. Is that the intention here?

[Siggi Cherem (dart-lang), 2012-03-26 22:48:04]

I meant to lgtm, a couple additional comments.

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
File tools/testing/dart/test_options.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
tools/testing/dart/test_options.dart:355: isValid = (const ['d8', 'drt', 'ff',
'chrome', 'safari', 'ie',
btw - is the current os/system available in [config] as well? (e.g. can we later
check that ie is only valid if you are in windows?)

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
tools/testing/dart/test_options.dart:356: 'opera',
'none']).indexOf(config['runtime']) >= 0;
On 2012/03/26 22:28:40, sigmund wrote:
> I thought [none] wasn't valid here. I guess if you just want to see that it
> compiles things correctly this would be ok. Is that the intention here?

BTW - One disadvantage of adding 'none' here is that we wont be able to run some
combinations together that easily. For instance, before we were able to do:

test.py -c frog,dart2js,dartc -r drt,none

to run these configurations:
frog_drt
dart2js_drt
dartc_none

With this change we will repeat work tested above by running also:
frog_none
dart2js_none

I think this is ok, but if later we don't need 'none' with dart2js anymore, I'd
remove it from here.

[Emily Fortuna, 2012-03-26 23:31:31]

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
File tools/testing/dart/test_options.dart (right):

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
tools/testing/dart/test_options.dart:345: * Loop through the set of
configurations that we will be testing. Return a
On 2012/03/26 22:28:40, sigmund wrote:
> rewrite comment to talk about a single config? (the current comment seems
> appropriate for 'filter' above where you are iterating through the configs).

Done.

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
tools/testing/dart/test_options.dart:356: 'opera',
'none']).indexOf(config['runtime']) >= 0;
On 2012/03/26 22:28:40, sigmund wrote:
> I thought [none] wasn't valid here. I guess if you just want to see that it
> compiles things correctly this would be ok. Is that the intention here?

I added this at Peter's request. Peter wanted to add "none" for ensuring that
their tests compile correctly with dart:io (I think?)

https://chromiumcodereview.appspot.com/9838068/diff/14004/tools/testing/dart/...
tools/testing/dart/test_options.dart:356: 'opera',
'none']).indexOf(config['runtime']) >= 0;
On 2012/03/26 22:48:04, sigmund wrote:
> On 2012/03/26 22:28:40, sigmund wrote:
> > I thought [none] wasn't valid here. I guess if you just want to see that it
> > compiles things correctly this would be ok. Is that the intention here?
> 
> BTW - One disadvantage of adding 'none' here is that we wont be able to run
some
> combinations together that easily. For instance, before we were able to do:
> 
> test.py -c frog,dart2js,dartc -r drt,none
> 
> to run these configurations:
> frog_drt
> dart2js_drt
> dartc_none
> 
> With this change we will repeat work tested above by running also:
> frog_none
> dart2js_none
> 
> I think this is ok, but if later we don't need 'none' with dart2js anymore,
I'd
> remove it from here.

Added a comment.

[ricow1, 2012-03-27 05:48:56]

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
File frog/scripts/buildbot_annotated_steps.py (right):

https://chromiumcodereview.appspot.com/9838068/diff/7010/frog/scripts/buildbo...
frog/scripts/buildbot_annotated_steps.py:32: - compiler: 'dart2js', 'frog',
'frogsh', or None when the builder has an
On 2012/03/26 20:56:53, ahe wrote:
> On 2012/03/26 20:52:32, Emily Fortuna wrote:
> > On 2012/03/24 13:56:40, ahe wrote:
> > > On 2012/03/23 23:57:33, sigmund wrote:
> > > > - also 'dartc'?
> > > > - is 'none' allowed, or should we say that None is also when the builder
> > > doesn't
> > > > specify a compiler (e.g. vm/dartium)
> > > 
> > > dartc is not using this script.
> > 
> > Which script does dartc use? I found changes for the dart editor and the VM
> > elsewhere in my other CL, but nothing for dartc....
> 
> The VM and dartc are using a script that's checked in on the build bot. ricow@
> know more.
So the vm and dartc compilation steps are in:
build/scripts/master/factory/dart_commands.py
in the AddCompileStep function.
This is the "normal" buildbot way of doing this when you don't have annotated
steps

[ahe, 2012-04-13 13:46:19]

Sorry, I forgot to mail this out earlier. Feel free to ignore.

http://codereview.chromium.org/9838068/diff/7010/tools/testing/dart/test_opti...
File tools/testing/dart/test_options.dart (right):

http://codereview.chromium.org/9838068/diff/7010/tools/testing/dart/test_opti...
tools/testing/dart/test_options.dart:68: none: Do not compile the Dart code (run
native Dart code on the VM).
On 2012/03/26 20:52:32, Emily Fortuna wrote:
> On 2012/03/24 13:56:40, ahe wrote:
> > I don't think compiler 'none' should imply running on the VM.
> 
> In what case when the compiler=none would we not be running on a dart VM?

Dartium for example.

I'm just saying that, compiler=none should not imply running on the VM. Running
on the VM is an example, for example:

none: Do not compile the Dart code (for example, run native Dart code on the
VM).

[Emily Fortuna, 2012-04-13 17:45:56]

http://codereview.chromium.org/9838068/diff/7010/tools/testing/dart/test_opti...
File tools/testing/dart/test_options.dart (right):

http://codereview.chromium.org/9838068/diff/7010/tools/testing/dart/test_opti...
tools/testing/dart/test_options.dart:68: none: Do not compile the Dart code (run
native Dart code on the VM).
On 2012/04/13 13:46:20, ahe wrote:
> On 2012/03/26 20:52:32, Emily Fortuna wrote:
> > On 2012/03/24 13:56:40, ahe wrote:
> > > I don't think compiler 'none' should imply running on the VM.
> > 
> > In what case when the compiler=none would we not be running on a dart VM?
> 
> Dartium for example.
> 
> I'm just saying that, compiler=none should not imply running on the VM.
Running
> on the VM is an example, for example:
> 
> none: Do not compile the Dart code (for example, run native Dart code on the
> VM).

True. This was Ivan's request to make running on the VM the default behavior...