Improve --regenerate_expected
Improves --regenerate_expected to regenerate the most specific expected
images that exist, which is what we want in most cases. In the rare case
that we want to regenerate a less specific expectation, we should delete
the more specific expectation first: Otherwise, we leave stale,
unnecessary expectations behind.
Note that this eliminates the need to specify whether we're regenerating
"all" or "platform" expectations, as any existing "platform" expectation
is more specific than the "all" expectation.
This requires treating the expectation order as a tree: Rather than AGG
being a strict, linear subset of Skia, AGG and Skia should have
branching expectations, with a common base of generic expectations.
This change also makes additional improvements to pngdiffer.py:
1. Fixes error reporting when expected files are missing.
2. Checks for "optipng" not just on Linux, since Regenerate() tries to
execute it on all platforms anyway.
3. Prefixes more module-internal symbols with "_".
4. Migrates from distutils, which will be removed in Python 3.12.
This change also improves argparse usage in test_runner.py:
1. Quotes string literals consistently, using single quotes.
2. Drops the "dest" parameter if it's the same as the default.
Fixed: pdfium:1508
Change-Id: I39aa77d198a513fe92ea47f70aa6381b42ecce44
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/103710
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 c95da5a..39ced20 100755
--- a/testing/tools/pngdiffer.py
+++ b/testing/tools/pngdiffer.py
@@ -4,32 +4,17 @@
# found in the LICENSE file.
from dataclasses import dataclass
-import distutils.spawn
import itertools
import os
import shutil
import subprocess
import sys
+_PNG_OPTIMIZER = 'optipng'
-class PathMode:
- """PathMode indicates the available expected results' paths.
-
- Attributes:
- DEFAULT: Used for default expected paths in the format of
- 'NAME_expected(_OSNAME)?.pdf.#.png'. For a test, this path always
- exists.
- SKIA: Used when Skia is enabled, for paths in the format of
- 'NAME_expected_skia(_OSNAME)?.pdf.#.png'.
- Such paths only exist when the expected results of Skia are
- different from those of AGG.
-
- Always check PathMode in an incrementing order as the modes are listed in
- order of its matching paths' precedence.
- """
-
- DEFAULT = 0
- SKIA = 1
+_COMMON_SUFFIX_ORDER = ('_{os}', '')
+_AGG_SUFFIX_ORDER = ('_agg_{os}', '_agg') + _COMMON_SUFFIX_ORDER
+_SKIA_SUFFIX_ORDER = ('_skia_{os}', '_skia') + _COMMON_SUFFIX_ORDER
@dataclass
@@ -47,7 +32,6 @@
diff_path: str = None
reason: str = None
-
class PNGDiffer():
def __init__(self, finder, features, reverse_byte_order):
@@ -55,25 +39,23 @@
self.os_name = finder.os_name
self.reverse_byte_order = reverse_byte_order
if 'SKIA' in features:
- self.max_path_mode = PathMode.SKIA
+ self.suffix_order = _SKIA_SUFFIX_ORDER
else:
- self.max_path_mode = PathMode.DEFAULT
+ self.suffix_order = _AGG_SUFFIX_ORDER
def CheckMissingTools(self, regenerate_expected):
- if (regenerate_expected and self.os_name == 'linux' and
- not distutils.spawn.find_executable('optipng')):
- return 'Please install "optipng" to regenerate expected images.'
+ if regenerate_expected and not shutil.which(_PNG_OPTIMIZER):
+ return f'Please install "{_PNG_OPTIMIZER}" to regenerate expected images.'
return None
def GetActualFiles(self, input_filename, source_dir, working_dir):
actual_paths = []
- path_templates = PathTemplates(input_filename, source_dir, working_dir,
- self.os_name, self.max_path_mode)
+ path_templates = _PathTemplates(input_filename, source_dir, working_dir,
+ self.os_name, self.suffix_order)
for page in itertools.count():
actual_path = path_templates.GetActualPath(page)
- expected_paths = path_templates.GetExpectedPaths(page)
- if any(map(os.path.exists, expected_paths)):
+ if path_templates.GetExpectedPath(page, default_to_base=False):
actual_paths.append(actual_path)
else:
break
@@ -108,8 +90,8 @@
"""
image_diffs = []
- path_templates = PathTemplates(input_filename, source_dir, working_dir,
- self.os_name, self.max_path_mode)
+ path_templates = _PathTemplates(input_filename, source_dir, working_dir,
+ self.os_name, self.suffix_order)
for page in itertools.count():
page_diff = ImageDiff(actual_path=path_templates.GetActualPath(page))
if not os.path.exists(page_diff.actual_path):
@@ -151,11 +133,9 @@
return image_diffs
- # TODO(crbug.com/pdfium/1508): Add support to automatically generate Skia
- # specific expected results.
- def Regenerate(self, input_filename, source_dir, working_dir, platform_only):
- path_templates = PathTemplates(input_filename, source_dir, working_dir,
- self.os_name, self.max_path_mode)
+ 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
@@ -164,52 +144,39 @@
if not os.path.isfile(actual_path):
break
- platform_expected_path = path_templates.GetExpectedPathByPathMode(
- page, PathMode.DEFAULT, self.os_name)
-
- # If there is a platform expected png, we will overwrite it. Otherwise,
- # overwrite the generic png in "all" mode, or do nothing in "platform"
- # mode.
- if os.path.exists(platform_expected_path):
- expected_path = platform_expected_path
- elif not platform_only:
- expected_path = path_templates.GetExpectedPathByPathMode(
- page, PathMode.DEFAULT)
- else:
- continue
-
+ # Regenerate the most specific expected path that exists. If there are no
+ # existing expectations, regenerate the base case.
+ #
+ # TODO(crbug.com/pdfium/1987): Clean up redundant expectations.
+ expected_path = path_templates.GetExpectedPath(page)
shutil.copyfile(actual_path, expected_path)
- self._RunCommand(['optipng', expected_path])
+ self._RunCommand([_PNG_OPTIMIZER, expected_path])
-ACTUAL_TEMPLATE = '.pdf.%d.png'
-DIFF_TEMPLATE = '.pdf.%d.diff.png'
+_ACTUAL_TEMPLATE = '.pdf.%d.png'
+_DIFF_TEMPLATE = '.pdf.%d.diff.png'
-class PathTemplates:
+class _PathTemplates:
def __init__(self, input_filename, source_dir, working_dir, os_name,
- max_path_mode):
- assert PathMode.DEFAULT <= max_path_mode <= PathMode.SKIA, (
- 'Unexpected Maximum PathMode: %d.' % max_path_mode)
-
+ suffix_order):
input_root, _ = os.path.splitext(input_filename)
self.actual_path_template = os.path.join(working_dir,
- input_root + ACTUAL_TEMPLATE)
+ input_root + _ACTUAL_TEMPLATE)
self.diff_path_template = os.path.join(working_dir,
- input_root + DIFF_TEMPLATE)
- self.source_dir = source_dir
- self.input_root = input_root
- self.max_path_mode = max_path_mode
- self.os_name = os_name
+ input_root + _DIFF_TEMPLATE)
- # Pre-create the available templates depending on |max_path_mode|.
+ # Pre-create the available templates from most to least specific. We
+ # generally expect the most specific case to match first.
self.expected_templates = []
- for mode in range(PathMode.DEFAULT, max_path_mode + 1):
- self.expected_templates.extend([
- self._GetExpectedTemplateByPathMode(mode),
- self._GetExpectedTemplateByPathMode(mode, os_name),
- ])
+ for suffix in suffix_order:
+ formatted_suffix = suffix.format(os=os_name)
+ self.expected_templates.append(
+ os.path.join(
+ source_dir,
+ f'{input_root}_expected{formatted_suffix}{_ACTUAL_TEMPLATE}'))
+ assert self.expected_templates
def GetActualPath(self, page):
return self.actual_path_template % page
@@ -217,22 +184,14 @@
def GetDiffPath(self, page):
return self.diff_path_template % page
- def _GetExpectedTemplateByPathMode(self, mode, os_name=None):
- expected_str = '_expected'
- if mode == PathMode.DEFAULT:
- pass
- elif mode == PathMode.SKIA:
- expected_str += '_skia'
- else:
- assert False, 'Unexpected PathMode: %d.' % mode
-
- if os_name:
- expected_str += '_' + self.os_name
- return os.path.join(self.source_dir,
- self.input_root + expected_str + ACTUAL_TEMPLATE)
-
- def GetExpectedPathByPathMode(self, page, mode, os_name=None):
- return self._GetExpectedTemplateByPathMode(mode, os_name) % page
-
def GetExpectedPaths(self, page):
return [template % page for template in self.expected_templates]
+
+ 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):
+ if os.path.exists(expected_path):
+ return expected_path
+ last_not_found_expected_path = expected_path
+ return last_not_found_expected_path if default_to_base else None
diff --git a/testing/tools/test_runner.py b/testing/tools/test_runner.py
index 713812a..7fe2ee9 100644
--- a/testing/tools/test_runner.py
+++ b/testing/tools/test_runner.py
@@ -177,20 +177,17 @@
parser.add_argument(
'--disable-javascript',
- action="store_true",
- dest="disable_javascript",
+ action='store_true',
help='Prevents JavaScript from executing in PDF files.')
parser.add_argument(
'--disable-xfa',
- action="store_true",
- dest="disable_xfa",
+ action='store_true',
help='Prevents processing XFA forms.')
parser.add_argument(
'--render-oneshot',
- action="store_true",
- dest="render_oneshot",
+ action='store_true',
help='Sets whether to use the oneshot renderer.')
parser.add_argument(
@@ -203,36 +200,29 @@
parser.add_argument(
'--gold_properties',
default='',
- dest="gold_properties",
- help='Key value pairs that are written to the top level '
- 'of the JSON file that is ingested by Gold.')
+ help='Key value pairs that are written to the top level of the JSON '
+ 'file that is ingested by Gold.')
# TODO: Remove when pdfium recipe stops passing this argument
parser.add_argument(
'--gold_ignore_hashes',
default='',
- dest="gold_ignore_hashes",
help='Path to a file with MD5 hashes we wish to ignore.')
parser.add_argument(
'--regenerate_expected',
- default='',
- dest="regenerate_expected",
- help='Regenerates expected images. Valid values are '
- '"all" to regenerate all expected pngs, and '
- '"platform" to regenerate only platform-specific '
- 'expected pngs.')
+ action='store_true',
+ help='Regenerates expected images. For each failing image diff, this '
+ 'will regenerate the most specific expected image file that exists.')
parser.add_argument(
'--reverse-byte-order',
action='store_true',
- dest="reverse_byte_order",
help='Run image-based tests using --reverse-byte-order.')
parser.add_argument(
'--ignore_errors',
- action="store_true",
- dest="ignore_errors",
+ action='store_true',
help='Prevents the return value from being non-zero '
'when image comparison fails.')
@@ -248,11 +238,6 @@
self.per_process_config.options = parser.parse_args()
- if (self.options.regenerate_expected and
- self.options.regenerate_expected not in ['all', 'platform']):
- print('FAILURE: --regenerate_expected must be "all" or "platform"')
- return 1
-
finder = self.per_process_config.NewFinder()
pdfium_test_path = self.per_process_config.GetPdfiumTestPath(finder)
if not os.path.exists(pdfium_test_path):
@@ -611,18 +596,14 @@
return test_function()
- # TODO(crbug.com/pdfium/1508): Add support for an option to automatically
- # generate Skia specific expected results.
def _RegenerateIfNeeded(self):
if not self.options.regenerate_expected:
return
if self.IsResultSuppressed() or self.IsImageDiffSuppressed():
return
- _per_process_state.image_differ.Regenerate(
- self.input_filename,
- self.source_dir,
- self.working_dir,
- platform_only=self.options.regenerate_expected == 'platform')
+ _per_process_state.image_differ.Regenerate(self.input_filename,
+ self.source_dir,
+ self.working_dir)
def Generate(self):
input_event_path = os.path.join(self.source_dir, f'{self.test_id}.evt')
@@ -748,8 +729,11 @@
diff_log = []
for diff in image_diffs:
diff_map[diff.actual_path] = diff
- diff_log.append((f'{os.path.basename(diff.actual_path)} vs. '
- f'{os.path.basename(diff.expected_path)}\n'))
+ diff_log.append(f'{os.path.basename(diff.actual_path)} vs. ')
+ if diff.expected_path:
+ diff_log.append(f'{os.path.basename(diff.expected_path)}\n')
+ else:
+ diff_log.append('missing expected file\n')
for artifact in test_result.image_artifacts:
artifact.image_diff = diff_map.get(artifact.image_path)