Start two http servers for every configuration

Start two http servers for every configuration

This should fix: http://dartbug.com/8366

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

[Emily Fortuna, 2013-02-22 17:45:30]

https://codereview.chromium.org/12340021/diff/1/tools/test-runtime.dart
File tools/test-runtime.dart (right):

https://codereview.chromium.org/12340021/diff/1/tools/test-runtime.dart#newco...
tools/test-runtime.dart:75: var testSuites = new List<TestSuite>();
the value of runningBrowserTests is no longer defined in this file (in test.dart
you define it on line 112. probably should be added somewhere in this file as
well?

https://codereview.chromium.org/12340021/diff/1/tools/test-runtime.dart#newco...
tools/test-runtime.dart:83: servers.startServers('127.0.0.1', 9876);
we can get rid of the 9876 argument, since we can start on whatever port is
available now.

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
File tools/testing/dart/http_server.dart (left):

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
tools/testing/dart/http_server.dart:222: * This is intended to make it easier to
browse tests when manually running
why delete the documentation here? The functionality didn't seem to have
changed...

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
tools/testing/dart/http_server.dart:107: String httpServerCommandline() {
:-)

[kustermann, 2013-02-22 18:16:38]

PTAL

https://codereview.chromium.org/12340021/diff/1/tools/test-runtime.dart
File tools/test-runtime.dart (right):

https://codereview.chromium.org/12340021/diff/1/tools/test-runtime.dart#newco...
tools/test-runtime.dart:75: var testSuites = new List<TestSuite>();
On 2013/02/22 17:45:30, Emily Fortuna wrote:
> the value of runningBrowserTests is no longer defined in this file (in
test.dart
> you define it on line 112. probably should be added somewhere in this file as
> well? 

Thanks for catching that.

https://codereview.chromium.org/12340021/diff/1/tools/test-runtime.dart#newco...
tools/test-runtime.dart:83: servers.startServers('127.0.0.1', 9876);
On 2013/02/22 17:45:30, Emily Fortuna wrote:
> we can get rid of the 9876 argument, since we can start on whatever port is
> available now.
Well, I didn't know why this magic number was here, so I kept it. But I can
remove it.

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
File tools/testing/dart/http_server.dart (left):

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
tools/testing/dart/http_server.dart:222: * This is intended to make it easier to
browse tests when manually running
On 2013/02/22 17:45:30, Emily Fortuna wrote:
> why delete the documentation here? The functionality didn't seem to have
> changed...

Well it did change a little bit (but not in this CL, but rather in the 'putting
generated_tests into the build directory'-CL).

The first part of the comment refers to '... within *directory*'. We no longer
pass in the directory as an argument but rather the entries it should list.
The second part of the comment doesn't really describe what the function does
(but rather why we call this function).

I don't have strong opinions about keeping/deleting the comment. But IMHO it's
not necessary (the second part could be moved to the call-sites of
sendDirectoryListing).

[Emily Fortuna, 2013-02-22 18:42:51]

https://codereview.chromium.org/12340021/diff/1/tools/test-runtime.dart
File tools/test-runtime.dart (right):

https://codereview.chromium.org/12340021/diff/1/tools/test-runtime.dart#newco...
tools/test-runtime.dart:83: servers.startServers('127.0.0.1', 9876);
On 2013/02/22 18:16:38, kustermann wrote:
> On 2013/02/22 17:45:30, Emily Fortuna wrote:
> > we can get rid of the 9876 argument, since we can start on whatever port is
> > available now.
> Well, I didn't know why this magic number was here, so I kept it. But I can
> remove it.

The magic number was there from when Mads originally added the test server, he
had "special" ports that it would always use. Not sure why we have this logic in
this file anyway, seeing as it's only running for the VM in which case having a
server isn't necessary....

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
File tools/testing/dart/http_server.dart (left):

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
tools/testing/dart/http_server.dart:222: * This is intended to make it easier to
browse tests when manually running
On 2013/02/22 18:16:38, kustermann wrote:
> On 2013/02/22 17:45:30, Emily Fortuna wrote:
> > why delete the documentation here? The functionality didn't seem to have
> > changed...
> 
> Well it did change a little bit (but not in this CL, but rather in the
'putting
> generated_tests into the build directory'-CL).
> 
> The first part of the comment refers to '... within *directory*'. We no longer
> pass in the directory as an argument but rather the entries it should list.
> The second part of the comment doesn't really describe what the function does
> (but rather why we call this function).
> 
> I don't have strong opinions about keeping/deleting the comment. But IMHO it's
> not necessary (the second part could be moved to the call-sites of
> sendDirectoryListing).

Err on the side of keeping the documentation that explains why the function is
useful. It's intended for people that are unaware we have this functionality
(same goes for the testing server in general) to make it more discoverable for
them should they look at the http_server file.

[ricow1, 2013-02-25 11:42:56]

LGTM

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

https://codereview.chromium.org/12340021/diff/6001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:51: parser.addOption('build-directory',
help: 'The package root to use.');
Update option help text

https://codereview.chromium.org/12340021/diff/6001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:93: * [startHttpServer] will start two Http
servers.
startHttpServer -> startServers

[kustermann, 2013-02-25 11:51:22]

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
File tools/testing/dart/http_server.dart (left):

https://codereview.chromium.org/12340021/diff/1/tools/testing/dart/http_serve...
tools/testing/dart/http_server.dart:222: * This is intended to make it easier to
browse tests when manually running
On 2013/02/22 18:42:51, Emily Fortuna wrote:
> On 2013/02/22 18:16:38, kustermann wrote:
> > On 2013/02/22 17:45:30, Emily Fortuna wrote:
> > > why delete the documentation here? The functionality didn't seem to have
> > > changed...
> > 
> > Well it did change a little bit (but not in this CL, but rather in the
> 'putting
> > generated_tests into the build directory'-CL).
> > 
> > The first part of the comment refers to '... within *directory*'. We no
longer
> > pass in the directory as an argument but rather the entries it should list.
> > The second part of the comment doesn't really describe what the function
does
> > (but rather why we call this function).
> > 
> > I don't have strong opinions about keeping/deleting the comment. But IMHO
it's
> > not necessary (the second part could be moved to the call-sites of
> > sendDirectoryListing).
> 
> Err on the side of keeping the documentation that explains why the function is
> useful. It's intended for people that are unaware we have this functionality
> (same goes for the testing server in general) to make it more discoverable for
> them should they look at the http_server file.

IMHO, the people that are unaware of what the http server does should only need
to read the documentation at the top of the file (line 30).

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

https://codereview.chromium.org/12340021/diff/6001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:51: parser.addOption('build-directory',
help: 'The package root to use.');
On 2013/02/25 11:42:56, ricow1 wrote:
> Update option help text

Done.

https://codereview.chromium.org/12340021/diff/6001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:93: * [startHttpServer] will start two Http
servers.
On 2013/02/25 11:42:56, ricow1 wrote:
> startHttpServer -> startServers

Done.

[kustermann, 2013-02-25 11:55:05]

Committed patchset #3 manually as r18964 (presubmit successful).