Tighten expected image selection
Makes expected image selection stricter by only allowing the most
specific expected path that exists to match. An image diff test now will
fail if a less specific expected path matches.
Bug: pdfium:1987
Change-Id: Ie881e344db046a1eff242e759b60581c165c3f89
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/103750
Commit-Queue: K. Moon <kmoon@chromium.org>
Reviewed-by: Nigi <nigi@chromium.org>
diff --git a/testing/tools/pngdiffer.py b/testing/tools/pngdiffer.py
index 39ced20..b2e70d2 100755
--- a/testing/tools/pngdiffer.py
+++ b/testing/tools/pngdiffer.py
@@ -95,53 +95,47 @@
for page in itertools.count():
page_diff = ImageDiff(actual_path=path_templates.GetActualPath(page))
if not os.path.exists(page_diff.actual_path):
+ # No more actual pages.
break
- last_not_found_expected_path = None
- compare_error = None
- for expected_path in path_templates.GetExpectedPaths(page):
- if not os.path.exists(expected_path):
- last_not_found_expected_path = expected_path
- continue
+ expected_path = path_templates.GetExpectedPath(page)
+ if os.path.exists(expected_path):
page_diff.expected_path = expected_path
compare_error = self._RunImageCompareCommand(page_diff)
- if not compare_error:
- # Found a match.
- break
+ if compare_error:
+ page_diff.reason = str(compare_error)
- if page_diff.expected_path:
- if not compare_error:
- # Proceed to next page
- continue
- page_diff.reason = str(compare_error)
-
- # TODO(crbug.com/pdfium/1925): Compare and diff in a single invocation.
- page_diff.diff_path = path_templates.GetDiffPath(page)
- if not self._RunImageDiffCommand(page_diff):
- print(f'WARNING: No diff for {page_diff.actual_path}')
- page_diff.diff_path = None
+ # TODO(crbug.com/pdfium/1925): Compare and diff simultaneously.
+ page_diff.diff_path = path_templates.GetDiffPath(page)
+ if not self._RunImageDiffCommand(page_diff):
+ print(f'WARNING: No diff for {page_diff.actual_path}')
+ page_diff.diff_path = None
+ else:
+ # Validate that no other paths match.
+ for unexpected_path in path_templates.GetExpectedPaths(page)[1:]:
+ page_diff.expected_path = unexpected_path
+ if not self._RunImageCompareCommand(page_diff):
+ page_diff.reason = f'Also matches {unexpected_path}'
+ break
+ page_diff.expected_path = expected_path
else:
if page == 0:
print(f'WARNING: no expected results files for {input_filename}')
- if last_not_found_expected_path:
- page_diff.reason = f'{last_not_found_expected_path} does not exist'
- else:
- page_diff.reason = f'Missing expected result for 0-based page {page}'
+ page_diff.reason = f'{expected_path} does not exist'
- image_diffs.append(page_diff)
+ if page_diff.reason:
+ image_diffs.append(page_diff)
return image_diffs
def Regenerate(self, input_filename, source_dir, working_dir):
path_templates = _PathTemplates(input_filename, source_dir, working_dir,
self.os_name, self.suffix_order)
-
for page in itertools.count():
- # Loop through the generated page images. Stop when there is a page
- # missing a png, which means the document ended.
- actual_path = path_templates.GetActualPath(page)
- if not os.path.isfile(actual_path):
+ page_diff = ImageDiff(actual_path=path_templates.GetActualPath(page))
+ if not os.path.isfile(page_diff.actual_path):
+ # No more actual pages.
break
# Regenerate the most specific expected path that exists. If there are no
@@ -149,7 +143,7 @@
#
# TODO(crbug.com/pdfium/1987): Clean up redundant expectations.
expected_path = path_templates.GetExpectedPath(page)
- shutil.copyfile(actual_path, expected_path)
+ shutil.copyfile(page_diff.actual_path, expected_path)
self._RunCommand([_PNG_OPTIMIZER, expected_path])
@@ -184,13 +178,16 @@
def GetDiffPath(self, page):
return self.diff_path_template % page
- def GetExpectedPaths(self, page):
+ def _GetPossibleExpectedPaths(self, page):
return [template % page for template in self.expected_templates]
+ def GetExpectedPaths(self, page):
+ return list(filter(os.path.exists, self._GetPossibleExpectedPaths(page)))
+
def GetExpectedPath(self, page, default_to_base=True):
"""Returns the most specific expected path that exists."""
last_not_found_expected_path = None
- for expected_path in self.GetExpectedPaths(page):
+ for expected_path in self._GetPossibleExpectedPaths(page):
if os.path.exists(expected_path):
return expected_path
last_not_found_expected_path = expected_path
diff --git a/testing/tools/test_runner.py b/testing/tools/test_runner.py
index 7fe2ee9..d72ca33 100644
--- a/testing/tools/test_runner.py
+++ b/testing/tools/test_runner.py
@@ -104,8 +104,12 @@
# Report test result to ResultDB.
if self.resultdb:
only_artifacts = None
+ only_failure_reason = test_result.reason
if len(test_result.image_artifacts) == 1:
- only_artifacts = test_result.image_artifacts[0].GetDiffArtifacts()
+ only = test_result.image_artifacts[0]
+ only_artifacts = only.GetDiffArtifacts()
+ if only.GetDiffReason():
+ only_failure_reason += f': {only.GetDiffReason()}'
self.resultdb.Post(
test_id=test_result.test_id,
status=test_result.status,
@@ -113,7 +117,7 @@
test_log=test_result.log,
test_file=None,
artifacts=only_artifacts,
- failure_reason=test_result.reason)
+ failure_reason=only_failure_reason)
# Milo only supports a single diff per test, so if we have multiple pages,
# report each page as its own "test."
