Make Safari tests more robust.

Make Safari tests more robust.

R=whesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/199806da039db6dd03a64737a18e0cc97ed5dd0f

[ahe, 2016-04-08 12:56:52]

ahe@google.com changed reviewers:
+ whesse@google.com

[ahe, 2016-04-08 12:56:52]

Sorry, this is very much work in progress. I've been running Safari tests like
this:

  caffeinate -d -i -m -s -u ./test_safari.sh

This CL is based on 27b2c36ec1dcfa017d588b9328bc7a71de971241 and subsequent CLs
may cause conflicts in co19-dart2js.status.

https://codereview.chromium.org/1871883002/diff/1/nuke_safari.sh
File nuke_safari.sh (right):

https://codereview.chromium.org/1871883002/diff/1/nuke_safari.sh#newcode1
nuke_safari.sh:1: #!/bin/bash
I'm not sure if this should be a script like this, or if it should be rolled
into browser_controller.dart.

https://codereview.chromium.org/1871883002/diff/1/test_safari.sh
File test_safari.sh (right):

https://codereview.chromium.org/1871883002/diff/1/test_safari.sh#newcode5
test_safari.sh:5: if lsof -t /Applications/Safari.app/Contents/MacOS/Safari ;
then
Not sure how to integrate this test and the next into browser_controller.dart.
Perhaps these checks should be moved to nuke_safari.sh.

https://codereview.chromium.org/1871883002/diff/1/tools/testing/dart/browser_...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/1871883002/diff/1/tools/testing/dart/browser_...
tools/testing/dart/browser_controller.dart:1466: // DO NOT SUBMIT: Should only
happen when starting Safari, and ideally be
Bill, I hope you have an idea for how to integrate this in
browser_controller.dart?

[ahe, 2016-04-18 15:02:57]

Please take a look, I've cleaned up the code and I'm ready for a thorough
review.

[Bill Hesse, 2016-04-25 08:20:19]

https://codereview.chromium.org/1871883002/diff/60001/tools/safari_factory_re...
File tools/safari_factory_reset.py (right):

https://codereview.chromium.org/1871883002/diff/60001/tools/safari_factory_re...
tools/safari_factory_reset.py:18: dart_test_script = string.join([tools_dir,
dart_script_name], os.sep)
I would join tools_dir in the os.path.join in the line above.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
This is not rebased to the current version of browser_controller, which has many
changes.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:287: // DO NOT SUBMIT.
This comment says "do not submit".  Should we remove this function, and calls to
it, completely?

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:304: Zone parent = Zone.current;
Add a comment for why we are using the zone - is it only to change printing to
logging, or is it also giving error handling that we couldn't get another way.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:890: * case we wish to have a testing
server with different behavior (such as the
If we are removing the testingServer argument, then remove this comment too.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:1412: if (isStartup && requiresFocus)
{
Could we put this into the BrowserController object instead, and have it called
by getDriverPage?  That would mean the subprocess is created, and waited on,
before the page is written to the client, instead of after the page is written
but before the response is closed, but that should not matter.

I like having the platform-specific code in the BrowserController, and keeping
the testing server very generic.

If this is moved, then maybe we have a Browser object when handling it, so the
"requiresFocus" could become a getter instead of static, so the Safari class
could just override it with true.  That would also be a plus. The code creating
the subprocess could even move to the Safari class, which would be much much
better.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:1487: 
I think this blank line should go.  Do we run this file through the formatter?

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/rese...
File tools/testing/dart/reset_safari.dart (right):

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/rese...
tools/testing/dart/reset_safari.dart:45: const Duration defaultPollDelay = const
Duration(milliseconds: 1);
I'm surprised this is so small.  But I grew up with an 18.2 Hz timer.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/rese...
tools/testing/dart/reset_safari.dart:226: await killAndResetSafari();
Could we pass in "print" to killAndResetSafari here, and pass in the logger
function when we call it from the browser controller?  The closure could even be
put in a global variable, so you don't need a class or to pass it to all the
functions.  It could even be exposed by the statement importing this library.

This is only important if we only need the zone for this reason. If we also need
the zone for error handling, it is not as important to make this change.

[ahe, 2016-05-24 08:44:42]

Thank you, Bill. Please take another look.

https://codereview.chromium.org/1871883002/diff/60001/tools/safari_factory_re...
File tools/safari_factory_reset.py (right):

https://codereview.chromium.org/1871883002/diff/60001/tools/safari_factory_re...
tools/safari_factory_reset.py:18: dart_test_script = string.join([tools_dir,
dart_script_name], os.sep)
On 2016/04/25 08:20:18, Bill Hesse wrote:
> I would join tools_dir in the os.path.join in the line above.

Done.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2016/04/25 08:20:18, Bill Hesse wrote:
> This is not rebased to the current version of browser_controller, which has
many
> changes.

Done.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:287: // DO NOT SUBMIT.
On 2016/04/25 08:20:18, Bill Hesse wrote:
> This comment says "do not submit".  Should we remove this function, and calls
to
> it, completely?

Done.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:304: Zone parent = Zone.current;
On 2016/04/25 08:20:18, Bill Hesse wrote:
> Add a comment for why we are using the zone - is it only to change printing to
> logging, or is it also giving error handling that we couldn't get another way.

Done.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:890: * case we wish to have a testing
server with different behavior (such as the
On 2016/04/25 08:20:18, Bill Hesse wrote:
> If we are removing the testingServer argument, then remove this comment too.

Done.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:1412: if (isStartup && requiresFocus)
{
On 2016/04/25 08:20:19, Bill Hesse wrote:
> Could we put this into the BrowserController object instead, and have it
called
> by getDriverPage?  That would mean the subprocess is created, and waited on,
> before the page is written to the client, instead of after the page is written
> but before the response is closed, but that should not matter.
> 
> I like having the platform-specific code in the BrowserController, and keeping
> the testing server very generic.
> 
> If this is moved, then maybe we have a Browser object when handling it, so the
> "requiresFocus" could become a getter instead of static, so the Safari class
> could just override it with true.  That would also be a plus. The code
creating
> the subprocess could even move to the Safari class, which would be much much
> better.

Done.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:1487: 
On 2016/04/25 08:20:19, Bill Hesse wrote:
> I think this blank line should go.  Do we run this file through the formatter?

Done.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/rese...
File tools/testing/dart/reset_safari.dart (right):

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/rese...
tools/testing/dart/reset_safari.dart:45: const Duration defaultPollDelay = const
Duration(milliseconds: 1);
On 2016/04/25 08:20:19, Bill Hesse wrote:
> I'm surprised this is so small.  But I grew up with an 18.2 Hz timer.

Acknowledged.

https://codereview.chromium.org/1871883002/diff/60001/tools/testing/dart/rese...
tools/testing/dart/reset_safari.dart:226: await killAndResetSafari();
On 2016/04/25 08:20:19, Bill Hesse wrote:
> Could we pass in "print" to killAndResetSafari here, and pass in the logger
> function when we call it from the browser controller?  The closure could even
be
> put in a global variable, so you don't need a class or to pass it to all the
> functions.  It could even be exposed by the statement importing this library.
> 
> This is only important if we only need the zone for this reason. If we also
need
> the zone for error handling, it is not as important to make this change.

Acknowledged.

[ahe, 2016-05-31 06:02:43]

ping

[Bill Hesse, 2016-05-31 06:43:02]

On 2016/05/31 06:02:43, ahe wrote:
> ping

ack

[ahe, 2016-06-03 15:58:31]

ping

[ahe, 2016-06-06 07:11:58]

ping

[Bill Hesse, 2016-06-09 17:35:42]

LGTM.

https://codereview.chromium.org/1871883002/diff/100001/tools/testing/dart/bro...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/1871883002/diff/100001/tools/testing/dart/bro...
tools/testing/dart/browser_controller.dart:285: Future<Null>
notifyGetDriverPage() => new Future<Null>.value();
This could use a comment as explanation, that this signals when the driver page
is requested, so actions (like foregrounding the browser) can be taken.  Would a
better name be onDriverPageRequested?

https://codereview.chromium.org/1871883002/diff/100001/tools/testing/dart/tes...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/1871883002/diff/100001/tools/testing/dart/tes...
tools/testing/dart/test_options.dart:341: 'reset-browser-configuration',
All the internal names of flags have (or should have) underscores, not hyphens. 
That is the standard we have been using in the rest of the file.

[ahe, 2016-06-15 08:16:19]

Description was changed from

==========
Make Safari tests more robust.
==========

to

==========
Make Safari tests more robust.

R=whesse@google.com

Committed:
https://github.com/dart-lang/sdk/commit/199806da039db6dd03a64737a18e0cc97ed5dd0f
==========

[ahe, 2016-06-15 08:16:21]

Committed patchset #7 (id:120001) manually as
199806da039db6dd03a64737a18e0cc97ed5dd0f (presubmit successful).

[ahe, 2016-06-15 08:16:54]

Thank you, Bill.

https://codereview.chromium.org/1871883002/diff/100001/tools/testing/dart/bro...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/1871883002/diff/100001/tools/testing/dart/bro...
tools/testing/dart/browser_controller.dart:285: Future<Null>
notifyGetDriverPage() => new Future<Null>.value();
On 2016/06/09 17:35:42, Bill Hesse wrote:
> This could use a comment as explanation, that this signals when the driver
page
> is requested, so actions (like foregrounding the browser) can be taken.  Would
a
> better name be onDriverPageRequested? 

Done.

https://codereview.chromium.org/1871883002/diff/100001/tools/testing/dart/tes...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/1871883002/diff/100001/tools/testing/dart/tes...
tools/testing/dart/test_options.dart:341: 'reset-browser-configuration',
On 2016/06/09 17:35:42, Bill Hesse wrote:
> All the internal names of flags have (or should have) underscores, not
hyphens. 
> That is the standard we have been using in the rest of the file.

Done.