Bug 87552 - Test expectation pngs missing checksums are treated as MISSING by bots
Summary: Test expectation pngs missing checksums are treated as MISSING by bots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Marcus Bulach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-25 17:19 PDT by Joshua Bell
Modified: 2012-06-12 14:42 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2012-05-28 10:38 PDT, Marcus Bulach
no flags Details | Formatted Diff | Diff
Patch (2.22 KB, patch)
2012-05-29 01:52 PDT, Marcus Bulach
no flags Details | Formatted Diff | Diff
proposed patch (269.06 KB, patch)
2012-05-30 03:44 PDT, Andras Becsi
ojan: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-05-25 17:19:46 PDT
Change set http://trac.webkit.org/changeset/118566 added new Acid3 tests and baselines. However, the bots are reporting the files as MISSING despite the fallback rules:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fmisc%2Facid3.html

dpranke mentions (in chat) that "acid3-expected.png doesn't have an embedded checksum and it looks like we don't handle that very well"
Comment 1 Marcus Bulach 2012-05-28 10:38:51 PDT
Created attachment 144383 [details]
Patch
Comment 2 Marcus Bulach 2012-05-29 01:52:15 PDT
Created attachment 144479 [details]
Patch
Comment 3 Marcus Bulach 2012-05-29 02:12:33 PDT
Committed r118741: <http://trac.webkit.org/changeset/118741>
Comment 4 Marcus Bulach 2012-05-29 02:13:30 PDT
Sorry, forgot to land with --no-close. This bug still exists.
Comment 5 Ojan Vafai 2012-05-29 09:12:45 PDT
MISSING seems correct for a png missing a checksum. We do printout "No expected image hash found" for that test. I suppose we should make that text more clear like "No embedded checksum in the png file".

Arguably that test should still pass and just log a warning, but I think I prefer it failing hard like this. There simply shouldn't ever be png files without checksums. We should see how this one got committed and resolve that issue.
Comment 6 Ojan Vafai 2012-05-29 09:16:16 PDT
Looks like the break happened in https://bugs.webkit.org/show_bug.cgi?id=87187. Andras, did you use run-webkit-tests to generate the png files for that patch? We should figure out how you ended up with png files that lack embedded checksums.

On a tooling note, we should add a presubmit check for this. It would be super easy. And we should add a big warning in the code-review tool. Also super-easy.
Comment 7 Andras Becsi 2012-05-29 10:24:25 PDT
(In reply to comment #6)
> Looks like the break happened in https://bugs.webkit.org/show_bug.cgi?id=87187. Andras, did you use run-webkit-tests to generate the png files for that patch? We should figure out how you ended up with png files that lack embedded checksums.

D'oh, sorry for that. Those png files need to be regenerated.
Can the actual png's be recovered from the build bots or the ews?

> 
> On a tooling note, we should add a presubmit check for this. It would be super easy. And we should add a big warning in the code-review tool. Also super-easy.

Agree on that.
Comment 8 Dirk Pranke 2012-05-29 12:18:11 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Looks like the break happened in https://bugs.webkit.org/show_bug.cgi?id=87187. Andras, did you use run-webkit-tests to generate the png files for that patch? We should figure out how you ended up with png files that lack embedded checksums.
> 
> D'oh, sorry for that. Those png files need to be regenerated.
> Can the actual png's be recovered from the build bots or the ews?
> 

You should be able to fetch the chromium png's from the bots.

I tend to agree with Ojan that this should stay as a hard failure, although I am open to suggestions to make it clearer what's happened. I think the missing hash log message gets lost in the --verbose logging.
Comment 9 Ojan Vafai 2012-05-29 14:27:29 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Looks like the break happened in https://bugs.webkit.org/show_bug.cgi?id=87187. Andras, did you use run-webkit-tests to generate the png files for that patch? We should figure out how you ended up with png files that lack embedded checksums.
> 
> D'oh, sorry for that. Those png files need to be regenerated.

Do you know how this happened? If it's a bug in the tooling, I'd like to address it so it doesn't happen again. What process did you use to generate the png files?
Comment 10 Ojan Vafai 2012-05-29 16:19:22 PDT
https://bugs.webkit.org/show_bug.cgi?id=87791 for prettydiff.
Comment 11 Ojan Vafai 2012-05-29 16:56:02 PDT
https://bugs.webkit.org/show_bug.cgi?id=87793 for the linter error.

So, this bug can focus on the following:
1) How these pngs were generated in the first place.
2) Giving better output from run-webkit-tests.
Comment 12 Andras Becsi 2012-05-30 03:44:01 PDT
Created attachment 144776 [details]
proposed patch

This adds platform independent png with checksum and reverts the chromium expectations since layoutTestController.keepWebHistory() does not seem to work for the chromium test infrastructure.
I opened a bug for the issue: https://bugs.webkit.org/show_bug.cgi?id=87839
Comment 13 Andras Becsi 2012-05-30 03:46:21 PDT
(In reply to comment #9)
> (In reply to comment #7)
> Do you know how this happened? If it's a bug in the tooling, I'd like to address it so it doesn't happen again. What process did you use to generate the png files?

It was a human error, not a bug in the tools. Sorry for the inconvenience.
Comment 14 Andras Becsi 2012-05-30 03:49:52 PDT
(In reply to comment #13)
> (In reply to comment #9)
> > (In reply to comment #7)
> > Do you know how this happened? If it's a bug in the tooling, I'd like to address it so it doesn't happen again. What process did you use to generate the png files?

Loos like the mac-wk2 result prior r118566 does not have a checksum, that is what I copied to be a platform independent result.
Comment 15 Andras Becsi 2012-05-30 04:17:58 PDT
(In reply to comment #14)
> Loos like the mac-wk2 result prior r118566 does not have a checksum, that is what I copied to be a platform independent result.

In fact it did have a checksum, which means I ended up copying my mock-up png. Sorry for the noise.
Comment 16 Ojan Vafai 2012-05-30 10:44:12 PDT
Comment on attachment 144776 [details]
proposed patch

This patch looks good, but please upload it to a new bug. In general, webkit has a one patch per bug policy and this bug is about the tooling not giving good feedback when this error is hit. R=me if you upload this to a new bug with the appropriate title.
Comment 17 Joshua Bell 2012-06-12 14:42:52 PDT
Anything else actionable on this bug? If so, please re-open.