RESOLVED FIXED 69444
The new run-webkit-tests needs to dump out pixel hash failures even if the pixel test passes.
https://bugs.webkit.org/show_bug.cgi?id=69444
Summary The new run-webkit-tests needs to dump out pixel hash failures even if the pi...
Dave Hyatt
Reported 2011-10-05 10:48:10 PDT
To those of us running pixel tests, it's very important to be able to see when pixel hash failures occur, since they often indicate a legitimate failure that is going unnoticed. old-run-webkit-tests properly reports these pixel hash failures so that you can see if you caused a minute pixel test failure and fix it. The new tool does not, which is really painful for me. I'm having to stick with old-run-webkit-tests because of this issue.
Attachments
Patch (4.21 KB, patch)
2012-02-15 20:00 PST, Dirk Pranke
no flags
make the tests pass (7.65 KB, patch)
2012-02-15 20:44 PST, Dirk Pranke
no flags
ignore - wrong bug (7.16 KB, patch)
2012-02-15 20:46 PST, Dirk Pranke
no flags
change warning message to match ORWT (7.53 KB, patch)
2012-02-16 20:21 PST, Dirk Pranke
eric: review+
Dirk Pranke
Comment 1 2011-10-05 14:27:32 PDT
I assume you're running on the apple mac port ... is this a problem because by default the apple mac port runs with a tolerance set and allows some amount of fuzziness to match? IIRC, this happens even if you specify --tolerance=0.0. Also, to clarify, do you want to see the pixel hash mismatches on the console, in the results html file, or both?
Dave Hyatt
Comment 2 2011-10-27 15:19:16 PDT
Console would be sufficient for me.
Eric Seidel (no email)
Comment 3 2011-10-27 15:43:25 PDT
Interesting. It sounds like we just want --tolerance=0.0 by default. But it's also possible to spit out these errors to the console. I'm not sure I understand the current --tolerance vs. not decisions on various ports.
Dirk Pranke
Comment 4 2011-10-27 15:47:33 PDT
Note that we added the %age of the pixel mismatch in bug 67253, and as I noted in comment #1, using a tolerance of 0.0 may still mask some differences on the apple ports. It should be easy to note the hash mismatch in either the console or the results file.
Dirk Pranke
Comment 5 2012-02-15 19:49:52 PST
On a related note, currently, if you use the 'test' port, we have a test case failures/expected/checksum.html, where the checksum fails to match but the image matches; when you run rwt --platform test, this is reported as an 'unexpected pass', when it should be reported as an image hash mismatch
Dirk Pranke
Comment 6 2012-02-15 19:57:07 PST
Adding Ojan and Tony to this ... If memory serves, Chromium has always considered the case where the image hashes don't match but the images themselves do to be a passing case. I think we used to report a warning in the results.html file, but at some point we stopped doing that (either when we made the results file generated from the json data, or when we embedded the checksums into the image, I think). The upshot is we currently ignore the problem, which seems wrong, but I'm not sure which is the right thing to do. I think we have four choices: 1) report a warning to the console but otherwise treat the test as passing across the board 2) report a warning to the console and include the warning in results.html / results.json, but consider the test as passing (similar to how we handle stderr output now) 3) consider the test as failing, but only if --tolerance is set to 0.0 4) consider the test as failing, regardless of what --tolerance is set to In any situation, we'd probably need to change the results.html output to identify this case properly and not just confuse the user. I have a patch that does #1 but only logs an error to the console that I will post, just to demonstrate some of the scope of work involved in fixing it. I have no idea how often this will come up in practice so we probably don't want to overengineer this.
Dirk Pranke
Comment 7 2012-02-15 20:00:18 PST
Dirk Pranke
Comment 8 2012-02-15 20:44:33 PST
Created attachment 127305 [details] make the tests pass
Dirk Pranke
Comment 9 2012-02-15 20:46:33 PST
Created attachment 127306 [details] ignore - wrong bug
Tony Chang
Comment 10 2012-02-16 10:12:32 PST
Comment on attachment 127305 [details] make the tests pass View in context: https://bugs.webkit.org/attachment.cgi?id=127305&action=review We should just match old-run-webkit-tests behavior, which is to log to the console. > Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:270 > + _log.warning('%s had mismatched checksums but matching images (exp %s, got %s)' % > + (self._test_name, expected_driver_output.image_hash, driver_output.image_hash)) Can we match ORWT's text? If not, we should at least include the image diff percent.
Dirk Pranke
Comment 11 2012-02-16 11:41:54 PST
(In reply to comment #10) > (From update of attachment 127305 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127305&action=review > > We should just match old-run-webkit-tests behavior, which is to log to the console. > > > Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:270 > > + _log.warning('%s had mismatched checksums but matching images (exp %s, got %s)' % > > + (self._test_name, expected_driver_output.image_hash, driver_output.image_hash)) > > Can we match ORWT's text? If not, we should at least include the image diff percent. Good idea. I will take a look.
Dirk Pranke
Comment 12 2012-02-16 20:21:32 PST
Created attachment 127506 [details] change warning message to match ORWT
Dirk Pranke
Comment 13 2012-02-16 20:22:46 PST
Output now is: $ ./new-run-webkit-tests -p fast/html/keygen.html fast/html/keygen.html -> pixel hash failed (but pixel test still passes) All 1 tests ran as expected. $ I would be happy to add the tolerance value to both NRWT and ORWT as well if desired.
Eric Seidel (no email)
Comment 14 2012-02-17 16:18:01 PST
Comment on attachment 127506 [details] change warning message to match ORWT OK.
Dirk Pranke
Comment 15 2012-02-17 17:01:39 PST
Note You need to log in before you can comment on or make changes to this bug.