Read locales over http in the browser

Read locales over http in the browser
Committed: https://code.google.com/p/dart/source/detail?r=12664

[Alan Knight, 2012-09-20 21:05:12]

Seems to be working now, at least on Mac. I've disabled running it on Windows,
as that seems likely to be worth a CL by itself :-)

PTAL.

[Emily Fortuna, 2012-09-20 23:07:19]

lgtm with some suggestions

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/date_time_fo...
File pkg/intl/test/date_time_format_http_request_test.dart (right):

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/date_time_fo...
pkg/intl/test/date_time_format_http_request_test.dart:14: //
ExtraCommandArgs=pkg/intl/test/start_web_server.dart
will this be removed after Graham has implemented his server-browser solution?

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
File pkg/intl/test/start_web_server.dart (right):

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
pkg/intl/test/start_web_server.dart:23: new Timer(1000, (t) => exit(0));
Perhaps if we find ourselves doing more of these sorts of tests in the future,
we may want to make some sort of polling solution.

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

https://codereview.chromium.org/10959002/diff/5002/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:442: List _append(list1,list2) =>
[]..addAll(list1)..addAll(list2);
woohoo! real-life use of method cascades!

Is there a reason why we don't just do 
list1..addAll(list2)? Looks like commands will always be at least [] and it
seems to only be called in one place. (in which case maybe you don't need to
define the extra method?

[Emily Fortuna, 2012-09-20 23:13:45]

+ whesse, resident test framework expert

[Alan Knight, 2012-09-20 23:29:59]

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/date_time_fo...
File pkg/intl/test/date_time_format_http_request_test.dart (right):

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/date_time_fo...
pkg/intl/test/date_time_format_http_request_test.dart:14: //
ExtraCommandArgs=pkg/intl/test/start_web_server.dart
On 2012/09/20 23:07:19, Emily Fortuna wrote:
> will this be removed after Graham has implemented his server-browser solution?

I would love to be able to remove it. It seems like Graham's solution isn't
intended to be usable in the buildbot tests, but if we can, I'd be happy to.
Added a TODO.

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
File pkg/intl/test/start_web_server.dart (right):

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
pkg/intl/test/start_web_server.dart:23: new Timer(1000, (t) => exit(0));
On 2012/09/20 23:07:19, Emily Fortuna wrote:
> Perhaps if we find ourselves doing more of these sorts of tests in the future,
> we may want to make some sort of polling solution.

Yes. This is pretty ugly. The delay isn't necessary in normal usage, but I
though it would be helpful to leave in for debugging if something breaks.

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

https://codereview.chromium.org/10959002/diff/5002/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:442: List _append(list1,list2) =>
[]..addAll(list1)..addAll(list2);
On 2012/09/20 23:07:19, Emily Fortuna wrote:
> woohoo! real-life use of method cascades!
> 
> Is there a reason why we don't just do 
> list1..addAll(list2)? Looks like commands will always be at least [] and it
> seems to only be called in one place. (in which case maybe you don't need to
> define the extra method?

It's called in one place, but inside a loop, so I can't use the destructive
addAll.  It could be commands.copy().addAll, if we had a copy method. I actually
thought about using filter((x)=>true) but thought that was more suitable for an
obfuscated code contest and defining the method was better.

[ahe, 2012-09-24 07:39:42]

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

https://codereview.chromium.org/10959002/diff/13002/tools/testing/dart/test_s...
tools/testing/dart/test_suite.dart:688: extraCommand = new Options().executable;
This is wrong. I'll change this to:
extraCommand = TestUtils.vmFileName(configuration);

See: https://codereview.chromium.org/10965060

[William Hesse, 2012-09-24 09:40:20]

LGTM, except that this is an ugly and fragile solution, that adds more special
cases, and commands inside comments in the scripts.  I am worried that others
will use this ExtraCommand capability to do more complicated, hacky tests.

But there does seem to be no other way to do this.  The alternative would seem
to be to add a special "RunHttpServer" comment to a test, rather than the more
general "ExtraCommand" capability.

[Alan Knight, 2012-09-24 18:55:54]

On 2012/09/24 09:40:20, William Hesse wrote:
> LGTM, except that this is an ugly and fragile solution, that adds more special
> cases, and commands inside comments in the scripts.  I am worried that others
> will use this ExtraCommand capability to do more complicated, hacky tests.
> 
> But there does seem to be no other way to do this.  The alternative would seem
> to be to add a special "RunHttpServer" comment to a test, rather than the more
> general "ExtraCommand" capability.

Yes, I agree it's not pretty, but it was the least bad solution I could come up
with. Alternatives like trying to use the http server that's likely built into
the buildbot machines anyway is not much prettier and doesn't handle Windows.
Putting the files on an external HTTP server is likely to make the tests slower
from other locations and introduces another point of failure.

I can actually think of two other possible (complicated, hacky) uses for this
mechanism.  One is for locale testing. It's difficult to effectively test
getting the locale from the operating system because you don't know what the
right answer is supposed to be. Trying to check what the right answer is
probably  goes through the same mechanisms you're trying to test, so is mostly
tautological.  So it might work to use an external command to e.g. set the LANG
environment variable to a known value. It would, however, be better if the two
processes were related and we were able to set it without risking that setting
affecting other processes. I'm not sure if that's true or not. The way that's
tested now is to set the internal value to a known invalid value, try to get the
OS value, and then verify that the internal value is now different and is
syntactically valid.

The second usage I thought of was the similar problem of time zones. I don't
know in advance what the time zone of the machine running the tests is, and so I
don't have a good way to test  the mechanism for getting it.  Right now we don't
have a mechanism anyway (I think we can find the current offset from UTC, but
that's all) so that's a future question.

[Mads Ager (google), 2012-10-01 14:28:08]

Alan,

I sympathize with the need to be able to get resources via an http server in
connection with tests. However, I'm uncomfortable with the current solution. I
don't think we want to be able to run any command via comments in the test code
and the way this is done doesn't work on Windows at this point. I would
therefore like to remove it as soon as possible so we do not get extra
dependencies on it.

I would like to propose another way of doing this: How about writing a simple
webserver in Dart that will serve up all of the dart repo. Then we can have a
flag on test suites telling us whether http access to the repo is needed. We can
then build in the start and shutdown of the server in the test_runner. The test
runner can spawn the http server for the repo in the beginning if any of the
test suites run need http access and the server will be alive for the life-time
of the test script.

What do you think?

Cheers,    -- Mads

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
File pkg/intl/test/start_web_server.dart (right):

https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
pkg/intl/test/start_web_server.dart:23: new Timer(1000, (t) => exit(0));
On 2012/09/20 23:29:59, Alan Knight wrote:
> On 2012/09/20 23:07:19, Emily Fortuna wrote:
> > Perhaps if we find ourselves doing more of these sorts of tests in the
future,
> > we may want to make some sort of polling solution.
> 
> Yes. This is pretty ugly. The delay isn't necessary in normal usage, but I
> though it would be helpful to leave in for debugging if something breaks.

You actually have the hooks to wait for the process to start:
p.onStart(callback). We should use that instead of a Timer. However, I would
prefer this code to not be necessary at all (see other comments).

[Alan Knight, 2012-10-01 22:46:13]

So, if I understand what you're suggesting, you'd have a flag on test suites to
indicate starting a web server, and hard-code it to be a web server, rather than
an arbitrary command. That seems reasonable. How would a flag on test suites be
indicated? How much functionality would you expect the web server to need? The
one I did, copied from one of our web site examples, is extremely minimal, but
that might be adequate for many uses.

There are other uses cases I had thought of for the ability to run commands like
testing or setting system attributes like environment variables (e.g. LANG) or
checking the time zone. However, I think those could be done in simpler ways,
like spawning a process from inside the Dart program under test.


On 2012/10/01 14:28:08, Mads Ager wrote:
> Alan,
> 
> I sympathize with the need to be able to get resources via an http server in
> connection with tests. However, I'm uncomfortable with the current solution. I
> don't think we want to be able to run any command via comments in the test
code
> and the way this is done doesn't work on Windows at this point. I would
> therefore like to remove it as soon as possible so we do not get extra
> dependencies on it.
> 
> I would like to propose another way of doing this: How about writing a simple
> webserver in Dart that will serve up all of the dart repo. Then we can have a
> flag on test suites telling us whether http access to the repo is needed. We
can
> then build in the start and shutdown of the server in the test_runner. The
test
> runner can spawn the http server for the repo in the beginning if any of the
> test suites run need http access and the server will be alive for the
life-time
> of the test script.
> 
> What do you think?
> 
> Cheers,    -- Mads
> 
>
https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
> File pkg/intl/test/start_web_server.dart (right):
> 
>
https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
> pkg/intl/test/start_web_server.dart:23: new Timer(1000, (t) => exit(0));
> On 2012/09/20 23:29:59, Alan Knight wrote:
> > On 2012/09/20 23:07:19, Emily Fortuna wrote:
> > > Perhaps if we find ourselves doing more of these sorts of tests in the
> future,
> > > we may want to make some sort of polling solution.
> > 
> > Yes. This is pretty ugly. The delay isn't necessary in normal usage, but I
> > though it would be helpful to leave in for debugging if something breaks.
> 
> You actually have the hooks to wait for the process to start:
> p.onStart(callback). We should use that instead of a Timer. However, I would
> prefer this code to not be necessary at all (see other comments).

[Mads Ager (google), 2012-10-02 05:29:36]

On 2012/10/01 22:46:13, Alan Knight wrote:
> So, if I understand what you're suggesting, you'd have a flag on test suites
to
> indicate starting a web server, and hard-code it to be a web server, rather
than
> an arbitrary command. That seems reasonable. How would a flag on test suites
be
> indicated? 

I would add an extra named optional parameter to the TestSuite constructors in
test_suite.dart. In test_runner.dart all test suites get added to the queue of
test suites using the addTestSuite method. That would be a good place to check
if the test suite needs a web server and start it at that point if it has not
been started yet.

> How much functionality would you expect the web server to need? The
> one I did, copied from one of our web site examples, is extremely minimal, but
> that might be adequate for many uses.

I think we should start as simple as possible and then do extensions as we need
them. Something that just makes sure that you cannot get to files outside of the
repo and otherwise just serves the files as simply as possible would be a great
start I think.

> There are other uses cases I had thought of for the ability to run commands
like
> testing or setting system attributes like environment variables (e.g. LANG) or
> checking the time zone. However, I think those could be done in simpler ways,
> like spawning a process from inside the Dart program under test.

I agree with this. I would rather have that done by spawning a process from the
test case itself.

> On 2012/10/01 14:28:08, Mads Ager wrote:
> > Alan,
> > 
> > I sympathize with the need to be able to get resources via an http server in
> > connection with tests. However, I'm uncomfortable with the current solution.
I
> > don't think we want to be able to run any command via comments in the test
> code
> > and the way this is done doesn't work on Windows at this point. I would
> > therefore like to remove it as soon as possible so we do not get extra
> > dependencies on it.
> > 
> > I would like to propose another way of doing this: How about writing a
simple
> > webserver in Dart that will serve up all of the dart repo. Then we can have
a
> > flag on test suites telling us whether http access to the repo is needed. We
> can
> > then build in the start and shutdown of the server in the test_runner. The
> test
> > runner can spawn the http server for the repo in the beginning if any of the
> > test suites run need http access and the server will be alive for the
> life-time
> > of the test script.
> > 
> > What do you think?
> > 
> > Cheers,    -- Mads
> > 
> >
>
https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
> > File pkg/intl/test/start_web_server.dart (right):
> > 
> >
>
https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
> > pkg/intl/test/start_web_server.dart:23: new Timer(1000, (t) => exit(0));
> > On 2012/09/20 23:29:59, Alan Knight wrote:
> > > On 2012/09/20 23:07:19, Emily Fortuna wrote:
> > > > Perhaps if we find ourselves doing more of these sorts of tests in the
> > future,
> > > > we may want to make some sort of polling solution.
> > > 
> > > Yes. This is pretty ugly. The delay isn't necessary in normal usage, but I
> > > though it would be helpful to leave in for debugging if something breaks.
> > 
> > You actually have the hooks to wait for the process to start:
> > p.onStart(callback). We should use that instead of a Timer. However, I would
> > prefer this code to not be necessary at all (see other comments).

[gram, 2012-10-02 16:26:37]

For a moment yesterday I thought I had a great solution - incorporate a web
server in unittest and be able to start it up in setup/teardown. Then I realised
that wouldn't work of course because the tests in question are running in the
browser. However, I do want to throw out a related option, which is to have a
mock HttpClient. I'm not sure if that would meet your needs, Alan, but it does
have the advantage that the tests can be self-contained and should execute
faster.


On 2012/10/02 05:29:36, Mads Ager wrote:
> On 2012/10/01 22:46:13, Alan Knight wrote:
> > So, if I understand what you're suggesting, you'd have a flag on test suites
> to
> > indicate starting a web server, and hard-code it to be a web server, rather
> than
> > an arbitrary command. That seems reasonable. How would a flag on test suites
> be
> > indicated? 
> 
> I would add an extra named optional parameter to the TestSuite constructors in
> test_suite.dart. In test_runner.dart all test suites get added to the queue of
> test suites using the addTestSuite method. That would be a good place to check
> if the test suite needs a web server and start it at that point if it has not
> been started yet.
> 
> > How much functionality would you expect the web server to need? The
> > one I did, copied from one of our web site examples, is extremely minimal,
but
> > that might be adequate for many uses.
> 
> I think we should start as simple as possible and then do extensions as we
need
> them. Something that just makes sure that you cannot get to files outside of
the
> repo and otherwise just serves the files as simply as possible would be a
great
> start I think.
> 
> > There are other uses cases I had thought of for the ability to run commands
> like
> > testing or setting system attributes like environment variables (e.g. LANG)
or
> > checking the time zone. However, I think those could be done in simpler
ways,
> > like spawning a process from inside the Dart program under test.
> 
> I agree with this. I would rather have that done by spawning a process from
the
> test case itself.
> 
> > On 2012/10/01 14:28:08, Mads Ager wrote:
> > > Alan,
> > > 
> > > I sympathize with the need to be able to get resources via an http server
in
> > > connection with tests. However, I'm uncomfortable with the current
solution.
> I
> > > don't think we want to be able to run any command via comments in the test
> > code
> > > and the way this is done doesn't work on Windows at this point. I would
> > > therefore like to remove it as soon as possible so we do not get extra
> > > dependencies on it.
> > > 
> > > I would like to propose another way of doing this: How about writing a
> simple
> > > webserver in Dart that will serve up all of the dart repo. Then we can
have
> a
> > > flag on test suites telling us whether http access to the repo is needed.
We
> > can
> > > then build in the start and shutdown of the server in the test_runner. The
> > test
> > > runner can spawn the http server for the repo in the beginning if any of
the
> > > test suites run need http access and the server will be alive for the
> > life-time
> > > of the test script.
> > > 
> > > What do you think?
> > > 
> > > Cheers,    -- Mads
> > > 
> > >
> >
>
https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
> > > File pkg/intl/test/start_web_server.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/10959002/diff/5002/pkg/intl/test/start_web_se...
> > > pkg/intl/test/start_web_server.dart:23: new Timer(1000, (t) => exit(0));
> > > On 2012/09/20 23:29:59, Alan Knight wrote:
> > > > On 2012/09/20 23:07:19, Emily Fortuna wrote:
> > > > > Perhaps if we find ourselves doing more of these sorts of tests in the
> > > future,
> > > > > we may want to make some sort of polling solution.
> > > > 
> > > > Yes. This is pretty ugly. The delay isn't necessary in normal usage, but
I
> > > > though it would be helpful to leave in for debugging if something
breaks.
> > > 
> > > You actually have the hooks to wait for the process to start:
> > > p.onStart(callback). We should use that instead of a Timer. However, I
would
> > > prefer this code to not be necessary at all (see other comments).