Bug 87552 – Test expectation pngs missing checksums are treated as MISSING by bots

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

[Joshua Bell]

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"

[Marcus Bulach]

Created attachment 144383 [details]
Patch

[Marcus Bulach]

Created attachment 144479 [details]
Patch

[Marcus Bulach]

Committed r118741: <http://trac.webkit.org/changeset/118741>

[Marcus Bulach]

Sorry, forgot to land with --no-close. This bug still exists.

[Ojan Vafai]

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.

[Ojan Vafai]

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.

[Andras Becsi]

(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.

[Dirk Pranke]

(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.

[Ojan Vafai]

(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?

[Ojan Vafai]

https://bugs.webkit.org/show_bug.cgi?id=87791 for prettydiff.

[Ojan Vafai]

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.

[Andras Becsi]

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

[Andras Becsi]

(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.

[Andras Becsi]

(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.

[Andras Becsi]

(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.

[Ojan Vafai]

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.

[Joshua Bell]

Anything else actionable on this bug? If so, please re-open.