From 375b4726890d12b0845c40170a681c83f058c4ff Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Sun, 21 Aug 2022 10:21:10 +0100 Subject: [PATCH] Backport reftest comparison machinery from our gtk4 packaging This will let us distinguish between "fails by small differences caused by rounding/i387" and "completely different result", without having to move the whole build system to Meson, which seems like one variation too many during a transition. --- debian/close-enough.keyfile | 3 + debian/ignore.keyfile | 3 + debian/log-reftests.py | 33 ++++ debian/patches/reftest-known-fail.patch | 49 ------ ...ces-Report-how-much-the-images-diffe.patch | 142 ++++++++++++++++++ ...ow-minor-differences-to-be-tolerated.patch | 80 ++++++++++ debian/patches/series | 4 +- ...st-output-directory-to-be-forced-via.patch | 30 ++++ debian/rules | 45 +++--- debian/run-tests.sh | 68 +++++++++ 10 files changed, 390 insertions(+), 67 deletions(-) create mode 100644 debian/close-enough.keyfile create mode 100644 debian/ignore.keyfile create mode 100755 debian/log-reftests.py delete mode 100644 debian/patches/reftest-known-fail.patch create mode 100644 debian/patches/reftest_compare_surfaces-Report-how-much-the-images-diffe.patch create mode 100644 debian/patches/reftests-Allow-minor-differences-to-be-tolerated.patch create mode 100644 debian/patches/testsuite-Allow-reftest-output-directory-to-be-forced-via.patch create mode 100755 debian/run-tests.sh diff --git a/debian/close-enough.keyfile b/debian/close-enough.keyfile new file mode 100644 index 0000000000..1ae6ff92d9 --- /dev/null +++ b/debian/close-enough.keyfile @@ -0,0 +1,3 @@ +[reftest] +tolerated-diff-level=10 +tolerated-diff-pixels=100 diff --git a/debian/ignore.keyfile b/debian/ignore.keyfile new file mode 100644 index 0000000000..28946209a4 --- /dev/null +++ b/debian/ignore.keyfile @@ -0,0 +1,3 @@ +[reftest] +tolerated-diff-level=255 +tolerated-diff-pixels=4000 diff --git a/debian/log-reftests.py b/debian/log-reftests.py new file mode 100755 index 0000000000..96bdda6886 --- /dev/null +++ b/debian/log-reftests.py @@ -0,0 +1,33 @@ +#!/usr/bin/python3 +# Copyright 2021 Simon McVittie +# SPDX-License-Identifier: CC0-1.0 + +import base64 +import sys +from pathlib import Path + +if __name__ == '__main__': + for ui in Path('testsuite', 'reftests').glob('*.ui'): + for outputs in ( + Path( + 'debian', 'build', 'deb', 'testsuite', 'reftests', + 'output', + ), + ): + diff = (outputs / (ui.stem + '.diff.png')) + + if diff.exists(): + ref = (outputs / (ui.stem + '.ref.png')) + out = (outputs / (ui.stem + '.out.png')) + + for path in (ref, out, diff): + if path.exists(): + print('') + print('begin-base64 644 %s' % path) + sys.stdout.flush() + with open(path, 'rb') as reader: + base64.encode(reader, sys.stdout.buffer) + print('====') + print('') + + print('') diff --git a/debian/patches/reftest-known-fail.patch b/debian/patches/reftest-known-fail.patch deleted file mode 100644 index 50c65e9f4c..0000000000 --- a/debian/patches/reftest-known-fail.patch +++ /dev/null @@ -1,49 +0,0 @@ -From: Michael Biebl -Date: Mon, 2 May 2016 01:18:04 +0200 -Subject: Mark known failing tests as non-fatal - -Forwarded: no ---- - testsuite/reftests/gtk-reftest.c | 22 +++++++++++++++++++++- - 1 file changed, 21 insertions(+), 1 deletion(-) - -diff --git a/testsuite/reftests/gtk-reftest.c b/testsuite/reftests/gtk-reftest.c -index 1a51a97..fb5d243 100644 ---- a/testsuite/reftests/gtk-reftest.c -+++ b/testsuite/reftests/gtk-reftest.c -@@ -260,6 +260,20 @@ save_image (cairo_surface_t *surface, - g_free (filename); - } - -+static gboolean -+known_fail(const char *test_name) -+{ -+ char *filename = get_test_file (test_name, ".ui.known_fail", TRUE); -+ -+ if (filename) -+ { -+ g_free (filename); -+ return TRUE; -+ } -+ -+ return FALSE; -+} -+ - static void - test_ui_file (GFile *file) - { -@@ -292,7 +306,13 @@ test_ui_file (GFile *file) - if (diff_image) - { - save_image (diff_image, ui_file, ".diff.png"); -- g_test_fail (); -+ if (known_fail(ui_file)) -+ { -+ printf("KNOWN FAIL: "); -+ g_test_message ("KNOWN FAIL: %s", ui_file); -+ } -+ else -+ g_test_fail (); - } - - remove_extra_css (provider); diff --git a/debian/patches/reftest_compare_surfaces-Report-how-much-the-images-diffe.patch b/debian/patches/reftest_compare_surfaces-Report-how-much-the-images-diffe.patch new file mode 100644 index 0000000000..0d9d5c8508 --- /dev/null +++ b/debian/patches/reftest_compare_surfaces-Report-how-much-the-images-diffe.patch @@ -0,0 +1,142 @@ +From: Simon McVittie +Date: Sat, 13 Feb 2021 18:26:24 +0000 +Subject: reftest_compare_surfaces: Report how much the images differ + +In unattended/non-interactive/autobuilder environments where the images +are not trivially accessible, this provides a way to distinguish between +totally different rendering and more subtle issues. + +Signed-off-by: Simon McVittie +Forwarded: https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/3195 +Applied-upstream: no, upstream want reftests to be a strict pass/fail with identical results required +--- + testsuite/reftests/gtk-reftest.c | 9 ++++++++- + testsuite/reftests/reftest-compare.c | 28 +++++++++++++++++++++++++--- + testsuite/reftests/reftest-compare.h | 5 ++++- + 3 files changed, 37 insertions(+), 5 deletions(-) + +diff --git a/testsuite/reftests/gtk-reftest.c b/testsuite/reftests/gtk-reftest.c +index 1a51a97..5b3d21a 100644 +--- a/testsuite/reftests/gtk-reftest.c ++++ b/testsuite/reftests/gtk-reftest.c +@@ -266,6 +266,9 @@ test_ui_file (GFile *file) + char *ui_file, *reference_file; + cairo_surface_t *ui_image, *reference_image, *diff_image; + GtkStyleProvider *provider; ++ guint max_diff = 0; ++ guint pixels_changed = 0; ++ guint pixels = 0; + + ui_file = g_file_get_path (file); + +@@ -285,12 +288,16 @@ test_ui_file (GFile *file) + } + g_free (reference_file); + +- diff_image = reftest_compare_surfaces (ui_image, reference_image); ++ diff_image = reftest_compare_surfaces (ui_image, reference_image, ++ &max_diff, &pixels_changed, &pixels); + + save_image (ui_image, ui_file, ".out.png"); + save_image (reference_image, ui_file, ".ref.png"); ++ + if (diff_image) + { ++ g_test_message ("%u (out of %u) pixels differ from reference by up to %u levels", ++ pixels_changed, pixels, max_diff); + save_image (diff_image, ui_file, ".diff.png"); + g_test_fail (); + } +diff --git a/testsuite/reftests/reftest-compare.c b/testsuite/reftests/reftest-compare.c +index 84c560c..33379eb 100644 +--- a/testsuite/reftests/reftest-compare.c ++++ b/testsuite/reftests/reftest-compare.c +@@ -84,12 +84,16 @@ buffer_diff_core (const guchar *buf_a, + const guchar *buf_b, + int stride_b, + int width, +- int height) ++ int height, ++ guint *max_diff_out, ++ guint *pixels_changed_out) + { + int x, y; + guchar *buf_diff = NULL; + int stride_diff = 0; + cairo_surface_t *diff = NULL; ++ guint max_diff = 0; ++ guint pixels_changed = 0; + + for (y = 0; y < height; y++) + { +@@ -125,6 +129,10 @@ buffer_diff_core (const guchar *buf_a, + guint diff; + + diff = ABS (value_a - value_b); ++ ++ if (diff > max_diff) ++ max_diff = diff; ++ + diff *= 4; /* emphasize */ + if (diff) + diff += 128; /* make sure it's visible */ +@@ -133,6 +141,8 @@ buffer_diff_core (const guchar *buf_a, + diff_pixel |= diff << (channel*8); + } + ++ pixels_changed++; ++ + if ((diff_pixel & 0x00ffffff) == 0) + { + /* alpha only difference, convert to luminance */ +@@ -144,12 +154,21 @@ buffer_diff_core (const guchar *buf_a, + } + } + ++ if (max_diff_out != NULL) ++ *max_diff_out = max_diff; ++ ++ if (pixels_changed_out != NULL) ++ *pixels_changed_out = pixels_changed; ++ + return diff; + } + + cairo_surface_t * + reftest_compare_surfaces (cairo_surface_t *surface1, +- cairo_surface_t *surface2) ++ cairo_surface_t *surface2, ++ guint *max_diff_out, ++ guint *pixels_changed_out, ++ guint *pixels_out) + { + int w1, h1, w2, h2, w, h; + cairo_surface_t *diff; +@@ -165,7 +184,10 @@ reftest_compare_surfaces (cairo_surface_t *surface1, + cairo_image_surface_get_stride (surface1), + cairo_image_surface_get_data (surface2), + cairo_image_surface_get_stride (surface2), +- w, h); ++ w, h, max_diff_out, pixels_changed_out); ++ ++ if (pixels_out != NULL) ++ *pixels_out = w * h; + + return diff; + } +diff --git a/testsuite/reftests/reftest-compare.h b/testsuite/reftests/reftest-compare.h +index 551b1c5..c6e001c 100644 +--- a/testsuite/reftests/reftest-compare.h ++++ b/testsuite/reftests/reftest-compare.h +@@ -24,7 +24,10 @@ G_BEGIN_DECLS + + G_MODULE_EXPORT + cairo_surface_t * reftest_compare_surfaces (cairo_surface_t *surface1, +- cairo_surface_t *surface2); ++ cairo_surface_t *surface2, ++ guint *max_diff_out, ++ guint *pixels_changed_out, ++ guint *pixels_out); + + G_END_DECLS + diff --git a/debian/patches/reftests-Allow-minor-differences-to-be-tolerated.patch b/debian/patches/reftests-Allow-minor-differences-to-be-tolerated.patch new file mode 100644 index 0000000000..dd94f08a5c --- /dev/null +++ b/debian/patches/reftests-Allow-minor-differences-to-be-tolerated.patch @@ -0,0 +1,80 @@ +From: Simon McVittie +Date: Sat, 13 Feb 2021 16:19:10 +0000 +Subject: reftests: Allow minor differences to be tolerated + +Based on an earlier patch by Michael Biebl, with additional inspiration +from librsvg's reftests. + +Each .ui or .node reftest can have an accompanying .keyfile file +like this: + + [reftest] + tolerated-diff-level=20 + tolerated-diff-pixels=1000 + +If the image differs, but the number of pixels that differ is no more +than tolerated-diff-pixels and the differences are no more than +tolerated-diff-level, then we treat it as a success with warnings, save +the .diff.png for analysis, and use g_test_incomplete() to record the +test-case as "TODO". + +Signed-off-by: Simon McVittie +Forwarded: https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/3195 +Applied-upstream: no, upstream want reftests to be a strict pass/fail with identical results required +--- + testsuite/reftests/gtk-reftest.c | 32 +++++++++++++++++++++++++++++++- + 1 file changed, 31 insertions(+), 1 deletion(-) + +diff --git a/testsuite/reftests/gtk-reftest.c b/testsuite/reftests/gtk-reftest.c +index 5b3d21a..e526bc8 100644 +--- a/testsuite/reftests/gtk-reftest.c ++++ b/testsuite/reftests/gtk-reftest.c +@@ -260,6 +260,12 @@ save_image (cairo_surface_t *surface, + g_free (filename); + } + ++static char * ++get_test_keyfile (const char *ui_file) ++{ ++ return get_test_file (ui_file, ".keyfile", TRUE); ++} ++ + static void + test_ui_file (GFile *file) + { +@@ -296,10 +302,34 @@ test_ui_file (GFile *file) + + if (diff_image) + { ++ char *keyfile_path = get_test_keyfile (ui_file); ++ GKeyFile *keyfile = g_key_file_new (); ++ guint64 tolerated_diff = 0; ++ guint64 tolerated_pixels = 0; ++ ++ if (keyfile_path != NULL) ++ { ++ GError *error = NULL; ++ g_key_file_load_from_file (keyfile, keyfile_path, G_KEY_FILE_NONE, &error); ++ g_assert_no_error (error); ++ tolerated_diff = g_key_file_get_uint64 (keyfile, "reftest", "tolerated-diff-level", NULL); ++ g_test_message ("Maximum difference tolerated: %" G_GUINT64_FORMAT " levels", tolerated_diff); ++ tolerated_pixels = g_key_file_get_uint64 (keyfile, "reftest", "tolerated-diff-pixels", NULL); ++ g_test_message ("Different pixels tolerated: %" G_GUINT64_FORMAT, tolerated_pixels); ++ } ++ + g_test_message ("%u (out of %u) pixels differ from reference by up to %u levels", + pixels_changed, pixels, max_diff); + save_image (diff_image, ui_file, ".diff.png"); +- g_test_fail (); ++ cairo_surface_destroy (diff_image); ++ ++ if (max_diff <= tolerated_diff && pixels_changed <= tolerated_pixels) ++ g_test_message ("not right, but close enough?"); ++ else ++ g_test_fail (); ++ ++ g_key_file_unref (keyfile); ++ g_free (keyfile_path); + } + + remove_extra_css (provider); diff --git a/debian/patches/series b/debian/patches/series index 950fd0d2b0..20efb4a863 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -5,5 +5,7 @@ demos-examples-tests-Don-t-distribute-built-files.patch 016_no_offscreen_widgets_grabbing.patch 017_no_offscreen_device_grabbing.patch 060_ignore-random-icons.patch -reftest-known-fail.patch Disable-accessibility-dump-aka-a11ytests-test.patch +reftest_compare_surfaces-Report-how-much-the-images-diffe.patch +reftests-Allow-minor-differences-to-be-tolerated.patch +testsuite-Allow-reftest-output-directory-to-be-forced-via.patch diff --git a/debian/patches/testsuite-Allow-reftest-output-directory-to-be-forced-via.patch b/debian/patches/testsuite-Allow-reftest-output-directory-to-be-forced-via.patch new file mode 100644 index 0000000000..fface247c6 --- /dev/null +++ b/debian/patches/testsuite-Allow-reftest-output-directory-to-be-forced-via.patch @@ -0,0 +1,30 @@ +From: Simon McVittie +Date: Sun, 21 Aug 2022 10:45:03 +0100 +Subject: testsuite: Allow reftest output directory to be forced via + environment + +On a buildd, we don't want this going into /tmp where we won't find it. +Building with Meson wouldn't need this, but we're currently building +with Autotools and I don't want to change more parameters at once than +I have to. + +Forwarded: not-needed, the Meson build doesn't need this +--- + testsuite/reftests/gtk-reftest.c | 4 ++++ + 1 file changed, 4 insertions(+) + +diff --git a/testsuite/reftests/gtk-reftest.c b/testsuite/reftests/gtk-reftest.c +index e526bc8..1645d8b 100644 +--- a/testsuite/reftests/gtk-reftest.c ++++ b/testsuite/reftests/gtk-reftest.c +@@ -109,6 +109,10 @@ get_output_dir (void) + output_dir = g_file_get_path (file); + g_object_unref (file); + } ++ else if (g_getenv ("REFTEST_OUTPUT_DIR") != NULL) ++ { ++ output_dir = g_getenv ("REFTEST_OUTPUT_DIR"); ++ } + else + { + output_dir = g_get_tmp_dir (); diff --git a/debian/rules b/debian/rules index 52cf9c4ac0..679bca682a 100755 --- a/debian/rules +++ b/debian/rules @@ -132,11 +132,32 @@ configure_flags_udeb = \ %: dh $@ --buildsystem=autoconf +fuzzy_reftests = \ + $(NULL) + +# See testsuite/reftests/meson.build +ignore_reftests = \ + button-wrapping \ + cellrenderer-pixbuf-stock-rtl \ + label-sizing \ + label-wrap-justify \ + quit-mnemonic \ + symbolic-icon-translucent-color \ + window-height-for-width \ + $(NULL) + +# Debian-specific +ignore_reftests += \ + flipping-icons \ + $(NULL) + +xfail_reftests = \ + $(NULL) + override_dh_clean: debian/control # gross kludge to force control generation with the %.in target touch debian/control.in rm -f $(call dh_subst_files,$(all_binaries)) - rm -f testsuite/reftests/*.ui.known_fail rm -rf debian/build debian/install # The build system does not automatically update the GResource files # when resources change. Force an update in case we ship a patch which @@ -207,26 +228,16 @@ override_dh_auto_test: # where rust has been ported to, disable the tests on the other ones ifneq (,$(filter $(DEB_HOST_ARCH), amd64 arm64 armel armhf i386 mips64el mipsel ppc64el s390x powerpc ppc64 riscv64 sparc64)) ifeq (,$(filter nocheck,$(DEB_BUILD_OPTIONS))) - # Mark reftests with known failures as non-fatal - # (See also somehow_broken in testsuite/reftests/meson.build) - touch testsuite/reftests/button-wrapping.ui.known_fail - touch testsuite/reftests/cellrenderer-pixbuf-stock-rtl.ui.known_fail - touch testsuite/reftests/flipping-icons.ui.known_fail - touch testsuite/reftests/label-sizing.ui.known_fail - touch testsuite/reftests/label-wrap-justify.ui.known_fail - touch testsuite/reftests/quit-mnemonic.ui.known_fail - touch testsuite/reftests/symbolic-icon-translucent-color.ui.known_fail - touch testsuite/reftests/window-height-for-width.ui.known_fail # So that gsettings can find the (uninstalled) gtk schemas mkdir -p debian/build/glib-2.0/schemas/ cp gtk/org.gtk.* debian/build/glib-2.0/schemas/ glib-compile-schemas debian/build/glib-2.0/schemas/ - # Remove LD_PRELOAD so we don't run with fakeroot, which makes dbus-related tests fail - env -u LD_PRELOAD GIO_USE_VFS=local GIO_USE_VOLUME_MONITOR=unix \ - dbus-run-session -- \ - xvfb-run -a \ - dh_auto_test --builddirectory=debian/build/deb -- \ - -k check -j1 + env \ + BUILDDIR=$(CURDIR)/debian/build/deb \ + FUZZY_REFTESTS="$(fuzzy_reftests)" \ + IGNORE_REFTESTS="$(ignore_reftests)" \ + XFAIL_REFTESTS="$(xfail_reftests)" \ + debian/run-tests.sh endif endif diff --git a/debian/run-tests.sh b/debian/run-tests.sh new file mode 100755 index 0000000000..d634c57315 --- /dev/null +++ b/debian/run-tests.sh @@ -0,0 +1,68 @@ +#!/bin/sh + +set -ex + +BUILDDIR=${BUILDDIR:-"$(pwd)/debian/build/deb"} + +FUZZY_REFTESTS=${FUZZY_REFTESTS:-} +IGNORE_REFTESTS=${IGNORE_REFTESTS:-} +XFAIL_REFTESTS=${XFAIL_REFTESTS:-} + +test_data="$(mktemp -d -t debian-test-data-XXXXXXXX)" +mkdir -p "$test_data" + +cleanup() { + rm -rf "$test_data" + + # Avoid incremental builds with -nc leaking settings into the next build + for reftest in $FUZZY_REFTESTS $IGNORE_REFTESTS; do + rm -f "testsuite/reftests/$reftest.keyfile" + done +} + +trap 'cleanup' EXIT INT + +for reftest in $FUZZY_REFTESTS; do + cp debian/close-enough.keyfile "testsuite/reftests/$reftest.keyfile" +done + +for reftest in $IGNORE_REFTESTS; do + cp debian/ignore.keyfile "testsuite/reftests/$reftest.keyfile" +done + +# So that gsettings can find the (uninstalled) gtk schemas +mkdir -p "$test_data/glib-2.0/schemas/" +cp gtk/org.gtk.* "$test_data/glib-2.0/schemas/" +glib-compile-schemas "$test_data/glib-2.0/schemas/" + +for BACKEND in x11; do + # Remove LD_PRELOAD so we don't run with fakeroot, which makes dbus-related tests fail + mkdir -p "$BUILDDIR/testsuite/reftests/output" + env \ + -u LD_PRELOAD \ + GIO_USE_VFS=local \ + GIO_USE_VOLUME_MONITOR=unix \ + REFTEST_OUTPUT_DIR="$BUILDDIR/testsuite/reftests/output" \ + dbus-run-session -- \ + xvfb-run -a \ + dh_auto_test --builddirectory="$BUILDDIR" -- \ + -k check -j1 \ + GTESTER="gtester -k --verbose -o gtester.xml" \ + || touch "$test_data/tests-failed" + + # Don't base64-encode the image results for tests that upstream + # expect to fail + for reftest in $XFAIL_REFTESTS; do + rm -f "$BUILDDIR/testsuite/reftests/output/$reftest.diff.png" + done +done + +# gtester unhelpfully suppresses stdout/stderr, add those to the log +find "$BUILDDIR" -name gtester.xml -print0 | xargs -0 -r head -v -n-0 +# Put the images in the log as base64 since we don't have an +# equivalent of AUTOPKGTEST_ARTIFACTS for buildds +debian/log-reftests.py + +if [ -e "$test_data/tests-failed" ]; then + exit 1 +fi