Bug 85410 – Need tests for PerfTestRunner.computeStatistics

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[WebKit Review Bot]

Need tests for PerfTestRunner.computeStatistics
Requested by tomz on #webkit.

[Tom Zakrajsek]

Created attachment 139867 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 139867 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139867&action=review

Thanks for the patch! I think we want one more iteration here.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:6
> +  <script src="../../fast/js/resources/js-test-pre.js"></script>
> +  <script src="../../../PerformanceTests/resources/runner.js"></script>
> +  <script type="text/javascript">

I would prefer not indent script elements like this.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:11
> +    /** Find the minimum value in an array of numbers.
> +     *
> +     * @param array     input array of numbers
> +     * @return          minimum value in array
> +     */

We don't normally add per-function comment like this.
It makes the code more verbose than needed.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:12
> +    Array.min = function(array) {

You should probably check that Array doesn't already have min.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:13
> +      return Math.min.apply(Math, array);

Please use 4-space indentation.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:33
> +      array.sort( function(a,b) {return a - b;} );

Please add spaces between { and return, and ; and }

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:48
> +      return Array.sum(array)/array.length;

Space needed around /.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:58
> +      for (var a in array) {

Please don't use one-letter variable name like a.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:72
> +      for (var a in array) {

Ditto.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:95
> +      for (var a in array) {

Ditto.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:124
> +    /** Ensure no latent divide by 0's for an odd number of elements. */

Instead of adding comment like this. It's probably more helpful to put it in debug().

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:138
> +    /** This test will catch if any order dependencies in the data, such as
> +     *  needing to be numerically sorted, are not resolved by the algorithm.
> +     *  This variant covers an odd number of elements.
> +     */

Ditto.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:159
> +    /** This test will catch if any order dependencies in the data, such as
> +     *  needing to be numerically sorted, are not resolved by the algorithm.
> +     *  This variant covers an odd number of elements, and negative values.
> +     */

Ditto.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:204
> +  <script type="text/javascript">
> +    // runner.js marks this as an async test so we need to call notifyDone.
> +    layoutTestController.notifyDone();
> +  </script>

We can merge this into the script element above, no?

[Tom Zakrajsek]

Created attachment 139898 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 139898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139898&action=review

> LayoutTests/fast/harness/perftests/perf-runner-compute-statistics.html:15
> +        // don't want side effects on the input array, so...

Please don't abbreviate "original".

> PerformanceTests/resources/runner.js:2
> +// There are tests for computeStatistics() located in
> +// LayoutTests/fast/harness/perftests

You can probably fit this in one line.

[Tom Zakrajsek]

Committed r115905: <http://trac.webkit.org/changeset/115905>

[Ryosuke Niwa]

Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/115917

[Tom Zakrajsek]

Committed r115942: <http://trac.webkit.org/changeset/115942>

[WebKit Review Bot]

Re-opened since this is blocked by 85770

[Tom Zakrajsek]

This was reopened while testing "rollout" capability.  This fix was good and the rollout patch was cancelled after it was verified.  This bug remains fixed.