Need tests for PerfTestRunner.computeStatistics Requested by tomz on #webkit.
Created attachment 139867 [details] Patch
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?
Created attachment 139898 [details] Patch
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.
Committed r115905: <http://trac.webkit.org/changeset/115905>
Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/115917
Committed r115942: <http://trac.webkit.org/changeset/115942>
Re-opened since this is blocked by 85770
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.