Create generated tests inside the build directory

Create generated tests inside the build directory

Patch Set 1 is a revert of r18295.

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

[kustermann, 2013-02-12 01:49:23]

There's still a nasty issue with 'html/dromaeo_smoke_test' and I need to test it
in all possible ways -- but if you've got some time, take a look at it (only the
difference between patch set 1 and patch set 2 are new changes).

[ricow1, 2013-02-12 07:14:04]

https://codereview.chromium.org/12223074/diff/5003/tests/html/xhr_cross_origi...
File tests/html/xhr_cross_origin_test.dart (right):

https://codereview.chromium.org/12223074/diff/5003/tests/html/xhr_cross_origi...
tests/html/xhr_cross_origin_test.dart:33: var url =
"http://localhost:$port/root_dart/tests/html/xhr_cross_origin_data.txt";
Long line

https://codereview.chromium.org/12223074/diff/5003/tests/html/xhr_cross_origi...
tests/html/xhr_cross_origin_test.dart:52: var url =
"http://localhost:$port/root_dart/tests/html/xhr_cross_origin_data.txt";
long line

https://codereview.chromium.org/12223074/diff/5003/tests/html/xhr_cross_origi...
tests/html/xhr_cross_origin_test.dart:62: var url =
"http://localhost:$port/root_dart/tests/html/xhr_cross_origin_data.txt";
long line

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

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:16: const PREFIX_BUILDDIR = 'root_build';
how about putting these in the utils class, we use it elsewhere as well

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:133: Path getFilePathFromRequestPath(String
urlRequestPath) {
could we just move this and the other functions out of the request handler

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:138: // NOTE: files from the dart and from
the build directory are served as
from the dart -> from the dart repository

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:145: if (pathSegments.length > 1) {
using segments here seems like a lot of work, why not just
urlRequestPath.startsWith(PREFIX_BUILDDIR)

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

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:841: Except.isTrue(false);
Expect.fail();

[Emily Fortuna, 2013-02-12 18:04:09]

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

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/browser...
tools/testing/dart/browser_test.dart:25: <script type="text/javascript"
src="/root_dart/pkg/unittest/lib/test_controller.js">
long line

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

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:16: const PREFIX_BUILDDIR = 'root_build';
I know you have an explanation for how these are used down on lines 141, but can
you put some comments explaining what these constants are for exactly here when
the person opens the file?

Also, nit-picky, but why not "build_root" instead of "root_build"?

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:145: if (pathSegments.length > 1) {
On 2013/02/12 07:14:04, ricow1 wrote:
> using segments here seems like a lot of work, why not just
> urlRequestPath.startsWith(PREFIX_BUILDDIR)

Also, consider using the relativeTo function from the vendored_pkg path lib
rather than rewriting yet another of your own. It's thoroughly tested.

[kustermann, 2013-02-15 08:00:30]

I rebased now on top of blois http server change and addressed your comments.
This CL should be also a large step into the direction of 'making test.py
independent of the directory where it is run from'.

The dromaeo issue is still there and I need to test it extensively (this is now
the 3rd attempt to get this in, I don't like to do it a 4th time).

https://codereview.chromium.org/12223074/diff/5003/tests/html/xhr_cross_origi...
File tests/html/xhr_cross_origin_test.dart (right):

https://codereview.chromium.org/12223074/diff/5003/tests/html/xhr_cross_origi...
tests/html/xhr_cross_origin_test.dart:33: var url =
"http://localhost:$port/root_dart/tests/html/xhr_cross_origin_data.txt";
On 2013/02/12 07:14:04, ricow1 wrote:
> Long line

Done.

https://codereview.chromium.org/12223074/diff/5003/tests/html/xhr_cross_origi...
tests/html/xhr_cross_origin_test.dart:52: var url =
"http://localhost:$port/root_dart/tests/html/xhr_cross_origin_data.txt";
On 2013/02/12 07:14:04, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/12223074/diff/5003/tests/html/xhr_cross_origi...
tests/html/xhr_cross_origin_test.dart:62: var url =
"http://localhost:$port/root_dart/tests/html/xhr_cross_origin_data.txt";
On 2013/02/12 07:14:04, ricow1 wrote:
> long line

Done.

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

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/browser...
tools/testing/dart/browser_test.dart:25: <script type="text/javascript"
src="/root_dart/pkg/unittest/lib/test_controller.js">
On 2013/02/12 18:04:09, Emily Fortuna wrote:
> long line

Done.

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

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:16: const PREFIX_BUILDDIR = 'root_build';
On 2013/02/12 07:14:04, ricow1 wrote:
> how about putting these in the utils class, we use it elsewhere as well

I'm not sure what we should do. Here's my reasoning: 
These constants are part of the REST API of this server. The REST API is what
the http server provides to the outside world, so having them in
http_server.dart seems logical.

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:16: const PREFIX_BUILDDIR = 'root_build';
On 2013/02/12 18:04:09, Emily Fortuna wrote:
> I know you have an explanation for how these are used down on lines 141, but
can
> you put some comments explaining what these constants are for exactly here
when
> the person opens the file?

Good idea. Done.

> Also, nit-picky, but why not "build_root" instead of "root_build"?

Actually I started with 'build_root', but having
  /root_dart
  /root_build
  /root_packages   [not yet]
seems more appealing to me than (I like the common prefix)
  /dart_root
  /build_root
  /package_root    [not yet]

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:133: Path getFilePathFromRequestPath(String
urlRequestPath) {
On 2013/02/12 07:14:04, ricow1 wrote:
> could we just move this and the other functions out of the request handler

Done.

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:138: // NOTE: files from the dart and from
the build directory are served as
On 2013/02/12 07:14:04, ricow1 wrote:
> from the dart -> from the dart repository

Done.

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:145: if (pathSegments.length > 1) {
On 2013/02/12 07:14:04, ricow1 wrote:
> using segments here seems like a lot of work, why not just
> urlRequestPath.startsWith(PREFIX_BUILDDIR)

a) segments was used in the old code as well
('requestPath.segments().contains(packagesDirName)')
b) IMHO the code is easier to read/understand with segments

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:145: if (pathSegments.length > 1) {
On 2013/02/12 18:04:09, Emily Fortuna wrote:
> On 2013/02/12 07:14:04, ricow1 wrote:
> > using segments here seems like a lot of work, why not just
> > urlRequestPath.startsWith(PREFIX_BUILDDIR)
> 
> Also, consider using the relativeTo function from the vendored_pkg path lib
> rather than rewriting yet another of your own. It's thoroughly tested.

a) I don't like using vendored_pkg: the less things we depend on, the better
b) I don't get your point. I don't see any 'relativeTo' functionality here?! 
This code does the following (simplified):
  [first_segment, remaining] = requestPath.segments()
  switch (first_segment) {
      case PREFIX_BUILDDIR: return buildDir.join(remaining);
      case PREFIX_DARTDIR:  return dartDir.join(remaining);
      default: return null;
  }
(ignoring the search for 'packages')

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

https://codereview.chromium.org/12223074/diff/5003/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:841: Except.isTrue(false);
On 2013/02/12 07:14:04, ricow1 wrote:
> Expect.fail();

Done.

[kustermann, 2013-02-16 01:05:15]

Here are the fixes for dromaeo.
I tested the CL as good as I can and hope it works now without any issues.

As a positive side-effect: 'test.py' should now be independent of the current
working directory (as long as '--build-directory=.....' is passed, since the
build directory contains 'generated_tests/' and 'packages/').

[Emily Fortuna, 2013-02-16 02:22:00]

https://codereview.chromium.org/12223074/diff/30001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12223074/diff/30001/tools/test.dart#newcode129
tools/test.dart:129: // FIXME(kustermann,ricow): This is boken and should be
fixed ASAP.
nit: typo -- broken

https://codereview.chromium.org/12223074/diff/30001/tools/test.dart#newcode132
tools/test.dart:132: TestingServerRunner.setPackageRootDir(firstConf);
I don't understand this change. Why move line 137 up here?

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

https://codereview.chromium.org/12223074/diff/30001/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:21: // /echo: This will stream the data
received in the request stream back
nit: doc comments should be /// or /*

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

https://codereview.chromium.org/12223074/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:29: import 'vendored_pkg/path/path.dart' as
pathLib;
this isn't an "external dependency" if it's used only and specifically for this
test suite. But as it looks like you've removed all references to pathLib, you
might as well remove this import and delete the dead code in vendored_pkg/path
then, too

[kustermann, 2013-02-18 09:27:02]

PTAL

https://codereview.chromium.org/12223074/diff/30001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12223074/diff/30001/tools/test.dart#newcode129
tools/test.dart:129: // FIXME(kustermann,ricow): This is boken and should be
fixed ASAP.
On 2013/02/16 02:22:01, Emily Fortuna wrote:
> nit: typo -- broken

Done.

https://codereview.chromium.org/12223074/diff/30001/tools/test.dart#newcode132
tools/test.dart:132: TestingServerRunner.setPackageRootDir(firstConf);
On 2013/02/16 02:22:01, Emily Fortuna wrote:
> I don't understand this change. Why move line 137 up here?

Well, there are two things here (I don't know which one you are refering to):

 a) We need to (currently) call TestingServerRunner.setBuilddDir and
TestingServerRunner.setPackageRootDir (ricow started this, and I haven't
modified this).

 b) The old code has these calls inside the for loop (line 137): Now this could
lead people to the (wrong!) conclusion that this works -- but it doesn't work:
Since the variables in TestingServerRunner are all static, we call
'setPackageRootDir' multiple times and only the last call will have an effect.
By putting these calls before the for loop, I explicitly! state that we
currently use only the first configuration (and I addded a FIXME to change
that).

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

https://codereview.chromium.org/12223074/diff/30001/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:21: // /echo: This will stream the data
received in the request stream back
On 2013/02/16 02:22:01, Emily Fortuna wrote:
> nit: doc comments should be /// or /*

Done.

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

https://codereview.chromium.org/12223074/diff/30001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:29: import 'vendored_pkg/path/path.dart' as
pathLib;
On 2013/02/16 02:22:01, Emily Fortuna wrote:
> this isn't an "external dependency" if it's used only and specifically for
this
> test suite. But as it looks like you've removed all references to pathLib, you
> might as well remove this import and delete the dead code in vendored_pkg/path
> then, too

I removed it completely.

[ricow1, 2013-02-18 10:08:48]

LGTM

[Emily Fortuna, 2013-02-19 17:50:19]

lgtm

https://codereview.chromium.org/12223074/diff/30001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12223074/diff/30001/tools/test.dart#newcode132
tools/test.dart:132: TestingServerRunner.setPackageRootDir(firstConf);
On 2013/02/18 09:27:02, kustermann wrote:
> On 2013/02/16 02:22:01, Emily Fortuna wrote:
> > I don't understand this change. Why move line 137 up here?
> 
> Well, there are two things here (I don't know which one you are refering to):
> 
>  a) We need to (currently) call TestingServerRunner.setBuilddDir and
> TestingServerRunner.setPackageRootDir (ricow started this, and I haven't
> modified this).
> 
>  b) The old code has these calls inside the for loop (line 137): Now this
could
> lead people to the (wrong!) conclusion that this works -- but it doesn't work:
> Since the variables in TestingServerRunner are all static, we call
> 'setPackageRootDir' multiple times and only the last call will have an effect.
> By putting these calls before the for loop, I explicitly! state that we
> currently use only the first configuration (and I addded a FIXME to change
> that).

Sgtm. I was referring to line 132, the line I commented on, but was misreading
that, and thought that that loop was executed throughout running the tests
instead of before.

[kustermann, 2013-02-21 08:10:52]

Committed patchset #6 manually as r18802 (presubmit successful).