Bug 77329

Summary: [PerformanceTests] Add landing html for Dromaeo dom-query test
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Tools / TestsAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ojan, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76156    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing none

Description Hajime Morrita 2012-01-30 04:14:25 PST
Adding landing html which include actual test as an iframe.
Comment 1 Hajime Morrita 2012-01-30 04:34:40 PST
Created attachment 124524 [details]
Patch
Comment 2 Hajime Morrita 2012-01-30 04:36:35 PST
Hi Ryosue, could you take a look?

I'm intending this to be used for a "proof" test for Dromaeo integration.
Once the Dromaeo checkout is visible from run-perf-tests, I'll add other test cases.
Comment 3 Ryosuke Niwa 2012-01-30 09:21:35 PST
Comment on attachment 124524 [details]
Patch

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

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:254
> +        re.compile(r'^CONSOLE MESSAGE: line \d+: \d+'),

I'm not sure if we should ignore all console messages since some console messages contain actual errors. What are examples of console messages you get?

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:255
> +        re.compile(r'^Blocked access to external URL'),

What are examples of external URLs Dromaeo tries to load?

> PerformanceTests/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> PerformanceTests/Dromaeo/dom-query.html:1
> +<html>

Missing DOCTYPE.

> PerformanceTests/Dromaeo/dom-query.html:3
> +<script src="../Checkouts/dromaeo/web/jquery.js"></script>

So Dromaeo will be added to PerformanceTests/Checkouts? Can we instead add it to Dromaeo/resources/? Or license doesn't allow it?

> PerformanceTests/Dromaeo/resources/drtrunner.js:10
> +                 median += item.median;

This is an approximation at best but I guess it's okay. Or maybe it's better not to report median in this case.

> PerformanceTests/Dromaeo/resources/drtrunner.js:14
> +                 stdev += item.deviation;

You can't add standard deviations like this. Assuming N_i are mutually independent random variables for all i = 0 through n, stdev of N_0 + N_1 ... + N_n is sqrt(Var[N_0] + Var[N_1] ... + Var[N_n]). So here, you can do:
variance += item.deviation * item.deviation;

> PerformanceTests/Dromaeo/resources/drtrunner.js:76
> +             DRT.log("");
> +             DRT.log("avg " + scores.avg);
> +             DRT.log("median " + scores.median);
> +             DRT.log("stdev " + scores.stdev);
> +             DRT.log("min " + scores.min);
> +             DRT.log("max " + scores.max);

Maybe we should extract a function from logStatistics so that we don't have to duplicate code here.

> PerformanceTests/Dromaeo/resources/drtrunner.js:80
> +                                   if (window.layoutTestController)
> +                                       layoutTestController.notifyDone();
> +                               }, 0);

Wrong indentation.
Comment 4 Hajime Morrita 2012-01-30 16:29:36 PST
Comment on attachment 124524 [details]
Patch

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

Hi Ryosuke, thanks for taking a look at this. I'll update this soon.
By the way

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:254
>> +        re.compile(r'^CONSOLE MESSAGE: line \d+: \d+'),
> 
> I'm not sure if we should ignore all console messages since some console messages contain actual errors. What are examples of console messages you get?

I use console.log() to print progress for DRT. It's like "CONSOLE MESSAGE: Line 31: 3091". But I can remove this.

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:255
>> +        re.compile(r'^Blocked access to external URL'),
> 
> What are examples of external URLs Dromaeo tries to load?

This is for icons which test html contains. For example: https://github.com/omo/dromaeo/blob/master/tests/dom-query.html#L108

>> PerformanceTests/ChangeLog:13
>> +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the long description.

Oops. will fix.

>> PerformanceTests/Dromaeo/dom-query.html:1
>> +<html>
> 
> Missing DOCTYPE.

Sure.

>> PerformanceTests/Dromaeo/dom-query.html:3
>> +<script src="../Checkouts/dromaeo/web/jquery.js"></script>
> 
> So Dromaeo will be added to PerformanceTests/Checkouts? Can we instead add it to Dromaeo/resources/? Or license doesn't allow it?

Dromaeo specific test cases are MIT license. But I'm thinking we can check out as a part of test execution instead of checking it in statically.
If we're going to check it in, resource/ would be better.

>> PerformanceTests/Dromaeo/resources/drtrunner.js:14
>> +                 stdev += item.deviation;
> 
> You can't add standard deviations like this. Assuming N_i are mutually independent random variables for all i = 0 through n, stdev of N_0 + N_1 ... + N_n is sqrt(Var[N_0] + Var[N_1] ... + Var[N_n]). So here, you can do:
> variance += item.deviation * item.deviation;

You are right. Math is hard. Will fix.

>> PerformanceTests/Dromaeo/resources/drtrunner.js:76
>> +             DRT.log("max " + scores.max);
> 
> Maybe we should extract a function from logStatistics so that we don't have to duplicate code here.

Will do.

>> PerformanceTests/Dromaeo/resources/drtrunner.js:80
>> +                               }, 0);
> 
> Wrong indentation.

Will fix.
Comment 5 Hajime Morrita 2012-01-30 16:32:46 PST
> Hi Ryosuke, thanks for taking a look at this. I'll update this soon.
> By the way
...By the way, I intended to write something about Bug 77328. But I should write there.
Comment 6 Ryosuke Niwa 2012-01-30 16:36:09 PST
Comment on attachment 124524 [details]
Patch

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

>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:254
>>> +        re.compile(r'^CONSOLE MESSAGE: line \d+: \d+'),
>> 
>> I'm not sure if we should ignore all console messages since some console messages contain actual errors. What are examples of console messages you get?
> 
> I use console.log() to print progress for DRT. It's like "CONSOLE MESSAGE: Line 31: 3091". But I can remove this.

I think we should remove that. There are cases where we want to report errors (e.g. script exceptions) using console message. You can call log(~) as well but I feel that console.log should be a stronger indication of something going wrong.

>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:255
>>> +        re.compile(r'^Blocked access to external URL'),
>> 
>> What are examples of external URLs Dromaeo tries to load?
> 
> This is for icons which test html contains. For example: https://github.com/omo/dromaeo/blob/master/tests/dom-query.html#L108

Huh... can we replace those urls by relative paths if we're checking tests into webkit?

>>> PerformanceTests/Dromaeo/dom-query.html:3
>>> +<script src="../Checkouts/dromaeo/web/jquery.js"></script>
>> 
>> So Dromaeo will be added to PerformanceTests/Checkouts? Can we instead add it to Dromaeo/resources/? Or license doesn't allow it?
> 
> Dromaeo specific test cases are MIT license. But I'm thinking we can check out as a part of test execution instead of checking it in statically.
> If we're going to check it in, resource/ would be better.

I think we can check in MIT-license code: https://lists.webkit.org/pipermail/webkit-dev/2010-January/011406.html
Comment 7 Hajime Morrita 2012-01-30 17:10:37 PST
Comment on attachment 124524 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:254
>>>> +        re.compile(r'^CONSOLE MESSAGE: line \d+: \d+'),
>>> 
>>> I'm not sure if we should ignore all console messages since some console messages contain actual errors. What are examples of console messages you get?
>> 
>> I use console.log() to print progress for DRT. It's like "CONSOLE MESSAGE: Line 31: 3091". But I can remove this.
> 
> I think we should remove that. There are cases where we want to report errors (e.g. script exceptions) using console message. You can call log(~) as well but I feel that console.log should be a stronger indication of something going wrong.

OK. I'll remove this.

>>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:255
>>>> +        re.compile(r'^Blocked access to external URL'),
>>> 
>>> What are examples of external URLs Dromaeo tries to load?
>> 
>> This is for icons which test html contains. For example: https://github.com/omo/dromaeo/blob/master/tests/dom-query.html#L108
> 
> Huh... can we replace those urls by relative paths if we're checking tests into webkit?

Okay. I'll make some blank image and replace W3C logo with that.

>>>> PerformanceTests/Dromaeo/dom-query.html:3
>>>> +<script src="../Checkouts/dromaeo/web/jquery.js"></script>
>>> 
>>> So Dromaeo will be added to PerformanceTests/Checkouts? Can we instead add it to Dromaeo/resources/? Or license doesn't allow it?
>> 
>> Dromaeo specific test cases are MIT license. But I'm thinking we can check out as a part of test execution instead of checking it in statically.
>> If we're going to check it in, resource/ would be better.
> 
> I think we can check in MIT-license code: https://lists.webkit.org/pipermail/webkit-dev/2010-January/011406.html

Sure. I found that it contains ext.js binary, which doesn't look capable for WebKit repo. I'll remove it before checkin.
It should be done by Bug 77328.
Comment 8 Hajime Morrita 2012-01-30 18:41:57 PST
Created attachment 124655 [details]
Patch
Comment 9 Ryosuke Niwa 2012-01-30 19:10:30 PST
Comment on attachment 124655 [details]
Patch

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

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:254
> +        re.compile(r'has \d+ onunload handler')]

Is it possible to make this expression more explicit? We don't want to ignore random errors appearing on the same line (e.g. due to missing \n)

> PerformanceTests/ChangeLog:16
> +        * Dromaeo/resources/drtrunner.js: Added.

It's kind of oxymoron to call it drtrunner.js now that this file is inside PerfomanceTests but I can't think of a better name.
Comment 10 Hajime Morrita 2012-01-30 21:07:02 PST
Comment on attachment 124655 [details]
Patch

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

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:254
>> +        re.compile(r'has \d+ onunload handler')]
> 
> Is it possible to make this expression more explicit? We don't want to ignore random errors appearing on the same line (e.g. due to missing \n)

Well, I'll put the exact message text here.

>> PerformanceTests/ChangeLog:16
>> +        * Dromaeo/resources/drtrunner.js: Added.
> 
> It's kind of oxymoron to call it drtrunner.js now that this file is inside PerfomanceTests but I can't think of a better name.

Good point. Will rename dromaeorunnner.js
Comment 11 Hajime Morrita 2012-01-30 22:48:59 PST
Created attachment 124674 [details]
Patch for landing
Comment 12 Hajime Morrita 2012-01-30 22:50:10 PST
Comment on attachment 124674 [details]
Patch for landing

missing reviewer name.
Comment 13 Hajime Morrita 2012-01-30 22:51:13 PST
Created attachment 124675 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-01-30 22:54:50 PST
Comment on attachment 124675 [details]
Patch for landing

Rejecting attachment 124675 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ceTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file PerformanceTests/Dromaeo/dom-query.html
patching file PerformanceTests/Dromaeo/resources/dromaeorunner.js
patching file PerformanceTests/Skipped
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file PerformanceTests/Skipped.rej
patching file PerformanceTests/resources/runner.js
Hunk #1 succeeded at 63 with fuzz 1.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/11373171
Comment 15 Hajime Morrita 2012-01-31 02:01:18 PST
Created attachment 124686 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-01-31 02:14:05 PST
Comment on attachment 124686 [details]
Patch for landing

Clearing flags on attachment: 124686

Committed r106348: <http://trac.webkit.org/changeset/106348>
Comment 17 WebKit Review Bot 2012-01-31 02:14:11 PST
All reviewed patches have been landed.  Closing bug.