Replace run_selenium.py with Dart script that uses the browser controller for performance tests.

Replace run_selenium.py with Dart script that uses the browser controller for performance tests.

BUG=
R=kustermann@google.com, ricow@google.com, vsm@google.com

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

[Emily Fortuna, 2014-02-26 19:33:33]

This is the first step (of several) to switch performance testing over to use
the Dart browser controller instead of webdriver. 

Basically tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart
replicates the behavior of what is in run_selenium.py

Rico, Martin, I had to make two minor tweaks to the testing framework to work
with this, so I'm including you on this review. You only need to look at
tools/testing/dart/http_server.dart and
tools/testing/dart/browser_controller.dart

The actual hooking up of this functionality with the perf running script will
come in a later CL.

[ricow1, 2014-02-27 08:12:12]

The two files LGTM, could you add a comment why we need it for completeness?

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

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_controller.dart:1165: });
thank you

[kustermann, 2014-02-27 09:37:15]

http_server/browser_controller: LGTM

Ad mentioned in a comment, please add an import to
"dart/tests/standalone/io/test_harness_analyzer_test.dart", so we run the
analyzer to ensure we don't break it.

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
File tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart
(right):

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart:1: //
Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
Please add an import to this file into
"dart/tests/standalone/io/test_harness_analyzer_test.dart".

Since you're using our browser controller, we want to run the analyzer on this
file, to ensure that we don't break it.

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart:30: var
results = _parse_arguments(args);
results -> options/config ?

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart:44: Map
_parse_arguments(List<String> args) {
_parse_arguments -> _parseArguments ?

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart:144:
return result;
I don't understand why you want to modify this behavior. I think something like
this should be possible:

var urlToBenchmarkHtml = ...;

var browserTest = new BrowserTest(urlToBenchmarkHtml, (BrowserTestOutput output)
{ 
  if (!output.didTimeout) {
    // Read ${output.message} and extract results from it
  } else {
    // print failure message
  }
});

var runner = new BrowserTestRunner(...);
runner.queueTest(browserTest);

The only thing that is necessary is that the perf test html file (i.e.
[urlToBenchmarkHtml]) behaves according to the protocol described in
tools/testing/dart/test_controller.js:

  5 /*
  6  * The communication protocol between test_controller.js and the driving
  7  * page are JSON encoded messages of the following form:
  8  *   message = {
  9  *      is_first_message: true/false,
 10  *      is_status_update: true/false,
 11  *      is_done: true/false,
 12  *      message: message_content,
 13  *   }
 14  *
 15  * The first message should have [is_first_message] set, the last message
 16  * should have [is_done] set. Status updates should have [is_status_update]
set.
 17  *
 18  * The [message_content] can be be any content. In our case it will a list
of
 19  * events encoded in JSON. See the next comment further down about what an
event
 20  * is.
 21  */

[You **don't** have to use test_controller.js, implementing this communication
protocol is enough.]

So the perf test html only needs to do this:

  window.parent.postMessage(JSON.stringify({
    message: ...,
    is_first_message: ...,
    is_status_update: ...,
    is_done: ...,
  })

I've implemented this on purpose, so the browser controller can be used for
anything (running tests, running benchmarks, other automated things):
- You give it an html file and it will run it.
- The html file can give you status/progress updates if it wants to.
- Once the html file is done, it'll post back a message (can be arbitarily
encoded JSON e.g.) with {is_done: true}
- The browser controller will give you this message.

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/perf...
File tools/testing/dart/perf_test_controller.js (right):

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/perf...
tools/testing/dart/perf_test_controller.js:9:
dartPrint('unittest-suite-wait-for-done');
I have no idea why you would need this. Why can't you have a base class for all
benchmarks, which prints this before starting the benchmark and prints the
corresponding done message once it's done?

[Emily Fortuna, 2014-02-27 19:27:48]

+ vsm for dromaeo updates.

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
File tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart
(right):

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart:1: //
Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
On 2014/02/27 09:37:15, kustermann wrote:
> Please add an import to this file into
> "dart/tests/standalone/io/test_harness_analyzer_test.dart".
> 
> Since you're using our browser controller, we want to run the analyzer on this
> file, to ensure that we don't break it.

Done.

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart:30: var
results = _parse_arguments(args);
On 2014/02/27 09:37:15, kustermann wrote:
> results -> options/config ?

Done.

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart:44: Map
_parse_arguments(List<String> args) {
On 2014/02/27 09:37:15, kustermann wrote:
> _parse_arguments -> _parseArguments ?

Thanks. Switching between the Python script and Dart is doing crazy things to my
style!

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...
tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart:144:
return result;
On 2014/02/27 09:37:15, kustermann wrote:
> I don't understand why you want to modify this behavior. I think something
like
> this should be possible:
> 
> var urlToBenchmarkHtml = ...;
> 
> var browserTest = new BrowserTest(urlToBenchmarkHtml, (BrowserTestOutput
output)
> { 
>   if (!output.didTimeout) {
>     // Read ${output.message} and extract results from it
>   } else {
>     // print failure message
>   }
> });
> 
> var runner = new BrowserTestRunner(...);
> runner.queueTest(browserTest);
> 
> The only thing that is necessary is that the perf test html file (i.e.
> [urlToBenchmarkHtml]) behaves according to the protocol described in
> tools/testing/dart/test_controller.js:
> 
>   5 /*
>   6  * The communication protocol between test_controller.js and the driving
>   7  * page are JSON encoded messages of the following form:
>   8  *   message = {
>   9  *      is_first_message: true/false,
>  10  *      is_status_update: true/false,
>  11  *      is_done: true/false,
>  12  *      message: message_content,
>  13  *   }
>  14  *
>  15  * The first message should have [is_first_message] set, the last message
>  16  * should have [is_done] set. Status updates should have
[is_status_update]
> set.
>  17  *
>  18  * The [message_content] can be be any content. In our case it will a list
> of
>  19  * events encoded in JSON. See the next comment further down about what an
> event
>  20  * is.
>  21  */
> 
> [You **don't** have to use test_controller.js, implementing this communication
> protocol is enough.]
> 
> So the perf test html only needs to do this:
> 
>   window.parent.postMessage(JSON.stringify({
>     message: ...,
>     is_first_message: ...,
>     is_status_update: ...,
>     is_done: ...,
>   })
> 
> I've implemented this on purpose, so the browser controller can be used for
> anything (running tests, running benchmarks, other automated things):
> - You give it an html file and it will run it.
> - The html file can give you status/progress updates if it wants to.
> - Once the html file is done, it'll post back a message (can be arbitarily
> encoded JSON e.g.) with {is_done: true}
> - The browser controller will give you this message.

The reason I had to modify this is your protocol assumes that no other messages
of any other form are sent. Which in the case of Dromaeo is not the case.
Dromaeo uses message passing within its own test, and the driver page throws a
fit if it receives messages that are not in that form, so I modified the driver
page to not complain. 

Could I have written my own version that doesn't use browser_controller.js? You
bet. But the code is SO SIMILAR with one minor adjustment, it seemed silly to
basically copy/paste everything and change two lines. The performance tests are
also run as correctness tests to ensure that they don't break, so I don't want
to make changes directly to the test itself -- it should be able to run both as
a performance test AND a correctness test. So I add this helper script +
perf_test_controller.js around the original test afterwards, so as not to modify
the original script.

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/perf...
File tools/testing/dart/perf_test_controller.js (right):

https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/perf...
tools/testing/dart/perf_test_controller.js:9:
dartPrint('unittest-suite-wait-for-done');
On 2014/02/27 09:37:15, kustermann wrote:
> I have no idea why you would need this. Why can't you have a base class for
all
> benchmarks, which prints this before starting the benchmark and prints the
> corresponding done message once it's done?

See my comment at the bottom of this file:
https://codereview.chromium.org/178223008/diff/200001/tools/testing/dart/brow...

But the short version is this is the "wrapper"/glue code around the test to make
it a perf test. All perf tests include this. I don't want to manually modify the
test because the test is also run as a correctness test.

https://codereview.chromium.org/178223008/diff/220001/tests/standalone/io/tes...
File tests/standalone/io/test_harness_analyzer_test.dart (right):

https://codereview.chromium.org/178223008/diff/220001/tests/standalone/io/tes...
tests/standalone/io/test_harness_analyzer_test.dart:7: import
'../../../tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart'
as browser_perf_testing;
Martin, how do I run this test to verify this is working?

[kustermann, 2014-02-28 08:22:53]

https://codereview.chromium.org/178223008/diff/220001/tests/standalone/io/tes...
File tests/standalone/io/test_harness_analyzer_test.dart (right):

https://codereview.chromium.org/178223008/diff/220001/tests/standalone/io/tes...
tests/standalone/io/test_harness_analyzer_test.dart:7: import
'../../../tools/testing/dart/browser_perf_testing/lib/browser_perf_testing.dart'
as browser_perf_testing;
On 2014/02/27 19:27:48, Emily Fortuna wrote:
> Martin, how do I run this test to verify this is working?

This is a normal test. It will pass on every configuration, since main is empty.
But the analyzer will nonetheless analyze the transitive import closure of this
file.

./tools/test.py -mrelease -cdartanalyzer -rnone
standalone/io/test_harness_analyzer_test

[This is running on our normal analyzer bots.]

[vsm, 2014-02-28 18:40:47]

lgtm for dromaeo changes

https://codereview.chromium.org/178223008/diff/260001/tools/testing/perf_test...
File tools/testing/perf_testing/run_perf_tests.py (right):

https://codereview.chromium.org/178223008/diff/260001/tools/testing/perf_test...
tools/testing/perf_testing/run_perf_tests.py:744: file_path =
'/'.join(['/root_dart', dromaeo_path, 'index%s.html?%s'% (
Is the file_path really a url?  If so, maybe re-name to make clear.

The old code is using the os separator.

[Emily Fortuna, 2014-02-28 21:06:37]

Committed patchset #5 manually as r33171 (presubmit successful).

[Emily Fortuna, 2014-02-28 21:09:17]

@Martin, if you have suggestions for how to better integrate with
browser_controller.dart given the postMessage and correctness test constraints,
I'm happy to make follow up modifications. I agree the current state is not
ideal as-is.

https://codereview.chromium.org/178223008/diff/260001/tools/testing/perf_test...
File tools/testing/perf_testing/run_perf_tests.py (right):

https://codereview.chromium.org/178223008/diff/260001/tools/testing/perf_test...
tools/testing/perf_testing/run_perf_tests.py:744: file_path =
'/'.join(['/root_dart', dromaeo_path, 'index%s.html?%s'% (
On 2014/02/28 18:40:48, vsm wrote:
> Is the file_path really a url?  If so, maybe re-name to make clear.
> 
> The old code is using the os separator.

Yes, it's a URL, which is why I stopped using the os.separator, but I renamed it
now as well. :-)

[kustermann, 2014-03-05 10:44:41]

On 2014/02/28 21:09:17, Emily Fortuna wrote:
> @Martin, if you have suggestions for how to better integrate with
> browser_controller.dart given the postMessage and correctness test
constraints,
> I'm happy to make follow up modifications. I agree the current state is not
> ideal as-is.
> 

Actually I do have a suggestion on how to solve it. The browser controller
should ignore all messages, except the ones ment for it. Here's a CL with the
changes to the browser controller:
https://codereview.chromium.org/177333004/

This was on my plate for a while, now I finally did a CL for it.