Adding landing html which include actual test as an iframe.
Created attachment 124524 [details] Patch
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 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 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.
> 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 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 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.
Created attachment 124655 [details] Patch
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 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
Created attachment 124674 [details] Patch for landing
Comment on attachment 124674 [details] Patch for landing missing reviewer name.
Created attachment 124675 [details] Patch for landing
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
Created attachment 124686 [details] Patch for landing
Comment on attachment 124686 [details] Patch for landing Clearing flags on attachment: 124686 Committed r106348: <http://trac.webkit.org/changeset/106348>
All reviewed patches have been landed. Closing bug.