Add temporary directory for dartc compilation of tests.

Add temporary directory for dartc compilation of tests.

This subdirectory of the system temp directory is deleted after tests are run.
This CL also unifies finding of the top-level dart checkout directory.


BUG=dart:1167
TEST=tools/test.dart -cdartc,chromium

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

[Mads Ager (chromium), 2012-01-18 06:57:16]

lgtm

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:426: [bool verbose = false,
We should go back to using this.verbose, this.listTests, and
this.keepGeneratedTests here instead of the initilizers below. This is left-over
from a temporary change where that syntax was not allowed in named optional
arguments.

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:438: if (new Platform().operatingSystem() !=
'windows') {
Can we filter out better when we create the temporary directory? For instance do
it in addTestSuite if we are adding a test suite that needs a temporary
directory? Then that would be the place to add an assert too that we are not
running on Windows?

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:474: new Process.start('/bin/rm', ['-rf',
_temporaryDirectory]);
Lets add a TODO that this should be replaced by
_temporaryDirectory.delete(recursive: true) once that has been implemented? Then
Windows would be supported as well.

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:478: deletion.close();  // Detach process -
don't wait for it to finish.
I'm not sure we should do that. It will allow the script to return faster but it
will make it harder to debug if something is wrong. Maybe we should instead add
an errorHandler to the process and print out any errors encountered?

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_suit...
File tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_suit...
tools/testing/dart/test_suite.dart:510: Expect.isTrue(new
Platform().operatingSystem() != 'windows',
I would move this assert to where we actually create the temp directory? Then we
can also get away with only creating the temp directory if we add a test suite
with one of these components.

[Mads Ager (chromium), 2012-01-18 06:58:33]

The 'lgtm' was pressing the wrong button. I'd like to have a quick second look.

[Bill Hesse, 2012-01-18 11:52:38]

It is really difficult to decide, in ProcessQueue, whether the directory will be
needed, and it is not very expensive to create and remove it if we don't need
it.  It depends on the components, not the test suites, and we don't have that
information.

http://codereview.chromium.org/9240011/diff/3001/tools/test.dart
File tools/test.dart (right):

http://codereview.chromium.org/9240011/diff/3001/tools/test.dart#newcode111
tools/test.dart:111: 
removed.

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:426: [bool verbose = false,
On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> We should go back to using this.verbose, this.listTests, and
> this.keepGeneratedTests here instead of the initilizers below. This is
left-over
> from a temporary change where that syntax was not allowed in named optional
> arguments.

Done.

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:438: if (new Platform().operatingSystem() !=
'windows') {
It is the components we run, not the test suites, that determine if we need the
temporary directory.  Almost all the test suites (the standard suites) need the
temp directory for component chromium.  For component dartc only some of the
suites need it.  Information about all the configurations is not passed in to
the processQueue constructor, and we would need to know if any configuration
used a relevant component.  Creating the directory is really cheap.
On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> Can we filter out better when we create the temporary directory? For instance
do
> it in addTestSuite if we are adding a test suite that needs a temporary
> directory? Then that would be the place to add an assert too that we are not
> running on Windows?

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:474: new Process.start('/bin/rm', ['-rf',
_temporaryDirectory]);
On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> Lets add a TODO that this should be replaced by
> _temporaryDirectory.delete(recursive: true) once that has been implemented?
Then
> Windows would be supported as well. 

Done.

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:478: deletion.close();  // Detach process -
don't wait for it to finish.
The deletion.close() is redundant, since _progress.allDone() calls
system.exit().  We would have to add waiting for all possible exits of the
deletion command to _progress.allDone().  We don't want to wait for this to
complete, it causes no error if it does not complete correctly, except filling
the temp directory, and shouldn't need debugging.  So the line is removed, but
the behavior will still be the same.

On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> I'm not sure we should do that. It will allow the script to return faster but
it
> will make it harder to debug if something is wrong. Maybe we should instead
add
> an errorHandler to the process and print out any errors encountered?

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_suit...
File tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_suit...
tools/testing/dart/test_suite.dart:510: Expect.isTrue(new
Platform().operatingSystem() != 'windows',
We can drop the assert, or change it to Expect.isTrue(globalTemporaryDirectory
!= null).

The assert != null is not as informative, unless we leave the message in.

On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> I would move this assert to where we actually create the temp directory? Then
we
> can also get away with only creating the temp directory if we add a test suite
> with one of these components.

[Mads Ager (chromium), 2012-01-18 12:23:14]

As discussed offline, it should be easy to make the creation of the global temp
dir lazy. Pass in a function to the places that might need it instead of passing
in the path to the already created temp dir. That would get rid of my concerns.
:)

On 2012/01/18 11:52:38, Bill Hesse wrote:
> It is really difficult to decide, in ProcessQueue, whether the directory will
be
> needed, and it is not very expensive to create and remove it if we don't need
> it.  It depends on the components, not the test suites, and we don't have that
> information.
> 
> http://codereview.chromium.org/9240011/diff/3001/tools/test.dart
> File tools/test.dart (right):
> 
> http://codereview.chromium.org/9240011/diff/3001/tools/test.dart#newcode111
> tools/test.dart:111: 
> removed.
> 
>
http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
> File tools/testing/dart/test_runner.dart (right):
> 
>
http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
> tools/testing/dart/test_runner.dart:426: [bool verbose = false,
> On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> > We should go back to using this.verbose, this.listTests, and
> > this.keepGeneratedTests here instead of the initilizers below. This is
> left-over
> > from a temporary change where that syntax was not allowed in named optional
> > arguments.
> 
> Done.
> 
>
http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
> tools/testing/dart/test_runner.dart:438: if (new Platform().operatingSystem()
!=
> 'windows') {
> It is the components we run, not the test suites, that determine if we need
the
> temporary directory.  Almost all the test suites (the standard suites) need
the
> temp directory for component chromium.  For component dartc only some of the
> suites need it.  Information about all the configurations is not passed in to
> the processQueue constructor, and we would need to know if any configuration
> used a relevant component.  Creating the directory is really cheap.
> On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> > Can we filter out better when we create the temporary directory? For
instance
> do
> > it in addTestSuite if we are adding a test suite that needs a temporary
> > directory? Then that would be the place to add an assert too that we are not
> > running on Windows?
> 
>
http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
> tools/testing/dart/test_runner.dart:474: new Process.start('/bin/rm', ['-rf',
> _temporaryDirectory]);
> On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> > Lets add a TODO that this should be replaced by
> > _temporaryDirectory.delete(recursive: true) once that has been implemented?
> Then
> > Windows would be supported as well. 
> 
> Done.
> 
>
http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_runn...
> tools/testing/dart/test_runner.dart:478: deletion.close();  // Detach process
-
> don't wait for it to finish.
> The deletion.close() is redundant, since _progress.allDone() calls
> system.exit().  We would have to add waiting for all possible exits of the
> deletion command to _progress.allDone().  We don't want to wait for this to
> complete, it causes no error if it does not complete correctly, except filling
> the temp directory, and shouldn't need debugging.  So the line is removed, but
> the behavior will still be the same.
> 
> On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> > I'm not sure we should do that. It will allow the script to return faster
but
> it
> > will make it harder to debug if something is wrong. Maybe we should instead
> add
> > an errorHandler to the process and print out any errors encountered?
> 
>
http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_suit...
> File tools/testing/dart/test_suite.dart (right):
> 
>
http://codereview.chromium.org/9240011/diff/3001/tools/testing/dart/test_suit...
> tools/testing/dart/test_suite.dart:510: Expect.isTrue(new
> Platform().operatingSystem() != 'windows',
> We can drop the assert, or change it to Expect.isTrue(globalTemporaryDirectory
> != null).
> 
> The assert != null is not as informative, unless we leave the message in.
> 
> On 2012/01/18 06:57:17, Mads Sig Ager wrote:
> > I would move this assert to where we actually create the temp directory?
Then
> we
> > can also get away with only creating the temp directory if we add a test
suite
> > with one of these components.

[Bill Hesse, 2012-01-18 15:03:38]

Please look at differences between #2 and #3.

[Mads Ager (google), 2012-01-18 15:19:58]

LGTM with a couple of minor comments.

[Mads Ager (google), 2012-01-18 15:26:29]

You already addressed one of my comments. Here is the other one.

http://codereview.chromium.org/9240011/diff/6003/tools/testing/dart/test_suit...
File tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/9240011/diff/6003/tools/testing/dart/test_suit...
tools/testing/dart/test_suite.dart:502: Directory
createTemporaryDirectory(String testPath,
This confused me a little. Maybe we should call this createOutputDirectory
instead? With the current name I thought that we should always call
globalTemporaryDirectory.