[webkit-dev] run-webkit-tests question; hashes when comparing ref test output

[Simon Fraser]

On Jan 23, 2015, at 10:01 AM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> 
> 
> On Friday, January 23, 2015, Darin Adler <darin at apple.com <mailto:darin at apple.com>> wrote:
> > On Jan 22, 2015, at 10:36 PM, Alexey Proskuryakov <ap at webkit.org <javascript:;>> wrote:
> >
> >> 22 янв. 2015 г., в 17:57, Darin Adler <darin at apple.com <javascript:;>> написал(а):
> >>
> >> What about the test I cited?
> >>
> >> svg/css/svg-resource-fragment-identifier-img-src.html
> >
> > This particular test is buggy - it is a hidpi test, so it dumps results as a 1600x1200 image, but its -expected.html is not hidpi, and is dumped as 800x600, so hashes are obviously different. I now filed <https://bugs.webkit.org/show_bug.cgi?id=140815 <https://bugs.webkit.org/show_bug.cgi?id=140815>> about this test.
> >
> > When we compare pixels, we draw both images into bitmaps of the same size, so they become similar enough for ImageDiff to consider them identical.
> >
> > Earlier today, Simon and I agreed that we should just silence the error message, because it only tells us about minor color differences that are inevitable when comparing compositing vs. non-compositing. Looks like it tells us about more actionable issues, too. Also, I just found <https://bugs.webkit.org/show_bug.cgi?id=69444 <https://bugs.webkit.org/show_bug.cgi?id=69444>>, and I think that its rationale applies in this case, too.
> >
> > So we should probably keep this error message. I'm not sure whether we should make it a hard error though.
> 
> I think that to improve things we should make an informative message for this particular mistake where the expected file has the wrong size or resolution.
> 
> For me, the bad thing about the current message is not simply that it’s a false positive, but that it’s an unclear error message that covers too many different possibilities.
> 
> I’m not sure that keeping things as is will be a good strategy. A message that is often expected but sometimes indicates a real problem is not good for the project. The average engineer has no idea whether to ignore these or act on them!

Agreed. The size difference certainly needs to be a failure.

> 
> We could add a new test expectation like ImageDiff to suppress these or we could expose a new internals or testRunner methods to mark a test as such.

I don’t think that addresses the issue. Darin’s point is that if a developer sees this output (possibly when running tests after making a change), it needs to describe something actionable.

Just suppressing this output for existing tests doesn’t help when the output crops up unexpectedly.

Simon

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-dev/attachments/20150123/6abf451a/attachment.html>