Add cross-origin test with credentials.

Add cross-origin test with credentials. I modified the test framework slightly
as well to Enable Cross-origin requests while testing from local files.



BUG=

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

[Emily Fortuna, 2012-12-20 23:21:25]

I added these tests while investigating:
http://code.google.com/p/dart/issues/detail?id=6792

While I couldn't reproduce the bug (on any platform!), we weren't testing the
functionality, so now we are. :-) In the meanwhile, I'll also improve the docs
so people hopefully will be less confused how to use HttpRequest.

[Bill Hesse, 2012-12-21 00:22:17]

https://codereview.chromium.org/11641005/diff/2001/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

https://codereview.chromium.org/11641005/diff/2001/tools/testing/run_selenium...
tools/testing/run_selenium.py:175: else:      options.binary_location =
os.path.join(dartium_dir, 'chrome')
DBC: Do we really allow this formatting of else:            blah.blah.blah?

[Emily Fortuna, 2012-12-21 00:33:08]

https://codereview.chromium.org/11641005/diff/2001/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

https://codereview.chromium.org/11641005/diff/2001/tools/testing/run_selenium...
tools/testing/run_selenium.py:175: else:      options.binary_location =
os.path.join(dartium_dir, 'chrome')
On 2012/12/21 00:22:17, Bill Hesse wrote:
> DBC: Do we really allow this formatting of else:            blah.blah.blah?

oh man -- no. not sure how that got in there to begin with. I'll fix it. Thanks
for noticing.

[ricow1, 2012-12-21 07:02:13]

Adding martin

[kustermann, 2012-12-21 09:37:16]

https://codereview.chromium.org/11641005/diff/2002/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

https://codereview.chromium.org/11641005/diff/2002/tools/testing/run_selenium...
tools/testing/run_selenium.py:163: options =
selenium.webdriver.chrome.options.Options()
Is there a reason why you create a new
"selenium.webdriver.chrome.options.Options()" object here? By using the one
above you would not need to add the "--allow-file-access-from-files" option
again (on line 167)!

[Emily Fortuna, 2012-12-21 18:53:14]

https://codereview.chromium.org/11641005/diff/2002/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

https://codereview.chromium.org/11641005/diff/2002/tools/testing/run_selenium...
tools/testing/run_selenium.py:163: options =
selenium.webdriver.chrome.options.Options()
On 2012/12/21 09:37:16, kustermann wrote:
> Is there a reason why you create a new
> "selenium.webdriver.chrome.options.Options()" object here? By using the one
> above you would not need to add the "--allow-file-access-from-files" option
> again (on line 167)!

Oops, nope, that and line 167 were  supposed to be deleted. Version control
failure on my part there!

[blois, 2012-12-28 18:10:35]

lgtm

[Emily Fortuna, 2012-12-29 01:23:57]

PTAL. 

I changed up the test framework a little bit more to have two servers running so
that we can legitimately test cross-origin requests without adding different
hacks like "--allow-file-access" for each browser.

[Bill Hesse, 2013-01-02 08:22:20]

I think Martin and Rico have comments.

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

https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:52: httpServer.listen(host, port);
I'm not sure about the whole concept of putting these in the test scripts,
rather than in individual tests, but in any case, anyone running these tests
locally has just started a server that serves their entire file system to anyone
on their network.  Could this be limited to accept only connections from
localhost?

[Mads Ager (google), 2013-01-02 10:09:50]

DBC

Very cool that we are starting to load the browser tests from an actual server!

As a follow-up change we should make sure that we can write tests in a way that
is independent of the actual port numbers used. If we are now loading all tests
from the test script server that should be possible. I think now would be a good
time to do that follow-up change before we get too many tests that rely on using
hard-coded port numbers.

https://codereview.chromium.org/11641005/diff/15001/tests/html/xhr_cross_orig...
File tests/html/xhr_cross_origin_test.dart (right):

https://codereview.chromium.org/11641005/diff/15001/tests/html/xhr_cross_orig...
tests/html/xhr_cross_origin_test.dart:15: var url =
"http://localhost:5432/tests/html/xhr_cross_origin_data.txt";
It would be cool to avoid the hard-coded port number here. If we start two
servers on any available port at the start of the test script, we can load the
tests with a URI that provides all of this information in the browser location
object. Something like:

http://localhost:${port1}/resource?crossOriginPort=${port2}

Then all tests can be written in a port number independent way. Resources can be
loaded relative to the current location and the cross origin port can be
extracted from the parameters.

https://codereview.chromium.org/11641005/diff/15001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/11641005/diff/15001/tools/test.dart#newcode145
tools/test.dart:145: startHttpServer('127.0.0.1', SERVER_PORT_ONE,
SERVER_PORT_TWO));
Instead of passing in two ports here, we could return two ports. If you call
listen on HttpServer without a port argument, it will use any available port.
That port can the be retrieved from the HttpServer instance and be returned
here. Those two port numbers can then be used in the URL for the test so that
the tests themselves are independent of the actual port numbers. That would
avoid issues when the hard-coded port numbers are already in use (either because
of other test scripts running or because those ports are used for other things).

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

https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:865: String filePrefix = '';
Looks like filePrefix can be completely removed?

https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:932:
'http://127.0.0.1:${SERVER_PORT_ONE}$htmlPath' :
Do we now load all browser tests from the test server? In that case, we don't
have to hard code the port number any more which would be great (allowing us to
run the browser test scripts in parallel without getting into problems with
ports).

Now that the tests are loaded from the server, there is no need to hard-code the
port number in the test that uses it to load resources. Instead, we can use the
browser location object to construct the resource URI based on the current
location. The test we added the http server for was: 

date_time_format_http_request_test.dart

It uses the server to load a resource. Now that the test itself is loaded from
the server we don't need the port number in the test and can load the resource
relative to the current location?

This can be done as follow ups to this change.

[blois, 2013-01-02 17:03:13]

On 2013/01/02 10:09:50, Mads Ager wrote:
> DBC
> 
> Very cool that we are starting to load the browser tests from an actual
server!
> 
> As a follow-up change we should make sure that we can write tests in a way
that
> is independent of the actual port numbers used. If we are now loading all
tests
> from the test script server that should be possible. I think now would be a
good
> time to do that follow-up change before we get too many tests that rely on
using
> hard-coded port numbers.
> 
>
https://codereview.chromium.org/11641005/diff/15001/tests/html/xhr_cross_orig...
> File tests/html/xhr_cross_origin_test.dart (right):
> 
>
https://codereview.chromium.org/11641005/diff/15001/tests/html/xhr_cross_orig...
> tests/html/xhr_cross_origin_test.dart:15: var url =
> "http://localhost:5432/tests/html/xhr_cross_origin_data.txt";
> It would be cool to avoid the hard-coded port number here. If we start two
> servers on any available port at the start of the test script, we can load the
> tests with a URI that provides all of this information in the browser location
> object. Something like:
> 
> http://localhost:%24%7Bport1%7D/resource?crossOriginPort=%24%7Bport2%7D
> 
> Then all tests can be written in a port number independent way. Resources can
be
> loaded relative to the current location and the cross origin port can be
> extracted from the parameters.
> 
> https://codereview.chromium.org/11641005/diff/15001/tools/test.dart
> File tools/test.dart (right):
> 
> https://codereview.chromium.org/11641005/diff/15001/tools/test.dart#newcode145
> tools/test.dart:145: startHttpServer('127.0.0.1', SERVER_PORT_ONE,
> SERVER_PORT_TWO));
> Instead of passing in two ports here, we could return two ports. If you call
> listen on HttpServer without a port argument, it will use any available port.
> That port can the be retrieved from the HttpServer instance and be returned
> here. Those two port numbers can then be used in the URL for the test so that
> the tests themselves are independent of the actual port numbers. That would
> avoid issues when the hard-coded port numbers are already in use (either
because
> of other test scripts running or because those ports are used for other
things).
> 
>
https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/test_s...
> File tools/testing/dart/test_suite.dart (right):
> 
>
https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/test_s...
> tools/testing/dart/test_suite.dart:865: String filePrefix = '';
> Looks like filePrefix can be completely removed?
> 
>
https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/test_s...
> tools/testing/dart/test_suite.dart:932:
> 'http://127.0.0.1:${SERVER_PORT_ONE}$htmlPath' :
> Do we now load all browser tests from the test server? In that case, we don't
> have to hard code the port number any more which would be great (allowing us
to
> run the browser test scripts in parallel without getting into problems with
> ports).
> 
> Now that the tests are loaded from the server, there is no need to hard-code
the
> port number in the test that uses it to load resources. Instead, we can use
the
> browser location object to construct the resource URI based on the current
> location. The test we added the http server for was: 
> 
> date_time_format_http_request_test.dart
> 
> It uses the server to load a resource. Now that the test itself is loaded from
> the server we don't need the port number in the test and can load the resource
> relative to the current location?
> 
> This can be done as follow ups to this change.

Agree with the comments about removing hard-coded ports- for debugging these
tests it'd be great if I could have an instance of the server running constantly
for manual testing, that would not conflict with another instance spun up when
running tests.

[Emily Fortuna, 2013-01-02 18:22:38]

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

https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:52: httpServer.listen(host, port);
On 2013/01/02 08:22:20, Bill Hesse wrote:
> I'm not sure about the whole concept of putting these in the test scripts,
> rather than in individual tests, but in any case, anyone running these tests
> locally has just started a server that serves their entire file system to
anyone
> on their network.  Could this be limited to accept only connections from
> localhost?
>  

This is no different from what we are currently doing -- firing up a server to
serve the dart repo. It's not your entire file system -- just your dart
directory (see TestUtils.dartDir(); on line 12). We're just firing up an
additional server.

[Emily Fortuna, 2013-01-04 00:51:03]

Whew, this is quite a CL!
It has a lot of files, but you don't need to look at all of them. I've added
people to do the review that are more familiar with the other files.

The only real changes overall are that I addressed all of the
comments/suggestions from the previous review; I just ended up touching a lot of
other files in the process of fixing everything.

Alan:
pkg/intl/test/date_time_format_http_request_test.dart

Siggi: 
pkg/unittest files (since Graham is out)

Jacob: 
sdk/lib/html/dart2js/html_dart2js.dart
sdk/lib/html/dartium/html_dartium.dart
tools/dom/src/_HttpRequestUtils.dart (complete with the mother of all comments)

Pete | Bill | Martin | Rico | Mads (aka anyone who wants to comment): 
The rest of the files, which are:
tests/html/form_data_test.dart
tests/html/html.status
tests/html/xhr_cross_origin_test.dart
tests/html/xhr_test.dart
tools/test.dart
tools/testing/dart/http_server.dart
tools/testing/dart/test_suite.dart
tools/testing/run_selenium.py

https://codereview.chromium.org/11641005/diff/15001/tests/html/xhr_cross_orig...
File tests/html/xhr_cross_origin_test.dart (right):

https://codereview.chromium.org/11641005/diff/15001/tests/html/xhr_cross_orig...
tests/html/xhr_cross_origin_test.dart:15: var url =
"http://localhost:5432/tests/html/xhr_cross_origin_data.txt";
On 2013/01/02 10:09:50, Mads Ager wrote:
> It would be cool to avoid the hard-coded port number here. If we start two
> servers on any available port at the start of the test script, we can load the
> tests with a URI that provides all of this information in the browser location
> object. Something like:
> 
> http://localhost:%24%7Bport1%7D/resource?crossOriginPort=%24%7Bport2%7D
> 
> Then all tests can be written in a port number independent way. Resources can
be
> loaded relative to the current location and the cross origin port can be
> extracted from the parameters.

Done.

https://codereview.chromium.org/11641005/diff/15001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/11641005/diff/15001/tools/test.dart#newcode145
tools/test.dart:145: startHttpServer('127.0.0.1', SERVER_PORT_ONE,
SERVER_PORT_TWO));
On 2013/01/02 10:09:50, Mads Ager wrote:
> Instead of passing in two ports here, we could return two ports. If you call
> listen on HttpServer without a port argument, it will use any available port.
> That port can the be retrieved from the HttpServer instance and be returned
> here. Those two port numbers can then be used in the URL for the test so that
> the tests themselves are independent of the actual port numbers. That would
> avoid issues when the hard-coded port numbers are already in use (either
because
> of other test scripts running or because those ports are used for other
things).

Done.

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

https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:865: String filePrefix = '';
On 2013/01/02 10:09:50, Mads Ager wrote:
> Looks like filePrefix can be completely removed?

Done.

https://codereview.chromium.org/11641005/diff/15001/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:932:
'http://127.0.0.1:${SERVER_PORT_ONE}$htmlPath' :
On 2013/01/02 10:09:50, Mads Ager wrote:
> Do we now load all browser tests from the test server? In that case, we don't
> have to hard code the port number any more which would be great (allowing us
to
> run the browser test scripts in parallel without getting into problems with
> ports).
> 
> Now that the tests are loaded from the server, there is no need to hard-code
the
> port number in the test that uses it to load resources. Instead, we can use
the
> browser location object to construct the resource URI based on the current
> location. The test we added the http server for was: 
> 
> date_time_format_http_request_test.dart
> 
> It uses the server to load a resource. Now that the test itself is loaded from
> the server we don't need the port number in the test and can load the resource
> relative to the current location?
> 
> This can be done as follow ups to this change.

Done.

https://codereview.chromium.org/11641005/diff/30001/tests/html/html.status
File tests/html/html.status (left):

https://codereview.chromium.org/11641005/diff/30001/tests/html/html.status#ol...
tests/html/html.status:299: xhr_test: Skip # Timeout
couldn't repro.

[Alan Knight, 2013-01-04 01:03:44]

I'll assume that the overall changes will enable the intl test to work that way,
and given that, LGTM.

[Siggi Cherem (dart-lang), 2013-01-04 01:49:43]

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_con...
File pkg/unittest/lib/html_config.dart (right):

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_con...
pkg/unittest/lib/html_config.dart:148: * this test.
nit: rephase, maybe "The value of the 'crossOriginPort' argument in the
[window.location]."

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_con...
pkg/unittest/lib/html_config.dart:150: int getCrossOriginPortNumber() {
I think this should not be in unittest. I mean, it feels very specific about how
we use test.dart + cross-domain tests. I think of unittest (and these
configurations) as something more general that is also used without test.dart.

Can we move this directly to the test that uses this or maybe provide a separate
library that can be imported by those tests?

Some additional suggestions on the code:
- 'PortNumber' sounds reduntant, how about just 'Port'?
- convert this into a getter: int get crossOriginPort { ... }

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_ind...
File pkg/unittest/lib/html_individual_config.dart (right):

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_ind...
pkg/unittest/lib/html_individual_config.dart:45: int getCrossOriginPortNumber()
{
style nit: use =>
int get crossOriginPort => htmlconfig.crossOriginPort;

but I guess if this moves out of unittest, then you might not need this anymore
=)

[Mads Ager (google), 2013-01-04 09:39:20]

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

https://codereview.chromium.org/11641005/diff/30004/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:11: HttpServer startHttpServer(String host,
[int allowedPort=-1]) {
Could you add spaces: [int allowedPort = -1]?

https://codereview.chromium.org/11641005/diff/30004/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:62: terminateHttpServers([List<HttpServer>
servers = const []]) {
Can we make the servers argument mandatory?

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

https://codereview.chromium.org/11641005/diff/30004/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:436: Map configuration, Path directory, [List
serverList=const []]) {
Could you add spacing around '=': [List serverList = const []]

https://codereview.chromium.org/11641005/diff/30004/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:941: if (htmlPath.startsWith('/')) htmlPath =
'/$htmlPath';
Shouldn't this be

if (!htmlPath.startsWith('/')) htmlPath = '/$htmlPath';

Otherwise it seems that we will add the slash only when it is not needed?

[Emily Fortuna, 2013-01-04 23:26:54]

PTAL.

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_con...
File pkg/unittest/lib/html_config.dart (right):

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_con...
pkg/unittest/lib/html_config.dart:148: * this test.
On 2013/01/04 01:49:43, Siggi Cherem (dart-lang) wrote:
> nit: rephase, maybe "The value of the 'crossOriginPort' argument in the
> [window.location]."

Done.

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_con...
pkg/unittest/lib/html_config.dart:150: int getCrossOriginPortNumber() {
On 2013/01/04 01:49:43, Siggi Cherem (dart-lang) wrote:
> I think this should not be in unittest. I mean, it feels very specific about
how
> we use test.dart + cross-domain tests. I think of unittest (and these
> configurations) as something more general that is also used without test.dart.
> 
> Can we move this directly to the test that uses this or maybe provide a
separate
> library that can be imported by those tests?
> 
> Some additional suggestions on the code:
> - 'PortNumber' sounds reduntant, how about just 'Port'?
> - convert this into a getter: int get crossOriginPort { ... }

Good point. Removed.

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_ind...
File pkg/unittest/lib/html_individual_config.dart (right):

https://codereview.chromium.org/11641005/diff/30004/pkg/unittest/lib/html_ind...
pkg/unittest/lib/html_individual_config.dart:45: int getCrossOriginPortNumber()
{
On 2013/01/04 01:49:43, Siggi Cherem (dart-lang) wrote:
> style nit: use =>
> int get crossOriginPort => htmlconfig.crossOriginPort;
> 
> but I guess if this moves out of unittest, then you might not need this
anymore
> =)

Done.

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

https://codereview.chromium.org/11641005/diff/30004/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:11: HttpServer startHttpServer(String host,
[int allowedPort=-1]) {
On 2013/01/04 09:39:20, Mads Ager wrote:
> Could you add spaces: [int allowedPort = -1]?

Done.

https://codereview.chromium.org/11641005/diff/30004/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:62: terminateHttpServers([List<HttpServer>
servers = const []]) {
On 2013/01/04 09:39:20, Mads Ager wrote:
> Can we make the servers argument mandatory?

Done.

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

https://codereview.chromium.org/11641005/diff/30004/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:436: Map configuration, Path directory, [List
serverList=const []]) {
On 2013/01/04 09:39:20, Mads Ager wrote:
> Could you add spacing around '=': [List serverList = const []]

Done.

https://codereview.chromium.org/11641005/diff/30004/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:941: if (htmlPath.startsWith('/')) htmlPath =
'/$htmlPath';
On 2013/01/04 09:39:20, Mads Ager wrote:
> Shouldn't this be
> 
> if (!htmlPath.startsWith('/')) htmlPath = '/$htmlPath';
> 
> Otherwise it seems that we will add the slash only when it is not needed?

Oops, yes. Refactoring fail. Fixed.

[Jacob, 2013-01-04 23:55:25]

couple tiny nits

https://codereview.chromium.org/11641005/diff/63001/tools/dom/src/_HttpReques...
File tools/dom/src/_HttpRequestUtils.dart (right):

https://codereview.chromium.org/11641005/diff/63001/tools/dom/src/_HttpReques...
tools/dom/src/_HttpRequestUtils.dart:20: // TODO(efortuna): Previously the
HttpRequest.get only called the
gramar nit: rewrite first sentence as
previously HttpRequest.get only invoked the callback when request.status was 0
or 200.

https://codereview.chromium.org/11641005/diff/63001/tools/dom/src/_HttpReques...
tools/dom/src/_HttpRequestUtils.dart:29: //(request.status == 200 ||
request.status == 0)) {
remove this last line.

[Alan Knight, 2013-01-04 23:58:30]

lgtm

[Emily Fortuna, 2013-01-05 00:06:48]

Bob, could you please take a look at the package:path modifications in
tools/testing/dart/browser_test.dart and
tools/testing/dart/http_server.dart

Then I think this CL will be ready.

[Bob Nystrom, 2013-01-05 00:26:42]

Some suggestions then browser_test LGTM. I didn't see any "package:" stuff in
http_server. Did I miss something?

https://codereview.chromium.org/11641005/diff/52003/tools/testing/dart/browse...
File tools/testing/dart/browser_test.dart (right):

https://codereview.chromium.org/11641005/diff/52003/tools/testing/dart/browse...
tools/testing/dart/browser_test.dart:60: var testPathDir =
pathLib.dirname(testPath.toString());
Remove the toString(), it's already one. :)

https://codereview.chromium.org/11641005/diff/52003/tools/testing/dart/browse...
tools/testing/dart/browser_test.dart:62:
dartHome.append("pkg/unittest/lib").toString(), from: testPathDir);
Mixing Path and the path lib is a bit confusing to me. Maybe:

var dartHomePath = dartHome.toNativeString();
var unitTest = pathLib.relative(pathLib.join(dartHomePath, 'pkg/unittest/lib',
from: testPathDir);

https://codereview.chromium.org/11641005/diff/52003/tools/testing/dart/browse...
tools/testing/dart/browser_test.dart:66: libString =
pathLib.join(dartHome.toString(), libString);
toNativeString(), I think? Path always trips me up.

[Siggi Cherem (dart-lang), 2013-01-05 00:31:10]

https://codereview.chromium.org/11641005/diff/52003/tests/html/xhr_cross_orig...
File tests/html/xhr_cross_origin_test.dart (right):

https://codereview.chromium.org/11641005/diff/52003/tests/html/xhr_cross_orig...
tests/html/xhr_cross_origin_test.dart:18: int get getCrossOriginPort {
nit: get rid of the 'get' prefix in the name. Simply 'crossOriginPort'

[Mads Ager (google), 2013-01-07 07:15:52]

https://codereview.chromium.org/11641005/diff/48003/tools/testing/dart/browse...
File tools/testing/dart/browser_test.dart (right):

https://codereview.chromium.org/11641005/diff/48003/tools/testing/dart/browse...
tools/testing/dart/browser_test.dart:58: var testPathDir =
pathLib.dirname(testPath);
Can you pass in a Path here, please? I don't like the mix of Path and String and
the mix of two Path libraries. Let's stick with the dart:io Path library for the
test scripts at this point.

The dart:io Path object has a relativeTo method as well. Would that work for
you?

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

https://codereview.chromium.org/11641005/diff/48003/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:42: if
(path.toNativePath().endsWith('.html')) {
path.extension == 'html'

https://codereview.chromium.org/11641005/diff/48003/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:44: } else if
(path.toNativePath().endsWith('.js')) {
ditto

https://codereview.chromium.org/11641005/diff/48003/tools/testing/dart/http_s...
tools/testing/dart/http_server.dart:46: } else if
(path.toNativePath().endsWith('.dart')) {
ditto

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

https://codereview.chromium.org/11641005/diff/48003/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:24: import '../../../pkg/path/lib/path.dart'
as pathLib;
I would prefer that we only use one path library in the test scripts. Either we
use the one in dart:io or the one in pkg. We are going to evaluate where to got
with path this quarter to eliminate one of them. Can we stick to the one in
dart:io for now?

It seems like what you are using is relativeTo which is also in the dart:io
library:

http://api.dartlang.org/docs/bleeding_edge/dart_io/Path.html

[Emily Fortuna, 2013-01-07 18:01:31]

https://codereview.chromium.org/11641005/diff/48003/tools/testing/dart/browse...
File tools/testing/dart/browser_test.dart (right):

https://codereview.chromium.org/11641005/diff/48003/tools/testing/dart/browse...
tools/testing/dart/browser_test.dart:58: var testPathDir =
pathLib.dirname(testPath);
On 2013/01/07 07:15:52, Mads Ager wrote:
> Can you pass in a Path here, please? I don't like the mix of Path and String
and
> the mix of two Path libraries. Let's stick with the dart:io Path library for
the
> test scripts at this point.
> 
> The dart:io Path object has a relativeTo method as well. Would that work for
> you?

Unfortunately the relativeTo method in the main Path library is does not work on
Windows where the path is already a relative path (see bob's comment that was
deleted in this file 62-65). Ideally once I started switching over to use the
other path library, I'd change it everywhere in test.dart, but this CL was
getting to be so big, I figured I'd do that in another CL. 

At any rate, this CL got reverted due to Windows issues, and I won't be putting
it back in until things calm down a lot on our bots.