Bug 67253 – NRWT should show the size of the pixel image delta in results.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Simon Fraser (smfr)]

RWT shows % pixel diffs in the results; NRWT results do not show this. It's very useful when looking at pixel diffs.

[Simon Fraser (smfr)]

Created attachment 105730 [details]
Sample ORWT output

[Eric Seidel (no email)]

Sounds like a great feature.  Easy to do.

[Simon Fraser (smfr)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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. :)

[Simon Fraser (smfr)]

Created attachment 108540 [details]
Patch in progress

[Simon Fraser (smfr)]

I have something mostly working.

[Simon Fraser (smfr)]

Created attachment 108554 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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. :(

[Simon Fraser (smfr)]

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

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

(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.

[Dirk Pranke]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

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

[Dirk Pranke]

Eric or Tony, can I haz review plz?

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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

[Dirk Pranke]

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.

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.