Final touches for running smoketests.

Final touches for running smoketests. On buildbot, run 
python create_graph.py -v -b -l
Committed: https://code.google.com/p/dart/source/detail?r=2491

[Emily Fortuna, 2011-12-15 01:22:02]

PTAL! :-)

[Siggi Cherem (dart-lang), 2011-12-15 19:37:33]

overall looks good!

Only a few minor nits

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
File tools/testing/perf_testing/smoketest/BenchmarkBase.dart (right):

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:6: * The superclass from
which all benchmarks inherit from.
nit: we tend to do the 1-line style doc (/** comment */) when the comment fits
in one line.
(here and below)

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:9: /* Benchmark name. */
nit: -> add one more * (/**) 
(here and below)

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:93: /**
nit: + empty line

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:112: static
BenchmarkSuite get ONLY() {
now that we have top-level getters, this would look nicer at the top level of
the library. for instance:

BenchmarkSuite get BENCHMARK_SUITE() {
 ...
}

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:129:
runBenchmarksHelper(benchmarks) {
here a type for the argument would be helpful (e.g. List<Function>)

do you need to pass this argument? (benchmarks is also a field in this class)

If you do, can this be a static method? can you name the argument differently so
that it doesn't shadow the field name?

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:172: static
BenchmarkView get ONLY() {
same here (move to top-level)

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
File tools/testing/perf_testing/smoketest/Smoketest.dart (right):

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/Smoketest.dart:11: class Smoketest extends
BenchmarkBase {
+ comment, e.g. this is a sample benchmark currently used to simply detect that
things are not terribly broken in FF, etc...

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
File tools/testing/perf_testing/smoketest/dartWebBase.dart (right):

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/dartWebBase.dart:13:
BenchmarkSuite.ONLY.runBenchmarks();
you should be able to use method-binding for this:
window.setTimeout(BenchmarkSuite.ONLY.runBenchmarks)

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/dartWebBase.dart:14: }, 200);
This might be flaky. Could we wait for the DOMContentLoaded event? e.g.

window.on.contentLoaded.add(BenchmarkSuite.ONLY.runBenchmarks))
});

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
File tools/testing/perf_testing/smoketest/smoketest_frog.html (right):

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/smoketest_frog.html:10: <meta
http-equiv="Content-Type" content="text/html;charset=iso-8859-1">
nit: some additional indentation might be useful in this file. At least to see
the begin and end of head and body

[Emily Fortuna, 2011-12-15 23:16:09]

PTAL2!

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
File tools/testing/perf_testing/smoketest/BenchmarkBase.dart (right):

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:6: * The superclass from
which all benchmarks inherit from.
On 2011/12/15 19:37:33, sigmund wrote:
> nit: we tend to do the 1-line style doc (/** comment */) when the comment fits
> in one line.
> (here and below)

Done.

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:9: /* Benchmark name. */
On 2011/12/15 19:37:33, sigmund wrote:
> nit: -> add one more * (/**) 
> (here and below)

Done.

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:93: /**
On 2011/12/15 19:37:33, sigmund wrote:
> nit: + empty line

Done.

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:112: static
BenchmarkSuite get ONLY() {
On 2011/12/15 19:37:33, sigmund wrote:
> now that we have top-level getters, this would look nicer at the top level of
> the library. for instance:
> 
> BenchmarkSuite get BENCHMARK_SUITE() {
>  ...
> }

Done.

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:129:
runBenchmarksHelper(benchmarks) {
I need to pass this argument so that there are brief breaks of control between
each benchmark. See comment on line 134. 

Other comments: fixed.

On 2011/12/15 19:37:33, sigmund wrote:
> here a type for the argument would be helpful (e.g. List<Function>)
> 
> do you need to pass this argument? (benchmarks is also a field in this class)
> 
> If you do, can this be a static method? can you name the argument differently
so
> that it doesn't shadow the field name?

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/BenchmarkBase.dart:172: static
BenchmarkView get ONLY() {
On 2011/12/15 19:37:33, sigmund wrote:
> same here (move to top-level)

Done.

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
File tools/testing/perf_testing/smoketest/Smoketest.dart (right):

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/Smoketest.dart:11: class Smoketest extends
BenchmarkBase {
On 2011/12/15 19:37:33, sigmund wrote:
> + comment, e.g. this is a sample benchmark currently used to simply detect
that
> things are not terribly broken in FF, etc...

Done.

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
File tools/testing/perf_testing/smoketest/dartWebBase.dart (right):

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/dartWebBase.dart:13:
BenchmarkSuite.ONLY.runBenchmarks();
On 2011/12/15 19:37:33, sigmund wrote:
> you should be able to use method-binding for this:
> window.setTimeout(BenchmarkSuite.ONLY.runBenchmarks)

Done.

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
File tools/testing/perf_testing/smoketest/smoketest_frog.html (right):

http://codereview.chromium.org/8890091/diff/8002/tools/testing/perf_testing/s...
tools/testing/perf_testing/smoketest/smoketest_frog.html:10: <meta
http-equiv="Content-Type" content="text/html;charset=iso-8859-1">
On 2011/12/15 19:37:33, sigmund wrote:
> nit: some additional indentation might be useful in this file. At least to see
> the begin and end of head and body

Done.

[Siggi Cherem (dart-lang), 2011-12-15 23:24:36]

lgtm