Bug 36620 – suppress image failures for now to get a clean test run w/ pixel tests enabled for new-run-webkit-tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

Bug 34826 allows --pixel-tests to properly work, but of course we have a lot of failing pixel tests. This bug will track the expectations and rebaselines to get the tests to run cleanly; fixing any real issues will need to be separate bugs.

[Dirk Pranke]

Created attachment 51799 [details]
add test_expectations.txt suppressions and fix a bunch of incorrect checksums

This patch suppresses all the tests that currently fail pixel tests. We still need to triage them and figure out which need new baselines and which are actual bugs.

[David Levin]

Comment on attachment 51799 [details]
add test_expectations.txt suppressions and fix a bunch of incorrect checksums


Needs a ChangeLog.

> diff --git a/LayoutTests/platform/mac/test_expectations.txt b/LayoutTests/platform/mac/test_expectations.txt
> -// This file consists of lines with specifications of what

I think these instructions are useful.

> +BUG33620: fast/block/positioning/absolute-positioning-no-scrollbar.html = MISSING

Missing a space.

r- for missing ChangeLog.

[Dirk Pranke]

Created attachment 51801 [details]
add missing Changelog, add instructions back to the beginning of test_expectations.txt, fix typo

[Ojan Vafai]

(In reply to comment #1)
> Created an attachment (id=51799) [details]
> add test_expectations.txt suppressions and fix a bunch of incorrect checksums
> 
> This patch suppresses all the tests that currently fail pixel tests. We still
> need to triage them and figure out which need new baselines and which are
> actual bugs.

Can you explain the updated checksums? The changelog description doesn't explain why/how they're incorrect. Do they not match the checked in pngs? 

More importantly, I don't think this is a good use of time. Until there are pixel bots running *somewhere*, there is no point getting the pixel tests green for a few runs. They'll break almost immediately. It's fighting a losing battle without bots.

Even if we somehow fixed all the broken pixel tests, they'll just start breaking again since we haven't addressed the root cause.

[David Levin]

Comment on attachment 51801 [details]
add missing Changelog, add instructions back to the beginning of test_expectations.txt, fix typo

> diff --git a/LayoutTests/platform/mac/test_expectations.txt b/LayoutTests/platform/mac/test_expectations.txt
>  // -A test cannot be both SLOW and TIMEOUT
>  // -A test can be included twice, but not via the same path.
>  // -If a test is included twice, then the more precise path wins.
> -// -CRASH tests cannot be WONTFIX
> +// -CRASH tests cannot be WONTFIXBUG36620 : compositing/color-matching/image-color-matching.html = IMAGE

I think you wanted a return here after WONTFIX.

[Dirk Pranke]

Committed r56649: <http://trac.webkit.org/changeset/56649>

[Ojan Vafai]

(In reply to comment #6)
> Committed r56649: <http://trac.webkit.org/changeset/56649>

Did you see my comments? Can you explain, at least for posterity sake, what you meant by incorrect checksums (e.g. they didn't match the checked in images?)?

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > Committed r56649: <http://trac.webkit.org/changeset/56649>
> 
> Did you see my comments? Can you explain, at least for posterity sake, what you
> meant by incorrect checksums (e.g. they didn't match the checked in images?)?

Ah, no, I didn't see your comments. Sorry!

You are correct in that "incorrect checksums" means that the checksums did not match the checked in images. When you ran run-webkit-test, you would get the error message "images match but checksums don't".

As to whether or not this is a waste of time, you may be right that suppressing the images failures is a semi-futile effort, but I look at it more as making sure that the test_expectations file does actually work, and fixing the checksums is definitely useful.

[Eric Seidel (no email)]

Committed r56982: <http://trac.webkit.org/changeset/56982>