Run Skia Gold as part of each pixel test
Runs Skia Gold as part of each pixel test, rather than after all pixel
tests have run. This simplifies the code, and more closely matches Skia
Gold's role as asserting on pixel test output, rather than acting as a
test itself.
Since each pixel test can produce multiple output images, and the
previous implementation submitted each output image to the
multiprocessing queue independently, this may lead to a small reduction
in parallelism. On the other hand, Skia Gold operations can now overlap
with pixel test operations. I expect insignificant impact overall.
Bug: pdfium:1916
Change-Id: I8faa2733c5d7a6b21ee5c487df1dc33c8d0febc0
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/99710
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: K. Moon <kmoon@chromium.org>
diff --git a/testing/tools/test_runner.py b/testing/tools/test_runner.py
index 615d7bd..2412246 100644
--- a/testing/tools/test_runner.py
+++ b/testing/tools/test_runner.py
@@ -96,6 +96,16 @@
self.failures.append(test_case.input_path)
result_status = result_types.FAIL
+ if test_result.image_artifacts:
+ for artifact in test_result.image_artifacts:
+ if artifact.skia_gold_status == result_types.PASS:
+ if self.IsResultSuppressed(artifact.image_path):
+ self.skia_gold_unexpected_successes.append(artifact.GetSkiaGoldId())
+ else:
+ self.skia_gold_successes.append(artifact.GetSkiaGoldId())
+ elif artifact.skia_gold_status == result_types.FAIL:
+ self.skia_gold_failures.append(artifact.GetSkiaGoldId())
+
if self.resultdb:
# TODO(crbug.com/pdfium/1916): Populate more ResultDB fields.
self.resultdb.Post(
@@ -271,27 +281,9 @@
processes=self.options.num_workers,
initializer=_InitializePerProcessState,
initargs=[self.per_process_config]) as pool:
- skia_gold_test_cases = TestCaseManager()
for result in pool.imap(_RunPdfiumTest, self.test_cases):
self.HandleResult(self.test_cases.GetTestCase(result.test_id), result)
- if self.IsSkiaGoldEnabled():
- for artifact in result.image_artifacts:
- # The output filename without image extension becomes the test ID.
- # For example, "/path/to/.../testing/corpus/example_005.pdf.0.png"
- # becomes "example_005.pdf.0".
- skia_gold_test_cases.NewTestCase(artifact.image_path)
-
- for result in pool.imap(_RunSkiaTest, skia_gold_test_cases):
- if result.IsPass():
- test_case = skia_gold_test_cases.GetTestCase(result.test_id)
- if self.IsResultSuppressed(test_case.input_path):
- self.skia_gold_unexpected_successes.append(result.test_id)
- else:
- self.skia_gold_successes.append(result.test_id)
- else:
- self.skia_gold_failures.append(result.test_id)
-
# Report test results.
#
# For some reason, summary will be cut off from stdout on windows if
@@ -378,20 +370,6 @@
raise KeyboardInterruptError() from exc
-def _RunSkiaTest(test_case):
- """Runs a Skia Gold test case."""
- try:
- skia_tester = _per_process_state.GetSkiaGoldTester()
- skia_success = skia_tester.UploadTestResultToSkiaGold(
- test_case.test_id, test_case.input_path)
- sys.stdout.flush()
-
- return test_case.NewResult(
- result_types.PASS if skia_success else result_types.FAIL)
- except KeyboardInterrupt as exc:
- raise KeyboardInterruptError() from exc
-
-
# `_PerProcessState` singleton. This is initialized when creating the
# `multiprocessing.Pool()`. `TestRunner.Run()` creates its own separate
# instance of `_PerProcessState` as well.
@@ -640,11 +618,23 @@
raised_exception, results = common.RunCommandExtractHashedFiles(cmd_to_run)
if results:
results = [
- ImageArtifact(image_path=image_path, md5_hash=md5_hash)
+ self._NewImageArtifact(image_path=image_path, md5_hash=md5_hash)
for image_path, md5_hash in results
]
return raised_exception, results
+ def _NewImageArtifact(self, *, image_path, md5_hash):
+ artifact = ImageArtifact(image_path=image_path, md5_hash=md5_hash)
+
+ if self.options.run_skia_gold:
+ if self.GetSkiaGoldTester().UploadTestResultToSkiaGold(
+ artifact.GetSkiaGoldId(), artifact.image_path):
+ artifact.skia_gold_status = result_types.PASS
+ else:
+ artifact.skia_gold_status = result_types.FAIL
+
+ return artifact
+
@dataclass
class TestCase:
@@ -689,9 +679,17 @@
Attributes:
image_path: The absolute path to the image file.
md5_hash: The MD5 hash of the pixel buffer.
+ skia_gold_status: Optional Skia Gold status.
"""
image_path: str
md5_hash: str
+ skia_gold_status: str = None
+
+ def GetSkiaGoldId(self):
+ # The output filename without image extension becomes the test ID. For
+ # example, "/path/to/.../testing/corpus/example_005.pdf.0.png" becomes
+ # "example_005.pdf.0".
+ return _GetTestId(os.path.basename(self.image_path))
class TestCaseManager:
@@ -709,7 +707,7 @@
def NewTestCase(self, input_path, **kwargs):
"""Creates and registers a new test case."""
input_basename = os.path.basename(input_path)
- test_id, _ = os.path.splitext(input_basename)
+ test_id = _GetTestId(input_basename)
if test_id in self.test_cases:
raise ValueError(
f'Test ID "{test_id}" derived from "{input_basename}" must be unique')
@@ -723,6 +721,11 @@
return self.test_cases[test_id]
+def _GetTestId(input_basename):
+ """Constructs a test ID by stripping the last extension from the basename."""
+ return os.path.splitext(input_basename)[0]
+
+
class _TestTimer:
"""Context manager for measuring the duration of a test."""
@@ -735,5 +738,7 @@
return self
def __exit__(self, exc_type, exc_value, traceback):
+ if not self.result:
+ return
duration = time.perf_counter_ns() - self.duration_start
self.result.duration_milliseconds = duration * 1e-6
