Bug 232225 - The script should decide when an image diff passes, not ImageDiff
Summary: The script should decide when an image diff passes, not ImageDiff
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 232222
Blocks: 232288 232294
  Show dependency treegraph
 
Reported: 2021-10-24 21:31 PDT by Simon Fraser (smfr)
Modified: 2021-10-26 10:40 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.02 KB, patch)
2021-10-25 21:21 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2021-10-24 21:31:21 PDT
Currently the ImageDiff binary determines when an image diff passes (diff is less than the tolerance), but it makes more sense for webkitpy to make that decision.
Comment 1 Simon Fraser (smfr) 2021-10-25 21:21:32 PDT
Created attachment 442462 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-10-25 22:53:26 PDT
Comment on attachment 442462 [details]
Patch

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

> Tools/ChangeLog:3
> +        The script should decide when an image diff cases, not ImageDiff

passes, not cases
Comment 3 Martin Robinson 2021-10-26 04:23:18 PDT
Comment on attachment 442462 [details]
Patch

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

This is a nice change.

> Tools/ImageDiff/ImageDiff.cpp:71
> +    fprintf(stdout, "diff: %01.8f%%\n", differenceData.percentageDifference);

This is a nice simplification.

> Tools/Scripts/webkitpy/port/image_diff.py:137
> +            return ImageDiffResult(passed=False, diff_image=None, difference=0, tolerance=self._tolerance, error_string=err_str or "Failed to match ImageDiff output %s" % diff_output)

I think using 

   "Failed to match ImageDiff output {}".format(diff_output)

would be better for future-proofing this code against changes to Python.
Comment 4 EWS 2021-10-26 10:39:26 PDT
Committed r284878 (243555@main): <https://commits.webkit.org/243555@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442462 [details].
Comment 5 Radar WebKit Bug Importer 2021-10-26 10:40:21 PDT
<rdar://problem/84670441>