Testrunner for 3rd parties.

Testrunner for 3rd parties.
Committed: https://code.google.com/p/dart/source/detail?r=11648

[gram, 2012-08-28 23:22:18]

From the main comment:

 * testrunner is a program to run Dart unit tests. Unlike ~/tools/test.dart,
 * this program is intended for 3rd parties to be able to run unit tests in
 * a batched fashion. As such, it adds some features and removes others. Some
 * of the removed features are:
 *
 *   - No support for test.status files. The assumption is that tests are
 *     expected to pass.
 *   - A restricted set of runtimes. The assumption here is that the Dart
 *     libraries deal with platform dependencies, and so the primary
 *     SKUs that a user of this app would be concerned with would be
 *     Dart-native versus compiled, and client (browser) vs server. To
 *     support these, four runtimes are allowed: 'drt-dart' and 'drt-js' (for
 *     client native and client-compiled, respectively), and 'vm' and 'd8'
 *     (for server-side native and compiled, respectively).
 *   - No sharding of test processes.
 *
 * On the other hand, a number of features have been added:
 *
 *   - The ability to filter tests by group or name.
 *   - The ability to run tests in isolates.
 *   - The ability to customize the format of the test result messages.
 *   - The ability to list the tests available.
 *
 * By default, testrunner will run all tests in the current directory.
 * With a -R option, it will recurse into subdirectories.
 * Directories can also be specified on the command line; if
 * any are specified they will override the use of the current directory.
 * All files that match the --test-file-pat will be included; by default
 * this is files with names that end in _test.dart.
 *
 * Options can be specified on the command line, via a configuration
 * file (--config) or via a test.config file in the test directory,
 * in decreasing order of priority.
 *
 * TODO(gram) - Layout tests. The plan here will be to look for a file
 * with a .layout extension that corresponds to the .dart file, that contains
 * multiple layouts, one for each test. Each test will be run in its own
 * instance of DRT and and the result compared with the expected layout.

[gram, 2012-08-29 00:37:29]

Sorry, made a few changes - I'll stop tweaking it now. The changes were to have
separate format strings for list/pass/error/fail, and to allow for immediate
output of test results, as requested in issue 4667.

[Siggi Cherem (dart-lang), 2012-08-29 01:05:54]

https://chromiumcodereview.appspot.com/10897016/diff/3001/pkg/unittest/test/m...
File pkg/unittest/test/mock_test.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/pkg/unittest/test/m...
pkg/unittest/test/mock_test.dart:6: #import('../unittest.dart');
I rather have you keep it like you had it, so that later, when we have package:
imports available here, we can simply use it here too.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:5: class DartWrapTask extends PipelineTask
{
docs?

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:125: String resultName(result) {
the mix of quoted code and normal code makes this really hard to read, can we
define these as constants at the end of the file, and use straight line code
here?

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/de...
File utils/testrunner/delete_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/de...
utils/testrunner/delete_task.dart:13: var fname = concretize(filenameTemplate,
testfile);
it seems that in some of the tasks the macros are not helping that much. For
instance, you could directly delete the file under the path, rather than
reconstructing the path from the filename.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/de...
utils/testrunner/delete_task.dart:15: f.deleteSync();
given that this is already an async task, you could use the async API here too
:)

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
File utils/testrunner/pipeline_runner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
utils/testrunner/pipeline_runner.dart:5: // A [PipelineRunner] represents the
execution of our pipeline for one test
see general style comments in testrunner.dart (e.g. use /** */ here and below,
add empty line before each comment on the fields below, etc)

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
utils/testrunner/pipeline_runner.dart:48: ++_stageNum;
style nit: prefer postfix ++

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
File utils/testrunner/pipeline_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
utils/testrunner/pipeline_task.dart:14: String concretize(String template, Path
testfile) {
rename ideas: concretize =>
 - instantiate
 - substituteTestInfo
 - expand/expandTestInfo/expandMacro/expandMetadata

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
File utils/testrunner/testrunner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:7: * testrunner is a program to run Dart uni
tests. Unlike ~/tools/test.dart,
typos:
- uni => unit
- ~/tools/test.dart => $DART/tools/test.dart

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:13: *     expected to pass.
a nice future feature would be to be able to indicate that some test cases are
expected to fail temporarily (e.g. instead of a status file that marked entire
test suites as pass/fail, be able to specify them in the way you specify filters
today)

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:18: *     support these, four runtimes are
allowed: 'drt-dart' and 'drt-js' (for
I think it would be nice to include in this documentation how to get drt
installed on your machine and make it possible for this runner to find it.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:20: *     (for server-side native and compiled,
respectively).
mmm... I wonder if we even want to include d8

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:34: * All files that match the --test-file-pat
will be included; by default
let's rename 'pat' to 'pattern'

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:38: * file (--config) or via a test.config file
in the test directory,
in these comments let's use code quotes around `--flags` so they show up nicely
in dartdoc.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:41: * TODO(gram) - Layout tests. The plan here
will be to look for a file
pull TODO's out of dartdocs

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:66: // The maximum number of pipelines that can
run concurrently.
general style for this file and the rest of the CL:
 - these are already documenting a field/method, let's use dartdoc comments /**
*/ for them
 - put empty line before any comment line

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:112: var mustCleanupJavascript = false;
I feel we can easily make these lines a bit more generic if we keep around a
list of cleanupTasks and append it at the end.

Basically:
- each task will also include some cleanup tasks
- we append them to a list of cleanup tasks where we currently set these flags
to true.
- then at the end we simply add all the cleanup tasks to the pipeline

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:116: var tempDartFile =
'$tempDirPath$pathSep${Meta.filenameNoExtension}.dart';
will this be problematic when using keep-files? basically 2 files with the same
name in different directories will conflict with each other.

I believe that in test.dart they did some canonicalization (either by creating a
parallel tree or by replacing / with _ and _ with __).

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:119: var tempCSSFile =
'$tempDirPath$pathSep${Meta.filenameNoExtension}.css';
there is a lot of redundancy in these last 4 lines, maybe:
var basename = '$tempDirPath$pathSep${Meta.filenameNoExtension}';
var tempDartFile = '$basename.dart';
var tempJsFile = '$basename.dart';
...

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:294: (f)=>processTests(pipelineTemplate, f));
nit: add spaces around '=>'

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/me...
File utils/testrunner/meta.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/me...
utils/testrunner/meta.dart:5: class Meta {
nits:
- please add docs
- rename to something more specific, e.g. CommandArguments?

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/op...
File utils/testrunner/options.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/op...
utils/testrunner/options.dart:217: bool sane(ArgResults config) {
rename: sane => isSane, or isSaneConfig

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/ut...
File utils/testrunner/utils.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/ut...
utils/testrunner/utils.dart:47: // Create the list of all the test files that we
are going to
nit: make into dartdoc (probably needs to be moved below the dirCount field).
Given that this is pretty general, it seems you can describe this without
mentioning 'tests'.

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/ut...
utils/testrunner/utils.dart:51: // dircount hackiness?
another way is to use Futures.wait:


var futures = [];
for (...) {
  ...
  var d = ...;
  if (d.existsSync()) {
    var completer = new Completer();
    futures.add(completer.future);
    var lister = d.list(...);
    ...
    lister.onDone = (c) => completer.complete(c);
  }
}

Futures.wait(futures).then((_) => onComplete(files));

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/ut...
utils/testrunner/utils.dart:52: int dirCount;
if you continue with dirCount, I think you can make it a local var inside
buildFileList.

https://chromiumcodereview.appspot.com/10897016/diff/4005/utils/testrunner/op...
File utils/testrunner/options.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/4005/utils/testrunner/op...
utils/testrunner/options.dart:213: return optionsParser.parse(options);
it might be worth converting the resulting ArgResults into an actual
configuration object. So instead of:
  config['immediate']
you can do:
  config.immediate

[gram, 2012-08-29 20:12:26]

PTAL

https://chromiumcodereview.appspot.com/10897016/diff/3001/pkg/unittest/test/m...
File pkg/unittest/test/mock_test.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/pkg/unittest/test/m...
pkg/unittest/test/mock_test.dart:6: #import('../unittest.dart');
On 2012/08/29 01:05:54, sigmund wrote:
> I rather have you keep it like you had it, so that later, when we have
package:
> imports available here, we can simply use it here too.

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:5: class DartWrapTask extends PipelineTask
{
On 2012/08/29 01:05:54, sigmund wrote:
> docs?

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:125: String resultName(result) {
On 2012/08/29 01:05:54, sigmund wrote:
> the mix of quoted code and normal code makes this really hard to read, can we
> define these as constants at the end of the file, and use straight line code
> here?

I've refactored this quite a bit; hopefully it will be more clear.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/de...
File utils/testrunner/delete_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/de...
utils/testrunner/delete_task.dart:13: var fname = concretize(filenameTemplate,
testfile);
On 2012/08/29 01:05:55, sigmund wrote:
> it seems that in some of the tasks the macros are not helping that much. For
> instance, you could directly delete the file under the path, rather than
> reconstructing the path from the filename. 

No, the macro is important. The concrete testfile here is the original test file
(e.g. pkg/unittest/tests/matchers_test.dart). The macro will expand to various
related transient files, e.g. /tmp/testrunner/matchers_test.css.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/de...
utils/testrunner/delete_task.dart:15: f.deleteSync();
On 2012/08/29 01:05:55, sigmund wrote:
> given that this is already an async task, you could use the async API here too
> :)

Yes, but my expectation is that the underlying OS functions for file deletion
(like unlink()) are sync, and thus making file deletion async is unnatural and
just Dart weirdness, that I see no reason to perpetuate :-)

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
File utils/testrunner/pipeline_runner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
utils/testrunner/pipeline_runner.dart:5: // A [PipelineRunner] represents the
execution of our pipeline for one test
On 2012/08/29 01:05:55, sigmund wrote:
> see general style comments in testrunner.dart (e.g. use /** */ here and below,
> add empty line before each comment on the fields below, etc)

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
utils/testrunner/pipeline_runner.dart:48: ++_stageNum;
On 2012/08/29 01:05:55, sigmund wrote:
> style nit: prefer postfix ++

Is there a good reason for this? Maybe I'm just showing my age, but back in the
Jurassic period, postinc/dec was considered inferior unless actually needed as
it could result in the creation of unnecessary temporaries, while a prefix
inc/dec could typically be translated into a single machine INC/DEC instruction.

I don't mind changing it; I'm just wondering why there would be a preference for
postfix.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
File utils/testrunner/pipeline_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/pi...
utils/testrunner/pipeline_task.dart:14: String concretize(String template, Path
testfile) {
On 2012/08/29 01:05:55, sigmund wrote:
> rename ideas: concretize =>
>  - instantiate
>  - substituteTestInfo
>  - expand/expandTestInfo/expandMacro/expandMetadata

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
File utils/testrunner/testrunner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:7: * testrunner is a program to run Dart uni
tests. Unlike ~/tools/test.dart,
On 2012/08/29 01:05:55, sigmund wrote:
> typos:
> - uni => unit
> - ~/tools/test.dart => $DART/tools/test.dart

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:13: *     expected to pass.
On 2012/08/29 01:05:55, sigmund wrote:
> a nice future feature would be to be able to indicate that some test cases are
> expected to fail temporarily (e.g. instead of a status file that marked entire
> test suites as pass/fail, be able to specify them in the way you specify
filters
> today)

I added a TODO.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:18: *     support these, four runtimes are
allowed: 'drt-dart' and 'drt-js' (for
On 2012/08/29 01:05:55, sigmund wrote:
> I think it would be nice to include in this documentation how to get drt
> installed on your machine and make it possible for this runner to find it.

DRT is in the Dart SDK. Should I just refer to that?
I have added the option to specify the Dart SDK path and compute the other paths
from that.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:20: *     (for server-side native and compiled,
respectively).
On 2012/08/29 01:05:55, sigmund wrote:
> mmm... I wonder if we even want to include d8

I agree - I've removed it.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:34: * All files that match the --test-file-pat
will be included; by default
On 2012/08/29 01:05:55, sigmund wrote:
> let's rename 'pat' to 'pattern'

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:38: * file (--config) or via a test.config file
in the test directory,
On 2012/08/29 01:05:55, sigmund wrote:
> in these comments let's use code quotes around `--flags` so they show up
nicely
> in dartdoc.

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:41: * TODO(gram) - Layout tests. The plan here
will be to look for a file
On 2012/08/29 01:05:55, sigmund wrote:
> pull TODO's out of dartdocs

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:66: // The maximum number of pipelines that can
run concurrently.
On 2012/08/29 01:05:55, sigmund wrote:
> general style for this file and the rest of the CL:
>  - these are already documenting a field/method, let's use dartdoc comments
/**
> */ for them
>  - put empty line before any comment line

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:112: var mustCleanupJavascript = false;
On 2012/08/29 01:05:55, sigmund wrote:
> I feel we can easily make these lines a bit more generic if we keep around a
> list of cleanupTasks and append it at the end.
> 
> Basically:
> - each task will also include some cleanup tasks
> - we append them to a list of cleanup tasks where we currently set these flags
> to true.
> - then at the end we simply add all the cleanup tasks to the pipeline

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:116: var tempDartFile =
'$tempDirPath$pathSep${Meta.filenameNoExtension}.dart';
On 2012/08/29 01:05:55, sigmund wrote:
> will this be problematic when using keep-files? basically 2 files with the
same
> name in different directories will conflict with each other.
> 
> I believe that in test.dart they did some canonicalization (either by creating
a
> parallel tree or by replacing / with _ and _ with __).

They created a parallel tree, and I didn't like that approach. Canonicalization
is better. I've done that.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:119: var tempCSSFile =
'$tempDirPath$pathSep${Meta.filenameNoExtension}.css';
On 2012/08/29 01:05:55, sigmund wrote:
> there is a lot of redundancy in these last 4 lines, maybe:
> var basename = '$tempDirPath$pathSep${Meta.filenameNoExtension}';
> var tempDartFile = '$basename.dart';
> var tempJsFile = '$basename.dart';
> ...

Done.

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:294: (f)=>processTests(pipelineTemplate, f));
On 2012/08/29 01:05:55, sigmund wrote:
> nit: add spaces around '=>'

Done.

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/me...
File utils/testrunner/meta.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/me...
utils/testrunner/meta.dart:5: class Meta {
On 2012/08/29 01:05:55, sigmund wrote:
> nits:
> - please add docs
> - rename to something more specific, e.g. CommandArguments?

Done.

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/op...
File utils/testrunner/options.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/op...
utils/testrunner/options.dart:217: bool sane(ArgResults config) {
On 2012/08/29 01:05:55, sigmund wrote:
> rename: sane => isSane, or isSaneConfig

Done.

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/ut...
File utils/testrunner/utils.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/ut...
utils/testrunner/utils.dart:47: // Create the list of all the test files that we
are going to
On 2012/08/29 01:05:55, sigmund wrote:
> nit: make into dartdoc (probably needs to be moved below the dirCount field).
> Given that this is pretty general, it seems you can describe this without
> mentioning 'tests'.

Done.

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/ut...
utils/testrunner/utils.dart:51: // dircount hackiness?
On 2012/08/29 01:05:55, sigmund wrote:
> another way is to use Futures.wait:
> 
> 
> var futures = [];
> for (...) {
>   ...
>   var d = ...;
>   if (d.existsSync()) {
>     var completer = new Completer();
>     futures.add(completer.future);
>     var lister = d.list(...);
>     ...
>     lister.onDone = (c) => completer.complete(c);
>   }
> }
> 
> Futures.wait(futures).then((_) => onComplete(files));

Interesting.

https://chromiumcodereview.appspot.com/10897016/diff/8001/utils/testrunner/ut...
utils/testrunner/utils.dart:52: int dirCount;
On 2012/08/29 01:05:55, sigmund wrote:
> if you continue with dirCount, I think you can make it a local var inside
> buildFileList.

Done.

https://chromiumcodereview.appspot.com/10897016/diff/4005/utils/testrunner/op...
File utils/testrunner/options.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/4005/utils/testrunner/op...
utils/testrunner/options.dart:213: return optionsParser.parse(options);
On 2012/08/29 01:05:55, sigmund wrote:
> it might be worth converting the resulting ArgResults into an actual
> configuration object. So instead of:
>   config['immediate']
> you can do:
>   config.immediate

Done.

[Siggi Cherem (dart-lang), 2012-08-29 20:47:35]

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:125: String resultName(result) {
On 2012/08/29 20:12:26, gram wrote:
> On 2012/08/29 01:05:54, sigmund wrote:
> > the mix of quoted code and normal code makes this really hard to read, can
we
> > define these as constants at the end of the file, and use straight line code
> > here?
> 
> I've refactored this quite a bit; hopefully it will be more clear.
> 

it is easier to see with syntax highlighting, but very hard to read in rietveld
:(

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
File utils/testrunner/testrunner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/3001/utils/testrunner/te...
utils/testrunner/testrunner.dart:18: *     support these, four runtimes are
allowed: 'drt-dart' and 'drt-js' (for
On 2012/08/29 20:12:26, gram wrote:
> On 2012/08/29 01:05:55, sigmund wrote:
> > I think it would be nice to include in this documentation how to get drt
> > installed on your machine and make it possible for this runner to find it.
> 
> DRT is in the Dart SDK. Should I just refer to that?
> I have added the option to specify the Dart SDK path and compute the other
paths
> from that.
> 
> 

I'm not sure - At some point drt was in the sdk of some distributions (e.g. in
mac, but not in linux) :(

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/c...
File utils/testrunner/configuration.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/c...
utils/testrunner/configuration.dart:5: class Configuration {
+dartdoc

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/d...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/d...
utils/testrunner/dart_wrap_task.dart:5: // The DartWrapTask generates a Dart
wrapper for a test file, that has a
use /** */ so we make all these comments valid dartdocs.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/d...
utils/testrunner/dart_wrap_task.dart:16: var sourceName =
makePathAbsolute(expandMacros(sourceFileTemplate, testfile));
80 col

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/m...
File utils/testrunner/macros.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/m...
utils/testrunner/macros.dart:12: // A verson of the directory that is flattened
to be like a filename,
use /** */, add empty line before comment

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/m...
utils/testrunner/macros.dart:17: // Test result message components. Thesde can
be used to specify a
Thesde => These

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/p...
File utils/testrunner/pipeline_runner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/p...
utils/testrunner/pipeline_runner.dart:10: // The path of the test file.
use /** */ here too...

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/t...
File utils/testrunner/testrunner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/t...
utils/testrunner/testrunner.dart:41: * The four runtimes are:
four => 3

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/t...
utils/testrunner/testrunner.dart:54: * TODO(gram) - for TDD, add the ability to
indicate that some test cases are
Unless you want them in the documentation, move these TODO's out of the dartdoc
comment /** */, into a normal // comment.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/t...
utils/testrunner/testrunner.dart:160: var HTMLFile =
'${Macros.directory}$pathSep${Macros.filenameNoExtension}.html';
80 col...

[Siggi Cherem (dart-lang), 2012-08-29 20:51:13]

here is the link to where to download drt from:
http://gsdview.appspot.com/dartium-archive/continuous/

[gram, 2012-08-30 00:16:55]

PTAL.

This includes moving the cleanup code to the tasks, and fixing the use of
--isolate when running in DRT.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/c...
File utils/testrunner/configuration.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/c...
utils/testrunner/configuration.dart:5: class Configuration {
On 2012/08/29 20:47:35, sigmund wrote:
> +dartdoc

Done.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/m...
File utils/testrunner/macros.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/m...
utils/testrunner/macros.dart:12: // A verson of the directory that is flattened
to be like a filename,
On 2012/08/29 20:47:35, sigmund wrote:
> use /** */, add empty line before comment

Done.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/m...
utils/testrunner/macros.dart:17: // Test result message components. Thesde can
be used to specify a
On 2012/08/29 20:47:35, sigmund wrote:
> Thesde => These

Done.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/p...
File utils/testrunner/pipeline_runner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/p...
utils/testrunner/pipeline_runner.dart:10: // The path of the test file.
On 2012/08/29 20:47:35, sigmund wrote:
> use /** */ here too... 

Done.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/t...
File utils/testrunner/testrunner.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/t...
utils/testrunner/testrunner.dart:41: * The four runtimes are:
On 2012/08/29 20:47:35, sigmund wrote:
> four => 3

Done.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/t...
utils/testrunner/testrunner.dart:54: * TODO(gram) - for TDD, add the ability to
indicate that some test cases are
On 2012/08/29 20:47:35, sigmund wrote:
> Unless you want them in the documentation, move these TODO's out of the
dartdoc
> comment /** */, into a normal // comment.

They are /*, not /**

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/t...
utils/testrunner/testrunner.dart:160: var HTMLFile =
'${Macros.directory}$pathSep${Macros.filenameNoExtension}.html';
On 2012/08/29 20:47:35, sigmund wrote:
> 80 col...

Done.

[Adam, 2012-08-30 06:53:07]

https://chromiumcodereview.appspot.com/10897016/diff/2034/pkg/unittest/unitte...
File pkg/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/pkg/unittest/unitte...
pkg/unittest/unittest.dart:159: Configuration get config() => _config;
Use new get syntax

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:8: String sourceFileTemplate;
Should have a final keyword?

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/de...
File utils/testrunner/delete_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/de...
utils/testrunner/delete_task.dart:7: String filenameTemplate;
final keyword?

[gram, 2012-08-30 16:28:50]

Made task members private and final, and added download link for DRT.

https://chromiumcodereview.appspot.com/10897016/diff/2034/pkg/unittest/unitte...
File pkg/unittest/unittest.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/pkg/unittest/unitte...
pkg/unittest/unittest.dart:159: Configuration get config() => _config;
On 2012/08/30 06:53:07, Adam wrote:
> Use new get syntax

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:8: String sourceFileTemplate;
On 2012/08/30 06:53:07, Adam wrote:
> Should have a final keyword?

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/de...
File utils/testrunner/delete_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/de...
utils/testrunner/delete_task.dart:7: String filenameTemplate;
On 2012/08/30 06:53:07, Adam wrote:
> final keyword?

Done.

[Siggi Cherem (dart-lang), 2012-08-30 16:46:40]

I'm very excited about this Graham, I can't wait to use it :) 

LGTM with the fixes for minor nits and the isolate communication part.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/d...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/d...
utils/testrunner/dart_wrap_task.dart:5: // The DartWrapTask generates a Dart
wrapper for a test file, that has a
On 2012/08/29 20:47:35, sigmund wrote:
> use /** */ so we make all these comments valid dartdocs.

note that this and the comment below are still pending...

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
File utils/testrunner/dart2js_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart2js_task.dart:24: verboseLogging, keepTestFiles, stdout);
nits:
- throughout this file, no need to put space next to [ and ], e.g.:
[jsFileTemplate], ['-out...', dartFile]
- verboseLogging => logging, keepTestFiles => keepFiles (some of these calls
might fit in 1 line :))

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
File utils/testrunner/dart_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_task.dart:17: [ dartFileTemplate ],
same here and throughout the cl, no spaces, I believe this would fit in a single
line.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:65: sbuf.add(stubPrintResultsFunction);
these 2 function names are extremely similar, it took me a while to realize one
was singular and the other plural. How about:
- printResultFunction => printTestResultFunction, (or printStep?)
- printResultsFunction => printAllTestsResultsFunction

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:97: deleteFiles([ tempDartFileTemplate ],
testfile,
ditto:
- space removal: [ ___ ] => [___]
- shorter vars

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:133: }
add empty line

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:154:
${filter?'unittest.filterTests(filterTest);':''}
spaces around ? and :

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:374:
slaveport.send('\${results[0].result}\\n'
no need to do this sophisticated encoding in a string, you can send arrays
between isolates:

TestCase test = results[0];
slaveport.send([test.result, test.runningTime.inMilliseconds, test.message,
test.stackTrace]);

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:376:
'\${results[0].message}\\n###\\n\${results[0].stackTrace}');
why only results[0], shouldn't onDone send every test result?

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:383: slaveport = sendport;
slaveport => masterPort?

(if this is the 'slave', then [sendport] is the port to the master.)

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:400: SendPort sport =
spawnFunction(runSlaveTest);
sport => slavePort

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:408: while (lines[idx] != '###') {
see comment above, you shouldn't need to do all this encoding/decoding by hand.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/dr...
File utils/testrunner/drt_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/dr...
utils/testrunner/drt_task.dart:10: [  '--no-timeout', htmlFileTemplate ],
as before...

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/ht...
File utils/testrunner/html_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/ht...
utils/testrunner/html_wrap_task.dart:134: deleteFiles([ htmlDestFileTemplate,
cssDestFileTemplate ], testfile,
ditto

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/pi...
File utils/testrunner/pipeline_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/pi...
utils/testrunner/pipeline_task.dart:15: bool verboseLogging, bool keepTestFiles)
{
ditto here and below (shorter names)

https://chromiumcodereview.appspot.com/10897016/diff/19001/utils/testrunner/d...
File utils/testrunner/delete_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/19001/utils/testrunner/d...
utils/testrunner/delete_task.dart:9: DeleteTask(filenameTemplate) :
_filenameTemplate = filenameTemplate;
my understanding is that it is valid to say:
DeleteTask(this._filenameTemplate)

[gram, 2012-08-30 17:34:04]

PTAL at the very last issue. If I am wrong let me know and I'll change that
before the submit.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/d...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/d...
utils/testrunner/dart_wrap_task.dart:5: // The DartWrapTask generates a Dart
wrapper for a test file, that has a
On 2012/08/29 20:47:35, sigmund wrote:
> use /** */ so we make all these comments valid dartdocs.

Done.

https://chromiumcodereview.appspot.com/10897016/diff/13004/utils/testrunner/d...
utils/testrunner/dart_wrap_task.dart:16: var sourceName =
makePathAbsolute(expandMacros(sourceFileTemplate, testfile));
On 2012/08/29 20:47:35, sigmund wrote:
> 80 col

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
File utils/testrunner/dart2js_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart2js_task.dart:24: verboseLogging, keepTestFiles, stdout);
On 2012/08/30 16:46:40, sigmund wrote:
> nits:
> - throughout this file, no need to put space next to [ and ], e.g.:
> [jsFileTemplate], ['-out...', dartFile]
> - verboseLogging => logging, keepTestFiles => keepFiles (some of these calls
> might fit in 1 line :))

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
File utils/testrunner/dart_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_task.dart:17: [ dartFileTemplate ],
On 2012/08/30 16:46:40, sigmund wrote:
> same here and throughout the cl, no spaces, I believe this would fit in a
single
> line.

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
File utils/testrunner/dart_wrap_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:65: sbuf.add(stubPrintResultsFunction);
On 2012/08/30 16:46:40, sigmund wrote:
> these 2 function names are extremely similar, it took me a while to realize
one
> was singular and the other plural. How about:
> - printResultFunction => printTestResultFunction, (or printStep?)
> - printResultsFunction => printAllTestsResultsFunction
> 

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:97: deleteFiles([ tempDartFileTemplate ],
testfile,
On 2012/08/30 16:46:40, sigmund wrote:
> ditto:
> - space removal: [ ___ ] => [___]
> - shorter vars

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:133: }
On 2012/08/30 16:46:40, sigmund wrote:
> add empty line

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:154:
${filter?'unittest.filterTests(filterTest);':''}
On 2012/08/30 16:46:40, sigmund wrote:
> spaces around ? and :

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:374:
slaveport.send('\${results[0].result}\\n'
On 2012/08/30 16:46:40, sigmund wrote:
> no need to do this sophisticated encoding in a string, you can send arrays
> between isolates:
> 
> TestCase test = results[0];
> slaveport.send([test.result, test.runningTime.inMilliseconds, test.message,
> test.stackTrace]);

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:376:
'\${results[0].message}\\n###\\n\${results[0].stackTrace}');
On 2012/08/30 16:46:40, sigmund wrote:
> why only results[0], shouldn't onDone send every test result?

No - because we run each test in its own isolate, there is only one test here.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:383: slaveport = sendport;
On 2012/08/30 16:46:40, sigmund wrote:
> slaveport => masterPort?
> 
> (if this is the 'slave', then [sendport] is the port to the master.)

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:400: SendPort sport =
spawnFunction(runSlaveTest);
On 2012/08/30 16:46:40, sigmund wrote:
> sport => slavePort

Done.

https://chromiumcodereview.appspot.com/10897016/diff/2034/utils/testrunner/da...
utils/testrunner/dart_wrap_task.dart:408: while (lines[idx] != '###') {
On 2012/08/30 16:46:40, sigmund wrote:
> see comment above, you shouldn't need to do all this encoding/decoding by
hand. 

Done.

https://chromiumcodereview.appspot.com/10897016/diff/19001/utils/testrunner/d...
File utils/testrunner/delete_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/19001/utils/testrunner/d...
utils/testrunner/delete_task.dart:9: DeleteTask(filenameTemplate) :
_filenameTemplate = filenameTemplate;
On 2012/08/30 16:46:40, sigmund wrote:
> my understanding is that it is valid to say:
> DeleteTask(this._filenameTemplate)

It was decided some time back that private members should not be used in
constructors as they should not be exposed in Dart docs, and so the pattern I
use is the recommended way. At least this is true for named parameters; I'm not
100% sure about normal parameters but it seems the issue would still apply.

[Siggi Cherem (dart-lang), 2012-08-30 17:55:37]

lgtm!

https://chromiumcodereview.appspot.com/10897016/diff/19001/utils/testrunner/d...
File utils/testrunner/delete_task.dart (right):

https://chromiumcodereview.appspot.com/10897016/diff/19001/utils/testrunner/d...
utils/testrunner/delete_task.dart:9: DeleteTask(filenameTemplate) :
_filenameTemplate = filenameTemplate;
On 2012/08/30 17:34:05, gram wrote:
> On 2012/08/30 16:46:40, sigmund wrote:
> > my understanding is that it is valid to say:
> > DeleteTask(this._filenameTemplate)
> 
> It was decided some time back that private members should not be used in
> constructors as they should not be exposed in Dart docs, and so the pattern I
> use is the recommended way. At least this is true for named parameters; I'm
not
> 100% sure about normal parameters but it seems the issue would still apply.
> 

good question - after talking with Bob, it seems that we do allow them and
currently prefer it.