Bug 97182

Summary: REGRESSION: layout test results doesn't show diffs
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Blocker CC: ap, cdumez, dpranke, gyuyoung.kim, ojan, ossy, simon.fraser
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Ryosuke Niwa 2012-09-20 01:51:40 PDT
See http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r129100%20%283410%29/results.html

Even though fast/text/atsui-multiple-renderers.html is failing, we can't see the diff. The diff is there, it's just that results page doesn't seem know how to handle new actual values.
Comment 1 Ryosuke Niwa 2012-09-20 01:56:06 PDT
This is probably a regression from http://trac.webkit.org/changeset/129047.
Comment 2 Ryosuke Niwa 2012-09-20 01:57:34 PDT
This bug is a little tricky to fix. We used to figure out which link (e.g wav diff, png diff, txt diff, etc...) to show based on actual failures. If we're going to just report "FAIL" in actual results, then we wouldn't know which file exist.
Comment 3 Dirk Pranke 2012-09-20 09:33:40 PDT
Sigh. Yeah, I think perhaps the thing to do (at least for the moment) is to go back to using IMAGE+TEXT, TEXT, and AUDIO internally and still store those values in the JSON file, but keep Failure as the only expectation in the TestExpectations file, so that it will automatically address all of these.

Longer term this means that we should probably consolidate IMAGE+TEXT, TEXT, and AUDIO with the TestFailures types so that we only have one list of detailed failures, and continue mapping those onto a smaller list of expectation types. It wouldn't be a bad thing if we were pushing more details about how exactly tests were failing into results.json (so you could see when checksums failed but diffs passed, etc.).
Comment 4 Dirk Pranke 2012-09-20 09:34:46 PDT
An alternative is to have results.html probe for the various -actual files and -diff files and only display the ones it finds, but this feels less good to me, because we have the information, we're just throwing it away and then trying to recover from throwing it away.
Comment 5 Ojan Vafai 2012-09-20 09:38:57 PDT
(In reply to comment #3)
> Longer term this means that we should probably consolidate IMAGE+TEXT, TEXT, and AUDIO with the TestFailures types so that we only have one list of detailed failures, and continue mapping those onto a smaller list of expectation types. It wouldn't be a bad thing if we were pushing more details about how exactly tests were failing into results.json (so you could see when checksums failed but diffs passed, etc.).

I agree. I think this is the better direction to go.
Comment 6 Dirk Pranke 2012-09-20 10:57:41 PDT
*** Bug 97238 has been marked as a duplicate of this bug. ***
Comment 7 Alexey Proskuryakov 2012-09-20 11:02:34 PDT
Not seeing failure diffs basically blocks WebKit development. Can this be addressed today, or should r129047 be rolled out?
Comment 8 Dirk Pranke 2012-09-20 11:36:04 PDT
Created attachment 164956 [details]
Patch
Comment 9 Dirk Pranke 2012-09-20 11:41:16 PDT
Comment on attachment 164956 [details]
Patch

Clearing flags on attachment: 164956

Committed r129148: <http://trac.webkit.org/changeset/129148>
Comment 10 Dirk Pranke 2012-09-20 11:41:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Dirk Pranke 2012-09-20 11:45:27 PDT
This should fix the issues. Unfortunately, we had unit tests for both sides of this (generating both sides of this) but not an integration test that the correct data propagated all the way through. 

It's not actually clear how to do such an integration test, unfortunately, but I'm going to keep thinking about it since it seems like a pretty important thing to ensure stays working ...