Bug 84887

Created attachment 149620 [details] Patch

@jacobg: Can you take at the testharnessreport.js change? @dpranke: Can you look at the python script? @mjs: This is what we'd discussed and started hacking on at the committers meeting (though this is from the webperf WG instead of CSS). Are you happy with the result? Any preference on where this should go? @tonyg: Can you review the results?

Comment on attachment 149620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149620&action=review the python file mostly looks fine; there's one edge case you need to check for (I think) and one nit about argument names :). > Tools/Scripts/import-w3c-performance-wg-tests:32 > +import sys There's a bunch of infrastructure in webkitpy that could be used here, but as we discussed in person, I'm not sure there's much value in doing so. In particular, much of the infrastructure is designed so that it's easier to write tests, but I wouldn't bother to write tests for this, since it'll only be run occasionally, manually, and with manual inspection of the results. I'm just noting this so that others don't have the same thought and wonder ... > Tools/Scripts/import-w3c-performance-wg-tests:35 > + print 'USAGE: %s path_to_webperf path_to_WebKit' Maybe 'path_to_w3c_checkout_root' 'path_to_webkit_checkout_root' ? > Tools/Scripts/import-w3c-performance-wg-tests:60 > + os.mkdir(os.path.join(destination_directory, root, dirname)) You need to trap exceptions here in case the directories already exist.

Changes to testharnessreport.js look good.

Comment on attachment 149620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149620&action=review A few minor suggestions, everything else lgtm > Tools/Scripts/import-w3c-performance-wg-tests:30 > + This file could use more usage explanation, probably in the form of a better USAGE output or file level comment. For instance, I'm not sure what to use for the path_to_webperf. Explaining where to grab that repo would be nice. > Tools/Scripts/import-w3c-performance-wg-tests:47 > +replacements = { Even though you've mentioned it in the bug, I think a comment would be nice explaining why these replacements are necessary. Also, this really feels more like it should be a list of tuples rather than a dict so that the author can assume replacements happen in a certain order. > Tools/Scripts/import-w3c-performance-wg-tests:49 > + '\'www.w3c-test.org': '\'localhost:8000', The single tic is a little brittle, but at the same time it is hard to know exactly how these urls might look in the future. So what about adding something like this at the point the file is written: assert 'w3c-test.org' not in line, 'Imported test must not depend on live site' > LayoutTests/http/tests/resources/testharnessreport.js:1 > +/* Like we talked about offline, I think everything imported should live in a w3c directory rather than intermingle. Bonus points for a README or comment on imported items that warns to use the import script rather than modify in place. > LayoutTests/http/tests/w3c/webperf/approved/navigation-timing/html/test_timing_xserver_redirect.html:13 > + "Starting document.location.hostname is correct (w3c-test.org)": {}, Is it okay that this didn't get replaced? > LayoutTests/resources/testharnessreport.js:16 > +if (self.layoutTestController) { This just undoes https://bugs.webkit.org/show_bug.cgi?id=89182 , should probably check w/ tkent about that.

Created attachment 151848 [details] Patch

I have addressed all of Dirk's and Tony's feedback. (In reply to comment #5) > This just undoes https://bugs.webkit.org/show_bug.cgi?id=89182 , should probably check w/ tkent about that. I've reverted that.

Comment on attachment 151848 [details] Patch Rejecting attachment 151848 [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: kipped patching file LayoutTests/platform/win/Skipped patching file LayoutTests/platform/wincairo/Skipped patching file LayoutTests/resources/testharness.css patching file LayoutTests/resources/testharnessreport.js Hunk #2 FAILED at 73. 1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/resources/testharnessreport.js.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Genti..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13202830

Created attachment 152065 [details] Patch for landing

Comment on attachment 152065 [details] Patch for landing Rejecting attachment 152065 [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: file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/win/Skipped patching file LayoutTests/platform/wincairo/Skipped patching file LayoutTests/resources/testharness.css patching file LayoutTests/resources/testharnessreport.js Hunk #2 FAILED at 73. 1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/resources/testharnessreport.js.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13198947

Created attachment 152072 [details] Patch for landing

Comment on attachment 152072 [details] Patch for landing Rejecting attachment 152072 [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: file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/win/Skipped patching file LayoutTests/platform/wincairo/Skipped patching file LayoutTests/resources/testharness.css patching file LayoutTests/resources/testharnessreport.js Hunk #2 FAILED at 73. 1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/resources/testharnessreport.js.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13199877

Committed r122528: <http://trac.webkit.org/changeset/122528>

http/tests/w3c/webperf/approved/navigation-timing/html5/test_performance_attributes_exist_in_object.html seems to be very flaky.

What is the difference between the html and html5 directories?

(In reply to comment #15) > What is the difference between the html and html5 directories? I have no idea. I just copied it straight from upstream. I assumed there was something different about them. Diffing them yields no differences. I will modify the import script to ignore one. There's no reason to run the same tests twice. (In reply to comment #14) > http/tests/w3c/webperf/approved/navigation-timing/html5/test_performance_attributes_exist_in_object.html seems to be very flaky. I'll mark it as such. It seems to pass reliably on the W3C server.

Thanks!

https://bugs.webkit.org/show_bug.cgi?id=91224 is an example of that test flaking out.

Marked as flaky in http://trac.webkit.org/changeset/122591

... and http://trac.webkit.org/changeset/122592