Bug 58402 – old-run-webkit-tests/new-run-webkit-tests shouldn't read or write .checksum files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

old-run-webkit-tests shouldn't write .checksum files

[Tony Chang]

Created attachment 89313 [details]
Patch

[Tony Chang]

I'll send a note to webkit-dev before landing this.

[Tony Chang]

Feel free to suggest other reviewers.

[Adam Roben (:aroben)]

Comment on attachment 89313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89313&action=review

> Tools/Scripts/old-run-webkit-tests:936
> +            if (-f $expectedChecksumPath) {
> +                unlink $expectedChecksumPath;
> +            }

This seems strange. People are going to have files disappearing from their working trees without any explanation. I think it would be a lot better to just do a one-time delete of all .checksum files in the WebKit tree and commit that separately.

[Tony Chang]

(In reply to comment #4)
> This seems strange. People are going to have files disappearing from their working trees without any explanation. I think it would be a lot better to just do a one-time delete of all .checksum files in the WebKit tree and commit that separately.

I said I would send a note to webkit-dev before landing this :)

But if you prefer, I can update the existing pngs and remove the .checksum files first.  The downside is there will be a window of time between the patches (or patches created before the .checksum delete and landed after) where people will be checking in .checksums after I've deleted all of them.  I guess I can just do a second pass a week later.

[Adam Roben (:aroben)]

Seems like the steps could be:

1) Embed checksums in all existing PNGs
2) Change ORWT not to read or write .checksum files
3) Delete all .checksum files

These could each be done separately.

[Tony Chang]

(In reply to comment #6)
> Seems like the steps could be:
> 
> 1) Embed checksums in all existing PNGs
> 2) Change ORWT not to read or write .checksum files
> 3) Delete all .checksum files
> 
> These could each be done separately.

That sounds fine to me.  This is part of step 2 (the read part is done).  I'll come back and r? this once step 1 is done.

[Tony Chang]

*** Bug 57575 has been marked as a duplicate of this bug. ***

[Tony Chang]

Created attachment 90566 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 90566 [details]
Patch

The ORWT changes look good to me. Someone more familiar with NRWT should review that part.

[Tony Chang]

(In reply to comment #6)
> 1) Embed checksums in all existing PNGs
> 2) Change ORWT not to read or write .checksum files
> 3) Delete all .checksum files


(1) is done.  This is (2).  I'll send a note to webkit-dev after landing this.  It'll be harmless if people continue to check in .checksum files for a few days.  They'll just be ignored.

[Tony Chang]

Created attachment 90567 [details]
add comment back to ORWT

[Ojan Vafai]

Comment on attachment 90567 [details]
add comment back to ORWT

View in context: https://bugs.webkit.org/attachment.cgi?id=90567&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:149
>          if self._options.pixel_tests and driver_output.image_hash:

maybe this check should be driver_output.image instead of driver_output.image_hash now?

[Tony Chang]

Committed r84544: <http://trac.webkit.org/changeset/84544>

[Tony Chang]

(In reply to comment #13)
> (From update of attachment 90567 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90567&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:149
> >          if self._options.pixel_tests and driver_output.image_hash:
> 
> maybe this check should be driver_output.image instead of driver_output.image_hash now?

I switched this before committing.  There's a lot of code in NRWT that we can update/delete now, but I'll do that in a follow up patch.