Bug 67253

Created attachment 105730 [details] Sample ORWT output

Sounds like a great feature. Easy to do.

I'll do it if someone points me in the right direction. I assume the % data will have to go into the json?

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py#L75 is responsible for generating the dictionary which gets dumped into full_results.json.

I'm still trying to figure out how exactly we'd do this. :)

ORWT just reads this information from ImageDiff: http://trac.webkit.org/browser/trunk/Tools/Scripts/old-run-webkit-tests#L942

Here is the relevant line in NRWT: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py#L201 We would need to change _read_image_diff to return a tuple. Then there is some plumbing to get it to the json, but should be relatively straightforward.

This can be done. It's just a lot of plumbing. I will add this to my list right after bug 65781 which is what I'm currently working on. If someone else wants to take this on, that'd be even better of course, as it frees me up to fix other NRWT bugs. :)

Created attachment 108540 [details] Patch in progress

I have something mostly working.

Created attachment 108554 [details] Patch

Attachment 108554 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/harness/results.html', u'..." exit_code: 1 Tools/Scripts/webkitpy/layout_tests/port/webkit.py:184: multiple statements on one line (semicolon) [pep8/E702] [5] Tools/Scripts/webkitpy/layout_tests/port/chromium.py:216: at least two spaces before inline comment [pep8/E261] [5] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 108554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108554&action=review > Tools/Scripts/webkitpy/layout_tests/models/test_failures.py:171 > class FailureImageHashMismatch(TestFailure): > """Image hashes didn't match.""" > + def __init__(self, diff_percent=0): > + self.diff_percent = diff_percent I feel like I've tried to add non-static data to these before and others have gotten sad. The test_failures.py design is bad/broken. We'll have to consult Ojan/Dirk here for suggestions. >> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:216 >> + return [result, 0] # FIXME: how to get % diff? > > at least two spaces before inline comment [pep8/E261] [5] It's sad, but true that our python vs. c++ comment spacing is different in webkit. :(

I'm a python noob, so bear in mind when reviewing!

Comment on attachment 108554 [details] Patch Attachment 108554 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9839093 New failing tests: fast/harness/results.html

(In reply to comment #15) > (From update of attachment 108554 [details]) > Attachment 108554 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/9839093 > > New failing tests: > fast/harness/results.html Why is results.html inside LayoutTests? This is so stupid.

Comment on attachment 108554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108554&action=review R-'ing this for now until we can add an integration test and I get a chance to run through the patch by hand. You probably also need to update diff_image in port/test.py. >> Tools/Scripts/webkitpy/layout_tests/models/test_failures.py:171 >> + self.diff_percent = diff_percent > > I feel like I've tried to add non-static data to these before and others have gotten sad. The test_failures.py design is bad/broken. We'll have to consult Ojan/Dirk here for suggestions. I think this should mostly work. We may have to double-check the serialization/pickling code to make sure it transfers across. >> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:184 >> + diff_percent = 0; > > multiple statements on one line (semicolon) [pep8/E702] [5] Python doesn't use semicolons :) > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:583 > + return [True, 0] We should probably have at least one integration test that returns a non-zero diff value. I will look into what it would take to make this work.

(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 108554 [details] [details]) > > Attachment 108554 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/9839093 > > > > New failing tests: > > fast/harness/results.html > > Why is results.html inside LayoutTests? This is so stupid. results.html itself is a test to ensure that we don't break the generated output.

Created attachment 108771 [details] update smfr's patch to add more tests, fix style nits

Eric or Tony, can I haz review plz?

Comment on attachment 108771 [details] update smfr's patch to add more tests, fix style nits View in context: https://bugs.webkit.org/attachment.cgi?id=108771&action=review Seems OK. > LayoutTests/fast/harness/resources/results-test.js:37 > +function mockExpectation(expected, actual, diff_percentage) I would recommend having this default to None so you don't have to change so many results. > LayoutTests/fast/harness/resources/results-test.js:446 > + subtree['expected-to-pass-but-crashed.html'] = mockExpectation('PASS', 'CRASH', 0); It seems mockExpectation should just have a default value for diff_percentage.

Created attachment 108773 [details] add default value to mockExpectation() in results-test.js

Comment on attachment 108773 [details] add default value to mockExpectation() in results-test.js (In reply to comment #21) > > LayoutTests/fast/harness/resources/results-test.js:37 > > +function mockExpectation(expected, actual, diff_percentage) > > I would recommend having this default to None so you don't have to change so many results. > Good suggestion. Of course, I didn't actually know how to do that, so I've now learned something as well. Done.

Comment on attachment 108773 [details] add default value to mockExpectation() in results-test.js View in context: https://bugs.webkit.org/attachment.cgi?id=108773&action=review OK. > Tools/Scripts/webkitpy/layout_tests/models/test_failures.py:171 > + def __init__(self, diff_percent=0): > + self.diff_percent = diff_percent The pythonistas would tell you you should use =None and then "diff_percent or 0" in the body. It doesn't matter because 0 is a constant, but if you were using [] for example, you could hose yourself, since the "default" object is a static and shared between all calls of the function.

Comment on attachment 108773 [details] add default value to mockExpectation() in results-test.js Clearing flags on attachment: 108773 Committed r96272: <http://trac.webkit.org/changeset/96272>

All reviewed patches have been landed. Closing bug.