[prev in list] [next in list] [prev in thread] [next in thread]
List: webkit-dev
Subject: Re: [webkit-dev] run-webkit-tests question; hashes when comparing ref test output
From: Simon Fraser <simon.fraser () apple ! com>
Date: 2015-01-23 18:27:01
Message-ID: B892AD3F-464D-480E-B77A-0A9432C71202 () apple ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Jan 23, 2015, at 10:01 AM, Ryosuke Niwa <rniwa@webkit.org> wrote:
>
>
> On Friday, January 23, 2015, Darin Adler <darin@apple.com <mailto:darin@apple.com>> \
> wrote:
> > On Jan 22, 2015, at 10:36 PM, Alexey Proskuryakov <ap@webkit.org <javascript:;>> \
> > wrote:
> > > 22 янв. 2015 г., в 17:57, Darin Adler <darin@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
[Attachment #5 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html \
charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; \
-webkit-line-break: after-white-space;" class="">On Jan 23, 2015, at 10:01 AM, \
Ryosuke Niwa <<a href="mailto:rniwa@webkit.org" class="">rniwa@webkit.org</a>> \
wrote:<br class=""><div><blockquote type="cite" class=""><br \
class="Apple-interchange-newline"><div class=""><br class="">On Friday, January 23, \
2015, Darin Adler <<a href="mailto:darin@apple.com" \
class="">darin@apple.com</a>> wrote:<br class=""><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> On Jan 22, \
2015, at 10:36 PM, Alexey Proskuryakov <<a href="javascript:;" onclick="_e(event, \
'cvml', 'ap@webkit.org')" class="">ap@webkit.org</a>> wrote:<br class=""> ><br \
class=""> >> 22 янв. 2015 г., в 17:57, Darin Adler <<a \
href="javascript:;" onclick="_e(event, 'cvml', 'darin@apple.com')" \
class="">darin@apple.com</a>> написал(а):<br class=""> >><br \
class=""> >> What about the test I cited?<br class="">
>><br class="">
>> svg/css/svg-resource-fragment-identifier-img-src.html<br class="">
><br class="">
> 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 <<a \
href="https://bugs.webkit.org/show_bug.cgi?id=140815" target="_blank" \
class="">https://bugs.webkit.org/show_bug.cgi?id=140815</a>> about this test.<br \
class=""> ><br class="">
> 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.<br class=""> \
><br class=""> > 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 <<a \
href="https://bugs.webkit.org/show_bug.cgi?id=69444" target="_blank" \
class="">https://bugs.webkit.org/show_bug.cgi?id=69444</a>>, and I think that its \
rationale applies in this case, too.<br class=""> ><br class="">
> So we should probably keep this error message. I'm not sure whether we should \
make it a hard error though.<br class=""> <br class="">
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.<br \
class=""> <br class="">
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.<br class=""> <br class="">
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!</blockquote></div></blockquote><div><br class=""></div>Agreed. The size \
difference certainly needs to be a failure.</div><div><br class=""><blockquote \
type="cite" class=""><div class=""><div class=""><br class=""></div><div class="">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.</div></div></blockquote><br class=""></div><div>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.</div><div><br class=""></div><div>Just suppressing this output for \
existing tests doesn't help when the output crops up unexpectedly.</div><div><br \
class=""></div><div>Simon</div><div><br class=""></div></body></html>
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic