Bug 134421 - Enhance Speedometer automation testing
Summary: Enhance Speedometer automation testing
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-27 16:39 PDT by fmeawad
Modified: 2015-09-14 10:47 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2014-08-21 17:20 PDT, fmeawad
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description fmeawad 2014-06-27 16:39:07 PDT
Allow the InteractiveRunner to accept a parameter that would select one of the suites.
If the parameter is passed, the passed suite is selected and started, otherwise enabled tests selected (current state).
Comment 1 Ryosuke Niwa 2014-06-28 02:09:04 PDT
That doesn't sound "interactive" to me.  I think it's better to improve Full.html to take configuration parameters and make them report individual measurement instead.
Comment 2 fmeawad 2014-08-21 17:20:39 PDT
Created attachment 236947 [details]
Patch
Comment 3 Ryosuke Niwa 2014-09-03 08:33:49 PDT
Comment on attachment 236947 [details]
Patch

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

> PerformanceTests/Speedometer/resources/main.js:153
> +function startBenchmark(selectedSuite) {
> +    var enabledSuites = Suites.filter(function (suite) { if (selectedSuite) return selectedSuite == suite.name; return !suite.disabled });

The right approach will be for this function to check the query string
since this is the function responsible for filtering suites.

> PerformanceTests/Speedometer/resources/main.js:201
> +function getQueryVariable(variable) {

WebKit's naming convention doesn't use "get" prefix unless we have an out argument.
I'd call this function queryStringValueForKey(key) if I were you.

> PerformanceTests/Speedometer/resources/main.js:204
> +    var vars = queryString.split("&");
> +    for (var i = 0; i < vars.length; i++) {

Each segment split by & are "pairs".
Also, we've been using single quotation marks for strings so please be consistent and use a single quotation here.

> PerformanceTests/Speedometer/resources/main.js:205
> +        var pair = vars[i].split("=");

Ditto.  s/"/'/g.

> PerformanceTests/Speedometer/resources/main.js:209
> +    return false;

It seems strange to return "false" when there's no value.
I think null or undefined will be better.
Comment 4 Ryosuke Niwa 2015-08-06 21:24:14 PDT
Is this bug still relevant?
Comment 5 fmeawad 2015-08-07 11:27:39 PDT
No, not any more.
Comment 6 Csaba Osztrogonác 2015-09-14 10:47:49 PDT
Comment on attachment 236947 [details]
Patch

Cleared review? from attachment 236947 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).