Change the location of the output directory of generated tests to be inside build directory, but no…

Change the location of the output directory of generated tests to be inside build directory, but not relative to the dart root.

This is causing issues on dartium builders where we actually only run because there is a build/Release (and out/Release or xcodebuild/Release for mac/linux) on the bot that has been created inside src/dart.

Also, fix the http server in the testing script to serve files from the output directory - otherwise the tests will not be accessible if an alternative build directory is used.

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

[kustermann, 2013-02-04 19:38:03]

This is basically ricow's change with a few additional fixes for wrong paths and
a bit of cleanup. I tested this in my dartium checkout and it seems to work.

Comments are welcome.

[ricow1, 2013-02-06 08:13:28]

LGTM

https://codereview.chromium.org/12183022/diff/9001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12183022/diff/9001/tools/test.dart#newcode129
tools/test.dart:129: // FIXME(kustermann,ricow): This is boken and should be
fixed ASAP.
lets have a bug to track this

https://codereview.chromium.org/12183022/diff/9001/tools/testing/dart/http_se...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12183022/diff/9001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:44: TestingServerRunner._buildDirectory =
new Path(args['package-root']);
setting the _buildDirectory to the package root calls for a comment

https://codereview.chromium.org/12183022/diff/9001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:88: // TODO(kustermann,ricow): We could
change this to the following scheme:
file a bug

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

https://codereview.chromium.org/12183022/diff/9001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1022: subtestNames, subtestIndex);
indentation

[kustermann, 2013-02-06 16:33:51]

https://codereview.chromium.org/12183022/diff/9001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12183022/diff/9001/tools/test.dart#newcode129
tools/test.dart:129: // FIXME(kustermann,ricow): This is boken and should be
fixed ASAP.
On 2013/02/06 08:13:28, ricow1 wrote:
> lets have a bug to track this

Done.

https://codereview.chromium.org/12183022/diff/9001/tools/testing/dart/http_se...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12183022/diff/9001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:44: TestingServerRunner._buildDirectory =
new Path(args['package-root']);
On 2013/02/06 08:13:28, ricow1 wrote:
> setting the _buildDirectory to the package root calls for a comment

Done.

https://codereview.chromium.org/12183022/diff/9001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:88: // TODO(kustermann,ricow): We could
change this to the following scheme:
On 2013/02/06 08:13:28, ricow1 wrote:
> file a bug

Done.

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

https://codereview.chromium.org/12183022/diff/9001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1022: subtestNames, subtestIndex);
On 2013/02/06 08:13:28, ricow1 wrote:
> indentation

Done.

[Emily Fortuna, 2013-02-07 23:37:47]

This CL needs an adjustment. As a result of this change, if you try to run
test.py while standing at the top level of your dartium checkout (the standard
thing to do), then the tests get generated in your dartium build directory, but
httpServer is still looking for it in the dart build directory.

https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/http_s...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:105: if
(requestPath.toString().startsWith(buildDirectory.toString())) {
This doesn't work because requestPath isn't an absolute path!

[kustermann, 2013-02-08 01:54:44]

First off, thanks Emily for looking into this CL as well (since the testing
scripts are pretty fragile, we've to be very careful with changes)

> This CL needs an adjustment. As a result of this change, if you try to run
> test.py while standing at the top level of your dartium checkout (the standard
> thing to do), then the tests get generated in your dartium build directory,
but
> httpServer is still looking for it in the dart build directory.

I think this is not quite correct. Here's my reasoning:
- the generated tests are placed into a subdirectory of
'TestUtils.buildDir(config)' [see createOutputDirectory in test_suite.dart]
- the http server serves files starting with 'TestUtils.buildDir(config)' from
'TestUtils.buildDir(config)' as well
Both use the same 'config', so the http server should serve the correct files.

Furthermore: In case we're in a dartium checkout we always have to specify
'--build-directory=out/Release'. Here's the reasoning:
- the building step creates the 'BUILDDIR/packages' directory with symbolic
links in it
- the testing script has to use the same build directory (otherwise we won't
find the 'packages/' directory)
- running test.py without '--build-directory' in a dartium checkout means that
we default to 'out/ReleaseIA32' (and here we don't have a 'packages' directory
-- so tests with package imports will fail)

But it's definitively possible that I miss something here?

On 2013/02/07 23:37:47, Emily Fortuna wrote:
> This CL needs an adjustment. As a result of this change, if you try to run
> test.py while standing at the top level of your dartium checkout (the standard
> thing to do), then the tests get generated in your dartium build directory,
but
> httpServer is still looking for it in the dart build directory.
> 
>
https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/http_s...
> File tools/testing/dart/http_server.dart (right):
> 
>
https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/http_s...
> tools/testing/dart/http_server.dart:105: if
> (requestPath.toString().startsWith(buildDirectory.toString())) {
> This doesn't work because requestPath isn't an absolute path!

[kustermann, 2013-02-08 01:55:13]

https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/http_s...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:105: if
(requestPath.toString().startsWith(buildDirectory.toString())) {
On 2013/02/07 23:37:47, Emily Fortuna wrote:
> This doesn't work because requestPath isn't an absolute path!

It "sort of" works because this code assumes that buildDirectory is relative (as
well as the requestPath).

This is broken if 'buildDirectory' is absolute (as you can see in
test_suite.dart I added a FIXME for that).

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

https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:1853: if (configuration['build_directory'] !=
'') {
This is the place of the FIXME.

[Emily Fortuna, 2013-02-08 18:30:12]

It's possible what I diagnosed as the problem is not actually the source of the
bug, but if you try running your tests (individually, given the commands printed
out in the script) in the top level dartium checkout, the tests *don't work*.
You get a bunch of 404s. I wasted over an hour unsuccessfully trying to track
down this problem.

perhaps that is the issue is in the command to rerun the tests the
package-root/buildDirectory specified is absolute? 

On 2013/02/08 01:55:13, kustermann wrote:
>
https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/http_s...
> File tools/testing/dart/http_server.dart (right):
> 
>
https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/http_s...
> tools/testing/dart/http_server.dart:105: if
> (requestPath.toString().startsWith(buildDirectory.toString())) {
> On 2013/02/07 23:37:47, Emily Fortuna wrote:
> > This doesn't work because requestPath isn't an absolute path!
> 
> It "sort of" works because this code assumes that buildDirectory is relative
(as
> well as the requestPath).
> 
> This is broken if 'buildDirectory' is absolute (as you can see in
> test_suite.dart I added a FIXME for that).
> 
>
https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/test_s...
> File tools/testing/dart/test_suite.dart (right):
> 
>
https://codereview.chromium.org/12183022/diff/15001/tools/testing/dart/test_s...
> tools/testing/dart/test_suite.dart:1853: if (configuration['build_directory']
!=
> '') {
> This is the place of the FIXME.

[kustermann, 2013-02-09 01:39:33]

On 2013/02/08 18:30:12, Emily Fortuna wrote:
> It's possible what I diagnosed as the problem is not actually the source of
the
> bug, but if you try running your tests (individually, given the commands
printed
> out in the script) in the top level dartium checkout, the tests *don't work*.
> You get a bunch of 404s.

I'll find the issue and fix it as soon I've some time. Thanks again, for
noticing.

> I wasted over an hour unsuccessfully trying to track down this problem.

You don't want to know how much time we've spent on such issues.
We're trying to get the code in better shape -- making it more
understandable/robust/.... But we've to do it in small steps.