- Add UI test for the V8 Benchmark Suite....

- Add UI test for the V8 Benchmark Suite.
- Modify the V8 Benchmark Suite to work with the UI test framework.
- Add utils for JavaScript UI tests.
- Move automation.js files for SunSpider and V8 Benchmark to the
  src/chrome/test/ui directory.
- Overload UITest::PrintResult for convenience.

BUG=9255
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12823

[Patrick Johnson, 2009-03-26 01:14:48]

Sorry to keep sending these reviews to you.  I thought you would be in a good
position to review this since you already reviewed my SunSpider test, which is
very similar.

http://codereview.chromium.org/42628/diff/1/2
File chrome/test/data/v8_benchmark/README.chromium (right):

http://codereview.chromium.org/42628/diff/1/2#newcode14
Line 14: - Added json2.js, a parser/stringifier for JSON in JavaScript. This is
used
Now that I've used this file a few times, I plan to move it to a common
location.  I'll do that in a follow-up change.

http://codereview.chromium.org/42628/diff/1/7
File chrome/test/ui/v8_benchmark_uitest.cc (right):

http://codereview.chromium.org/42628/diff/1/7#newcode80
Line 80: bool GetResults(TabProxy* tab, ResultsMap* results) {
I know that much of this code is the same or very similar to
sunspider_uitest.cc.  But I'm not sure if it should be extracted out into a
common location.  I tend to think that might be a bit overzealous at this point.
 For one, what would that common location be?

I'm interested to hear your opinion on this.

http://codereview.chromium.org/42628/diff/1/7#newcode139
Line 139: PrintResult("score", "", trace_name, score, "", true);
Note the lack of a unit.  The V8 Benchmark Suite does not deal in mortal units,
it just uses numerical scores.  Will this be okay or should I add a "dummy"
unit?

Also interesting, in this test suite bigger is better, contrary to (I think) all
of the other perf tests that we graph.  I'm not sure how we should convey that
in the graphs (maybe add a note).

[Pam (message me for reviews), 2009-03-26 17:09:30]

http://codereview.chromium.org/42628/diff/1/2
File chrome/test/data/v8_benchmark/README.chromium (right):

http://codereview.chromium.org/42628/diff/1/2#newcode14
Line 14: - Added json2.js, a parser/stringifier for JSON in JavaScript. This is
used
On 2009/03/26 01:14:49, Patrick Johnson wrote:
> Now that I've used this file a few times, I plan to move it to a common
> location.  I'll do that in a follow-up change.

OK, sounds good.

http://codereview.chromium.org/42628/diff/1/3
File chrome/test/data/v8_benchmark/automation.js (right):

http://codereview.chromium.org/42628/diff/1/3#newcode7
Line 7: // Automation utilities.
Although this is in test/data/, it isn't actually data. Is there a better place
for it?

http://codereview.chromium.org/42628/diff/1/7
File chrome/test/ui/v8_benchmark_uitest.cc (right):

http://codereview.chromium.org/42628/diff/1/7#newcode74
Line 74: return false;
Same comment about this construction here as in the last one.

http://codereview.chromium.org/42628/diff/1/7#newcode80
Line 80: bool GetResults(TabProxy* tab, ResultsMap* results) {
On 2009/03/26 01:14:49, Patrick Johnson wrote:
> I know that much of this code is the same or very similar to
> sunspider_uitest.cc.  But I'm not sure if it should be extracted out into a
> common location.  I tend to think that might be a bit overzealous at this
point.
>  For one, what would that common location be?
> 
> I'm interested to hear your opinion on this.

ui_test.cc has some code common to a bunch of UI tests. That might be too broad
for only two tests, though. (Is any of this also common to other automation-type
tests we already had?) I don't know of a file more specific to this sort of
externally-driven test, but it would be fine to create one alongside
ui_tests.cc.

http://codereview.chromium.org/42628/diff/1/7#newcode139
Line 139: PrintResult("score", "", trace_name, score, "", true);
On 2009/03/26 01:14:49, Patrick Johnson wrote:
> Note the lack of a unit.  The V8 Benchmark Suite does not deal in mortal
units,
> it just uses numerical scores.  Will this be okay or should I add a "dummy"
> unit?

I think it'll use "thing-a-ma-bobs" for an empty unit. Which is fine, or calling
it "points" or "score" or something else equally dull :) would be fine too.

> Also interesting, in this test suite bigger is better, contrary to (I think)
all
> of the other perf tests that we graph.  I'm not sure how we should convey that
> in the graphs (maybe add a note).

Since the units are only there for humans to look at, maybe that's where to put
"(bigger is better)".

[Patrick Johnson, 2009-03-30 18:09:57]

http://codereview.chromium.org/42628/diff/1/3
File chrome/test/data/v8_benchmark/automation.js (right):

http://codereview.chromium.org/42628/diff/1/3#newcode7
Line 7: // Automation utilities.
On 2009/03/26 17:09:30, Pam wrote:
> Although this is in test/data/, it isn't actually data. Is there a better
place
> for it?

Maybe test/scripts or test/ui/scripts?  Since it needs to be included by the
files in test/data, I'm a little hesitant to make the path dependencies too
complex.

http://codereview.chromium.org/42628/diff/1/7
File chrome/test/ui/v8_benchmark_uitest.cc (right):

http://codereview.chromium.org/42628/diff/1/7#newcode74
Line 74: return false;
On 2009/03/26 17:09:30, Pam wrote:
> Same comment about this construction here as in the last one.

Done.

http://codereview.chromium.org/42628/diff/1/7#newcode80
Line 80: bool GetResults(TabProxy* tab, ResultsMap* results) {
On 2009/03/26 17:09:30, Pam wrote:
> On 2009/03/26 01:14:49, Patrick Johnson wrote:
> > I know that much of this code is the same or very similar to
> > sunspider_uitest.cc.  But I'm not sure if it should be extracted out into a
> > common location.  I tend to think that might be a bit overzealous at this
> point.
> >  For one, what would that common location be?
> > 
> > I'm interested to hear your opinion on this.
> 
> ui_test.cc has some code common to a bunch of UI tests. That might be too
broad
> for only two tests, though. (Is any of this also common to other
automation-type
> tests we already had?)

I don't think so, I haven't seen anything much like this in our case base.  The
ExecuteAndExtract* functions don't seem to be used much, despite how useful they
are.

> I don't know of a file more specific to this sort of
> externally-driven test, but it would be fine to create one alongside
> ui_tests.cc.
> 

I created script_tets_util.{h,cc} -- please let me know if that seems reasonable
to you.

http://codereview.chromium.org/42628/diff/1/7#newcode139
Line 139: PrintResult("score", "", trace_name, score, "", true);
On 2009/03/26 17:09:30, Pam wrote:
> On 2009/03/26 01:14:49, Patrick Johnson wrote:
> > Note the lack of a unit.  The V8 Benchmark Suite does not deal in mortal
> units,
> > it just uses numerical scores.  Will this be okay or should I add a "dummy"
> > unit?
> 
> I think it'll use "thing-a-ma-bobs" for an empty unit.

Ah, yes.  Now I remember.  Which joker made that the default? :-)

> Which is fine, or calling
> it "points" or "score" or something else equally dull :) would be fine too.
> 
> > Also interesting, in this test suite bigger is better, contrary to (I think)
> all
> > of the other perf tests that we graph.  I'm not sure how we should convey
that
> > in the graphs (maybe add a note).
> 
> Since the units are only there for humans to look at, maybe that's where to
put
> "(bigger is better)".

Good idea.  I did that.

[Patrick Johnson, 2009-03-30 18:48:41]

http://codereview.chromium.org/42628/diff/1/7
File chrome/test/ui/v8_benchmark_uitest.cc (right):

http://codereview.chromium.org/42628/diff/1/7#newcode80
Line 80: bool GetResults(TabProxy* tab, ResultsMap* results) {
On 2009/03/30 18:09:58, Patrick Johnson wrote:
> I don't think so, I haven't seen anything much like this in our case base.

"case base" --> "code base"

[Pam (message me for reviews), 2009-03-30 21:42:07]

Just a few minor points.

http://codereview.chromium.org/42628/diff/1/3
File chrome/test/data/v8_benchmark/automation.js (right):

http://codereview.chromium.org/42628/diff/1/3#newcode7
Line 7: // Automation utilities.
On 2009/03/30 18:09:58, Patrick Johnson wrote:
> On 2009/03/26 17:09:30, Pam wrote:
> > Although this is in test/data/, it isn't actually data. Is there a better
> place
> > for it?
> 
> Maybe test/scripts or test/ui/scripts?  Since it needs to be included by the
> files in test/data, I'm a little hesitant to make the path dependencies too
> complex.

Maybe even just test/ui, alongside the C++ source files there?

http://codereview.chromium.org/42628/diff/2015/3028
File chrome/test/ui/script_test_util.h (right):

http://codereview.chromium.org/42628/diff/2015/3028#newcode12
Line 12: // that interact with JavaScript.
The names of these files are not clear to me. What about something like
javascript_test_util or json_test_util?

http://codereview.chromium.org/42628/diff/2015/3024
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/42628/diff/2015/3024#newcode366
Line 366: LOG(INFO) << "BROWSER_WRAPPER was set, prefixing command_line with "
<< browser_wrapper;
nit: 80 col

[Patrick Johnson, 2009-03-30 22:54:32]

http://codereview.chromium.org/42628/diff/1/3
File chrome/test/data/v8_benchmark/automation.js (right):

http://codereview.chromium.org/42628/diff/1/3#newcode7
Line 7: // Automation utilities.
On 2009/03/30 21:42:07, Pam wrote:
> On 2009/03/30 18:09:58, Patrick Johnson wrote:
> > On 2009/03/26 17:09:30, Pam wrote:
> > > Although this is in test/data/, it isn't actually data. Is there a better
> > place
> > > for it?
> > 
> > Maybe test/scripts or test/ui/scripts?  Since it needs to be included by the
> > files in test/data, I'm a little hesitant to make the path dependencies too
> > complex.
> 
> Maybe even just test/ui, alongside the C++ source files there?

Sure, seems reasonable.

http://codereview.chromium.org/42628/diff/2015/3028
File chrome/test/ui/script_test_util.h (right):

http://codereview.chromium.org/42628/diff/2015/3028#newcode12
Line 12: // that interact with JavaScript.
On 2009/03/30 21:42:07, Pam wrote:
> The names of these files are not clear to me. What about something like
> javascript_test_util or json_test_util?

Okay, went with javascript_test_util.

http://codereview.chromium.org/42628/diff/2015/3024
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/42628/diff/2015/3024#newcode366
Line 366: LOG(INFO) << "BROWSER_WRAPPER was set, prefixing command_line with "
<< browser_wrapper;
On 2009/03/30 21:42:07, Pam wrote:
> nit: 80 col

Fixed, thanks for noticing this.

[Pam (message me for reviews), 2009-03-30 22:58:25]

LGTM!

- Pam