Make browser tests all run from a server instead of the local filesystem.

Make browser tests all run from a server instead of the local filesystem.

BUG=

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

[Emily Fortuna, 2013-01-10 02:07:10]

I've included everyone that was on the previous version of this CL, plus a few
more due the the additional changes. Here's the files you should look over:

Alan: intl stuff
John: swarm adjustments
Pete: html test changes in html lib changes.
Peter: package root changes in test_runner.dart
Martin: test.dart changes.
Anyone else cc'd (or not cc'd): mainly FYI test.dart changes

https://codereview.chromium.org/11810004/diff/9092/samples/swarm/SwarmApp.dart
File samples/swarm/SwarmApp.dart (right):

https://codereview.chromium.org/11810004/diff/9092/samples/swarm/SwarmApp.dar...
samples/swarm/SwarmApp.dart:26: Swarm([bool useCannedData = false]) : super(),
onLoadFired = false {
I unfortunately had to add this hack because the way the data was being
transferred over the server on appengine was doing some fancy encoding thing
that my dumb dart server didn't know how to do. (see
samples/swarm/appengine/emitter.py)

https://codereview.chromium.org/11810004/diff/9092/tests/html/exceptions_test...
File tests/html/exceptions_test.dart (left):

https://codereview.chromium.org/11810004/diff/9092/tests/html/exceptions_test...
tests/html/exceptions_test.dart:13: expect(e.name, 'SECURITY_ERR');
the Security error, curiously, is called SecurityError when called over a
server, but SECURITY_ERR when through a file (it got renamed in WebKit a month
or so ago)

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:13: main() {
Q: What happened to this file? 
A: The main issue that caused all these changes was the dartium checkout output
directory is in a different location than the regular dart checkout build
directory. This causes problems when trying to set the package root in a way
that works on both checkouts, without passing on this complication to the user
writing tests. 

With the addition of the DART_PACKAGE_ROOT environment variable (added to drt in
a previous CL), we could no longer simply run the commands printed out by the
buildbot (or from our command line) to reproduce the environment of the test,
since we had to know what the package root value is, too. For debugabilty, I
added a command in the testing printout that lists the command to start this
file up as a local server for debugging.

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/test_ru...
File tools/testing/dart/test_runner.dart (left):

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:182: Uri packageRootUri,
Since all tests are being served via our server now, which deals with package
root issues on its own, this isn't being used.

(and package_root was never being propagated to Dartium proper)

[Jennifer Messerly, 2013-01-10 02:18:10]

lgtm

https://codereview.chromium.org/11810004/diff/9092/samples/swarm/SwarmApp.dart
File samples/swarm/SwarmApp.dart (right):

https://codereview.chromium.org/11810004/diff/9092/samples/swarm/SwarmApp.dar...
samples/swarm/SwarmApp.dart:26: Swarm([bool useCannedData = false]) : super(),
onLoadFired = false {
On 2013/01/10 02:07:10, Emily Fortuna wrote:
> I unfortunately had to add this hack because the way the data was being
> transferred over the server on appengine was doing some fancy encoding thing
> that my dumb dart server didn't know how to do. (see
> samples/swarm/appengine/emitter.py)

yeah that's totally cool. The old code was dumb and assumed any server meant it
was running from its own real server :)

https://codereview.chromium.org/11810004/diff/9092/samples/tests/samples/swar...
File samples/tests/samples/swarm/swarm_test.dart (right):

https://codereview.chromium.org/11810004/diff/9092/samples/tests/samples/swar...
samples/tests/samples/swarm/swarm_test.dart:21: Swarm swarm = new Swarm(true);
named arg?

[kustermann, 2013-01-10 09:05:42]

I took just a quick look at it.

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1786: return path;
We should really use the Path class everywhere we can. There are currently a lot
of places where we use Strings instead.

[ahe, 2013-01-10 09:17:58]

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:422: this.serverList,
Why not use a named argument?

[Mads Ager (google), 2013-01-10 09:18:55]

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:74: // Remove the initial slash in the
request, since it's not an absolute
I don't think this will work on Windows. I patched this in on my Windows machine
and something is fishy.

I don't like the mix of Path libraries in this change. It makes it very
confusing. I would really prefer if we can stick with one path library being
used. It seems simplest to stick with the dart:io one. Could you give me the
examples of stuff that you would like that doesn't work with that library?

[ahe, 2013-01-10 09:27:23]

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:13: main() {
On 2013/01/10 02:07:10, Emily Fortuna wrote:
> Q: What happened to this file? 
> A: The main issue that caused all these changes was the dartium checkout
output
> directory is in a different location than the regular dart checkout build
> directory. This causes problems when trying to set the package root in a way
> that works on both checkouts, without passing on this complication to the user
> writing tests. 
> 
> With the addition of the DART_PACKAGE_ROOT environment variable (added to drt
in
> a previous CL), we could no longer simply run the commands printed out by the
> buildbot (or from our command line) to reproduce the environment of the test,
> since we had to know what the package root value is, too. For debugabilty, I
> added a command in the testing printout that lists the command to start this
> file up as a local server for debugging.

Great idea!

[blois, 2013-01-10 17:58:16]

lgtm

Couple comments, but mostly for future items.

https://codereview.chromium.org/11810004/diff/9092/tests/html/exceptions_test...
File tests/html/exceptions_test.dart (left):

https://codereview.chromium.org/11810004/diff/9092/tests/html/exceptions_test...
tests/html/exceptions_test.dart:13: expect(e.name, 'SECURITY_ERR');
On 2013/01/10 02:07:10, Emily Fortuna wrote:
> the Security error, curiously, is called SecurityError when called over a
> server, but SECURITY_ERR when through a file (it got renamed in WebKit a month
> or so ago)

'SecurityError' is the correct string, per spec-
http://www.w3.org/TR/dom/#error-names-table

Note that code is deprecated and message is non-standard, so these fields aren't
really appropriate for testing. I've opened a bug to remove code and we should
tackle message when we get this test working on all platforms.

https://codereview.chromium.org/11810004/diff/9092/tests/html/html.status
File tests/html/html.status (right):

https://codereview.chromium.org/11810004/diff/9092/tests/html/html.status#new...
tests/html/html.status:94: url_test: Fail                           # IE10 does
not support from file://
Any idea why this test is not working now?

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

https://codereview.chromium.org/11810004/diff/9092/tests/html/xhr_cross_origi...
tests/html/xhr_cross_origin_test.dart:19: var searchUrl =
window.location.search;
It'd be nice if we exposed window.location.search as a map. I opened bug 7825.

https://codereview.chromium.org/11810004/diff/9092/tests/html/xhr_test.dart
File tests/html/xhr_test.dart (right):

https://codereview.chromium.org/11810004/diff/9092/tests/html/xhr_test.dart#n...
tests/html/xhr_test.dart:18:
xhr.on.readyStateChange.add(expectAsyncUntil1((event) {
Does the load event not suffice here?

[Alan Knight, 2013-01-10 19:12:56]

lgtm

I think this should obsolete dartbug.com/6410 , does that sound correct?

[Bob Nystrom, 2013-01-10 19:36:42]

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:74: // Remove the initial slash in the
request, since it's not an absolute
On 2013/01/10 09:18:55, Mads Ager wrote:
> I don't think this will work on Windows. I patched this in on my Windows
machine
> and something is fishy.

Any details here? If there's bugs in the path package, they will affect pub so
I'd like to know.

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:80: if
(requestPath.toString().contains(packagesDirName)) {
This will do the wrong thing if you have a directory with "packages" in the
middle of the name. Instead, you can do:

if (pathLib.split(requestPath).contains(packagesDirName))

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:83: requestPath =
requestPath.substring(requestPath.indexOf(packagesDirName));
Long line.

[ricow1, 2013-01-10 20:03:41]

https://codereview.chromium.org/11810004/diff/9092/samples/swarm/DataSource.dart
File samples/swarm/DataSource.dart (right):

https://codereview.chromium.org/11810004/diff/9092/samples/swarm/DataSource.d...
samples/swarm/DataSource.dart:60: void callback(Sections sections)) {
I would indent this under bool

https://codereview.chromium.org/11810004/diff/9092/sdk/lib/html/dart2js/html_...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/11810004/diff/9092/sdk/lib/html/dart2js/html_...
sdk/lib/html/dart2js/html_dart2js.dart:25647: onComplete(HttpRequest request),
indentation

https://codereview.chromium.org/11810004/diff/9092/tests/html/html.status
File tests/html/html.status (right):

https://codereview.chromium.org/11810004/diff/9092/tests/html/html.status#new...
tests/html/html.status:263: #    (NS_ERROR_DOM_BAD_URI) ...
does this comment belong to the removed line

https://codereview.chromium.org/11810004/diff/9092/tests/html/xhr_test.dart
File tests/html/xhr_test.dart (right):

https://codereview.chromium.org/11810004/diff/9092/tests/html/xhr_test.dart#n...
tests/html/xhr_test.dart:52: expect(event.readyState, equals(HttpRequest.DONE));
I would extract this body into a function, we do the excact same "validation" 3
times above and below

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/browser...
tools/testing/dart/browser_test.dart:58: var testPathDir =
pathLib.dirname(testPath);
could you talk with Mads, I think he is adding functionality to the build in
path class so that we can make this nicer (here and elsewhere). We are basically
mixin two path libraries and strings

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/browser...
tools/testing/dart/browser_test.dart:77: import '${pathLib.relative(libString,
from: testPathDir)}' as Test;
how is that possible with testPathDir here?

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/test_ru...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1709: if (test is BrowserTestCase) {
this is really useful, thanks

[Emily Fortuna, 2013-01-11 02:57:11]

PTAL.

Off of this code review Mads and I are discussing on the path libraries issue.

https://codereview.chromium.org/11810004/diff/9092/samples/swarm/DataSource.dart
File samples/swarm/DataSource.dart (right):

https://codereview.chromium.org/11810004/diff/9092/samples/swarm/DataSource.d...
samples/swarm/DataSource.dart:60: void callback(Sections sections)) {
On 2013/01/10 20:03:41, ricow1 wrote:
> I would indent this under bool

Done.

https://codereview.chromium.org/11810004/diff/9092/samples/tests/samples/swar...
File samples/tests/samples/swarm/swarm_test.dart (right):

https://codereview.chromium.org/11810004/diff/9092/samples/tests/samples/swar...
samples/tests/samples/swarm/swarm_test.dart:21: Swarm swarm = new Swarm(true);
On 2013/01/10 02:18:10, John Messerly wrote:
> named arg?

Done.

https://codereview.chromium.org/11810004/diff/9092/tests/html/html.status
File tests/html/html.status (right):

https://codereview.chromium.org/11810004/diff/9092/tests/html/html.status#new...
tests/html/html.status:94: url_test: Fail                           # IE10 does
not support from file://
On 2013/01/10 17:58:16, blois wrote:
> Any idea why this test is not working now?

I had tested on IE 9, but not IE10.

https://codereview.chromium.org/11810004/diff/9092/tests/html/html.status#new...
tests/html/html.status:263: #    (NS_ERROR_DOM_BAD_URI) ...
On 2013/01/10 20:03:41, ricow1 wrote:
> does this comment belong to the removed line

Good catch. yes.

https://codereview.chromium.org/11810004/diff/9092/tests/html/xhr_test.dart
File tests/html/xhr_test.dart (right):

https://codereview.chromium.org/11810004/diff/9092/tests/html/xhr_test.dart#n...
tests/html/xhr_test.dart:52: expect(event.readyState, equals(HttpRequest.DONE));
On 2013/01/10 20:03:41, ricow1 wrote:
> I would extract this body into a function, we do the excact same "validation"
3
> times above and below

Done.

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/browser...
tools/testing/dart/browser_test.dart:77: import '${pathLib.relative(libString,
from: testPathDir)}' as Test;
On 2013/01/10 20:03:41, ricow1 wrote:
> how is that possible with testPathDir here?

This string that is returned from this function will go into a file that is
generated in the output directory, and we want to find the path of the library
relative to that location. libString is an absolute path, and we can no longer
use absolute paths to be printed out here because we're serving these files from
a server.

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:80: if
(requestPath.toString().contains(packagesDirName)) {
On 2013/01/10 19:36:43, Bob Nystrom wrote:
> This will do the wrong thing if you have a directory with "packages" in the
> middle of the name. Instead, you can do:
> 
> if (pathLib.split(requestPath).contains(packagesDirName))

Done.

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:83: requestPath =
requestPath.substring(requestPath.indexOf(packagesDirName));
On 2013/01/10 19:36:43, Bob Nystrom wrote:
> Long line.

Done.

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:422: this.serverList,
On 2013/01/10 09:17:58, ahe wrote:
> Why not use a named argument?

good idea. fixed.

[Mads Ager (google), 2013-01-11 10:11:03]

LGTM once the issues on Windows have been addressed.

One command that failed for me was:

d:\src\dart\dart> python tools\test.py -mrelease -rdrt corelib

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:74: // Remove the initial slash in the
request, since it's not an absolute
On 2013/01/10 19:36:43, Bob Nystrom wrote:
> On 2013/01/10 09:18:55, Mads Ager wrote:
> > I don't think this will work on Windows. I patched this in on my Windows
> machine
> > and something is fishy.
> 
> Any details here? If there's bugs in the path package, they will affect pub so
> I'd like to know.

This is not a problem with the pathos library but I think it is an invalid use
of it on Windows. :-)

When working with HttpRequest paths you should not use the platform path
separator. In the dart:io path library that is why there is a specific
constructor to deal with native paths. I guess in pathos you could use a builder
here that explicitly deals with '/' as the separator on all platforms?

Here is what this will do to a request path on Windows:

var requestPath = '/build/flaf.dart';
var normalized = normalize(request.path);  // 'build\flaf.dart' ... yikes
var substring = normalized.substring(1);  // 'uild\flaf.dart' ... double yikes

Because of these subtleties and differences in the libraries we should only use
one of them. There is a comment in here somewhere that Path.isAbsolute does not
work on Windows. Could you give me an example of that?

I strongly agree with the direction of this patch and I want it to land soon.
Emily, could you add a TODO and make sure to unify how we deal with paths in a
follow-up change? :-)

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

https://codereview.chromium.org/11810004/diff/24001/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:37: 'Server listening on port
${TestingServerRunner.serverList[0].port}.');
Nit: consider splitting the string in the middle instead?

print('Server listening on port '
      '${TestingServerRunner.serverList[0].port}.');

https://codereview.chromium.org/11810004/diff/24001/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:46: * Runs a set of servers that are
initialized specifically the needs for our
Reformulate: specifically the needs for

https://codereview.chromium.org/11810004/diff/24001/tools/testing/dart/test_r...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/11810004/diff/24001/tools/testing/dart/test_r...
tools/testing/dart/test_runner.dart:1710: // Additional command for rerunning
the steps locally after the fact.
Nice! :)

[ricow1, 2013-01-11 10:22:51]

LGTM

https://codereview.chromium.org/11810004/diff/24001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/11810004/diff/24001/tools/test.dart#newcode108
tools/test.dart:108: // The http server is available on window.location.port,
and a second server
The http server -> The first http server

[Bob Nystrom, 2013-01-11 16:27:42]

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

https://codereview.chromium.org/11810004/diff/9092/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:74: // Remove the initial slash in the
request, since it's not an absolute
On 2013/01/11 10:11:03, Mads Ager wrote:
> On 2013/01/10 19:36:43, Bob Nystrom wrote:
> > On 2013/01/10 09:18:55, Mads Ager wrote:
> > > I don't think this will work on Windows. I patched this in on my Windows
> > machine
> > > and something is fishy.
> > 
> > Any details here? If there's bugs in the path package, they will affect pub
so
> > I'd like to know.
> 
> This is not a problem with the pathos library but I think it is an invalid use
> of it on Windows. :-)
> 
> When working with HttpRequest paths you should not use the platform path
> separator. In the dart:io path library that is why there is a specific
> constructor to deal with native paths. I guess in pathos you could use a
builder
> here that explicitly deals with '/' as the separator on all platforms?

Yes, that's exactly why Builder exists. Instead of calling the top level
functions, you can just do:

var builder = new pathLib.Builder(style: pathLib.Style.posix);

and then do builder.normalize() instead of pathLib.normalize();

> 
> Here is what this will do to a request path on Windows:
> 
> var requestPath = '/build/flaf.dart';
> var normalized = normalize(request.path);  // 'build\flaf.dart' ... yikes
> var substring = normalized.substring(1);  // 'uild\flaf.dart' ... double yikes

Yeah, I discussed this a bit with Emily. Using the top level functions is the
wrong thing here because the code is dealing with URLs and not the paths for the
host filesystem.

> Because of these subtleties and differences in the libraries we should only
use
> one of them. There is a comment in here somewhere that Path.isAbsolute does
not
> work on Windows. Could you give me an example of that?

From digging through the code, I think it may do the right thing if you're
scrupulous about using fromNative() and toNativeString()? I know I've been
burned by isAbsolute on Path before and I'm pretty good about using those (I
have the scars from failing there), but I can't recall the details now, sorry.

It may just be that I was confused by the documentation which says: "Is this
path an absolute path, beginning with a path separator?" which doesn't make
sense for a Windows path.

[ahe, 2013-01-16 13:30:37]

Please be very careful when you make changes to test.dart.

The Dart files in tools/testing/dart (and test.dart) should only import dart:
libraries and libraries from tools/testing/dart.

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

https://codereview.chromium.org/11810004/diff/28008/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:10: import
'../../../pkg/args/lib/args.dart';
This breaks test.dart in a subtle, but severe way.

test.dart is supposed to test the Dart platform, so it must be self-contained,
otherwise you'll be unable to test the platform. Before this CL, test.dart was
self-contained as all the libraries it uses are compiled into the binary VM in
tools/testing/bin/.

Let's say I'm working on a CL to change how you specify named parameters.

I find all the uses of named parameters in sdk/lib and pkg and update them. Then
I run the tests. This happens:

'file:///Users/ahe/Dart/all/dart/pkg/args/lib/args.dart': Error: line 356 pos
41: '=' expected
      this.negatable, this.allowMultiple: false]);
                                        ^
'file:///Users/ahe/Dart/all/dart/tools/testing/dart/http_server.dart': Error:
line 10 pos 1: library handler failed
import '../../../pkg/args/lib/args.dart';
^
'file:///Users/ahe/Dart/all/dart/tools/testing/dart/test_runner.dart': Error:
line 20 pos 1: library handler failed
import "http_server.dart" as http_server;
^
'file:///Users/ahe/Dart/all/dart/tools/test.dart': Error: line 28 pos 1: library
handler failed
import "testing/dart/test_runner.dart";
^

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

https://codereview.chromium.org/11810004/diff/28008/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:25: import '../../../pkg/path/lib/path.dart'
as pathLib;
Also breaks test.dart.