Bug 67253 - NRWT should show the size of the pixel image delta in results.html
Summary: NRWT should show the size of the pixel image delta in results.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 64491
  Show dependency treegraph
 
Reported: 2011-08-30 19:12 PDT by Simon Fraser (smfr)
Modified: 2011-09-28 17:08 PDT (History)
9 users (show)

See Also:


Attachments
Sample ORWT output (34.56 KB, text/html)
2011-08-30 19:12 PDT, Simon Fraser (smfr)
no flags Details
Patch in progress (9.04 KB, patch)
2011-09-23 14:48 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (12.58 KB, patch)
2011-09-23 16:29 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
update smfr's patch to add more tests, fix style nits (26.50 KB, patch)
2011-09-26 18:08 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
add default value to mockExpectation() in results-test.js (23.99 KB, patch)
2011-09-26 18:27 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-08-30 19:12:01 PDT
RWT shows % pixel diffs in the results; NRWT results do not show this. It's very useful when looking at pixel diffs.
Comment 1 Simon Fraser (smfr) 2011-08-30 19:12:40 PDT
Created attachment 105730 [details]
Sample ORWT output
Comment 2 Eric Seidel (no email) 2011-08-31 09:29:00 PDT
Sounds like a great feature.  Easy to do.
Comment 3 Simon Fraser (smfr) 2011-08-31 09:33:36 PDT
I'll do it if someone points me in the right direction. I assume the % data will have to go into the json?
Comment 4 Eric Seidel (no email) 2011-08-31 11:41:23 PDT
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.
Comment 5 Eric Seidel (no email) 2011-08-31 11:42:28 PDT
I'm still trying to figure out how exactly we'd do this. :)
Comment 6 Eric Seidel (no email) 2011-09-16 13:23:13 PDT
ORWT just reads this information from ImageDiff:
http://trac.webkit.org/browser/trunk/Tools/Scripts/old-run-webkit-tests#L942
Comment 7 Eric Seidel (no email) 2011-09-16 13:26:21 PDT
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.
Comment 8 Eric Seidel (no email) 2011-09-16 13:28:32 PDT
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. :)
Comment 9 Simon Fraser (smfr) 2011-09-23 14:48:49 PDT
Created attachment 108540 [details]
Patch in progress
Comment 10 Simon Fraser (smfr) 2011-09-23 16:23:40 PDT
I have something mostly working.
Comment 11 Simon Fraser (smfr) 2011-09-23 16:29:01 PDT
Created attachment 108554 [details]
Patch
Comment 12 WebKit Review Bot 2011-09-23 16:32:09 PDT
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 13 Eric Seidel (no email) 2011-09-23 17:02:33 PDT
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. :(
Comment 14 Simon Fraser (smfr) 2011-09-23 17:08:30 PDT
I'm a python noob, so bear in mind when reviewing!
Comment 15 WebKit Review Bot 2011-09-23 17:19:22 PDT
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
Comment 16 Simon Fraser (smfr) 2011-09-23 17:21:14 PDT
(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 17 Dirk Pranke 2011-09-23 17:29:44 PDT
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.
Comment 18 Dirk Pranke 2011-09-23 17:38:12 PDT
(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.
Comment 19 Dirk Pranke 2011-09-26 18:08:02 PDT
Created attachment 108771 [details]
update smfr's patch to add more tests, fix style nits
Comment 20 Dirk Pranke 2011-09-26 18:09:07 PDT
Eric or Tony, can I haz review plz?
Comment 21 Eric Seidel (no email) 2011-09-26 18:15:40 PDT
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.
Comment 22 Dirk Pranke 2011-09-26 18:27:35 PDT
Created attachment 108773 [details]
add default value to mockExpectation() in results-test.js
Comment 23 Dirk Pranke 2011-09-26 18:28:25 PDT
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 24 Eric Seidel (no email) 2011-09-27 00:28:16 PDT
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 25 Dirk Pranke 2011-09-28 17:08:39 PDT
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>
Comment 26 Dirk Pranke 2011-09-28 17:08:44 PDT
All reviewed patches have been landed.  Closing bug.