[PATCH weston 6/7] tests: add subsurface-shot test
Pekka Paalanen
pekka.paalanen at collabora.co.uk
Tue Feb 7 11:50:59 UTC 2017
On Tue, 7 Feb 2017 12:14:03 +0100
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:
> On 27/01/2017 17:30, Emilio Pozuelo Monfort wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > This is marked as a FAIL_TEST, because the last image comparison fails
> > due to a bug in Weston.
> >
> > Jointly authored by Pekka and Emilio.
> >
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > Signed-off-by: Emilio Pozuelo Monfort <emilio.pozuelo at collabora.co.uk>
>
> A few comments inline.
>
>
> > ---
> > Makefile.am | 12 +-
> > tests/reference/subsurface_z_order-00.png | Bin 0 -> 825 bytes
> > tests/reference/subsurface_z_order-01.png | Bin 0 -> 858 bytes
> > tests/reference/subsurface_z_order-02.png | Bin 0 -> 889 bytes
> > tests/reference/subsurface_z_order-03.png | Bin 0 -> 903 bytes
> > tests/reference/subsurface_z_order-04.png | Bin 0 -> 902 bytes
>
> Do we want even smaller screenshots? Do we have guidelines about it?
There aren't any guidelines yet. These seem small enough to me so far,
but yes, there is a risk of growing the repository size considerably
with reference images. Not sure what to do about that though, we still
want the reference images to be available whenever someone runs the
test suite.
PNG does compress extremely well when using simple patterns though. The
test shell actually helps with that as the panel and the clock caused
the shots to take quite much more space.
>
> > tests/subsurface-shot-test.c | 262 ++++++++++++++++++++++++++++++
> > 7 files changed, 273 insertions(+), 1 deletion(-)
> > create mode 100644 tests/reference/subsurface_z_order-00.png
> > create mode 100644 tests/reference/subsurface_z_order-01.png
> > create mode 100644 tests/reference/subsurface_z_order-02.png
> > create mode 100644 tests/reference/subsurface_z_order-03.png
> > create mode 100644 tests/reference/subsurface_z_order-04.png
> > create mode 100644 tests/subsurface-shot-test.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 3932095a..458dabb1 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -1213,6 +1213,7 @@ weston_tests = \
> > viewporter.weston \
> > roles.weston \
> > subsurface.weston \
> > + subsurface-shot.weston \
> > devices.weston
> >
> > ivi_tests =
> > @@ -1373,6 +1374,10 @@ subsurface_weston_SOURCES = tests/subsurface-test.c
> > subsurface_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> > subsurface_weston_LDADD = libtest-client.la
> >
> > +subsurface_shot_weston_SOURCES = tests/subsurface-shot-test.c
> > +subsurface_shot_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> > +subsurface_shot_weston_LDADD = libtest-client.la
> > +
> > presentation_weston_SOURCES = \
> > tests/presentation-test.c \
> > shared/helpers.h
> > @@ -1492,7 +1497,12 @@ EXTRA_DIST += \
> > tests/weston-tests-env \
> > tests/internal-screenshot.ini \
> > tests/reference/internal-screenshot-bad-00.png \
> > - tests/reference/internal-screenshot-good-00.png
> > + tests/reference/internal-screenshot-good-00.png \
> > + tests/reference/subsurface_z_order-00.png \
> > + tests/reference/subsurface_z_order-01.png \
> > + tests/reference/subsurface_z_order-02.png \
> > + tests/reference/subsurface_z_order-03.png \
> > + tests/reference/subsurface_z_order-04.png
>
> We should put weston-tests-env (or other file that will never change) at
> the end of the list, it’d avoid one line diff each time. [Cosmetics thought]
We could, yeah.
> Or a specific variables to list screenshots (with "$(null)" at the end
> for the one line diff too)?
I'm not too fond of $(null) but OTOH the one line extra does not bother
me either.
> > diff --git a/tests/subsurface-shot-test.c b/tests/subsurface-shot-test.c
> > new file mode 100644
> > index 00000000..29b03c41
> > --- /dev/null
> > +++ b/tests/subsurface-shot-test.c
> > +char *server_parameters = "--use-pixman --width=320 --height=240"
> > + " --shell=weston-test-desktop-shell.so";
>
> Would there be a way to tag a test as screenshot so it would happen with
> --use-pixman and the test shell?
This is the way. ;-)
More seriously, I really would not want to add even more magic into the
weston-tests-env script based running. I am still dreaming of setting
up all that in C, from inside the test programs themselves, including
launching weston. We had a long discussion about that at Collabora
recently, actually.
> > +static int
> > +check_screen(struct client *client,
> > + const char *ref_image,
> > + int ref_seq_no,
> > + const struct rectangle *clip,
> > + int seq_no)
> > +{
> > + const char *test_name = get_test_name();
> > + struct buffer *shot;
> > + pixman_image_t *ref;
> > + char *ref_fname;
> > + char *shot_fname;
> > + bool match;
> > +
> > + ref_fname = screenshot_reference_filename(ref_image, ref_seq_no);
> > + shot_fname = screenshot_output_filename(test_name, seq_no);
> > +
> > + ref = load_image_from_png(ref_fname);
> > + assert(ref);
> > +
> > + shot = capture_screenshot_of_output(client);
> > + assert(shot);
> > +
> > + match = check_images_match(shot->image, ref, clip);
> > + printf("ref %s vs. shot %s: %s\n", ref_fname, shot_fname,
> > + match ? "ok" : "FAIL");
>
> PASS/FAIL?
I can change that.
> > +
> > + write_image_as_png(shot->image, shot_fname);
> > + write_visual_diff(ref, shot, clip, test_name, seq_no);
>
> Why write diff on PASS?
Forgotten from debugging the diff writing itself, I think.
I'll fix it.
> > +
> > + buffer_destroy(shot);
> > + pixman_image_unref(ref);
> > + free(ref_fname);
> > + free(shot_fname);
> > +
> > + return match ? 0 : -1;
> > +}
> > +
> > + fail += check_screen(client, test_name, 4, &clip, 4);
> > +
> > + assert(fail == 0);
>
> Nit: move it after the destroys maybe?
*shrug* I don't see a difference.
> > +
> > + for (i = 0; i < ARRAY_LENGTH(sub); i++)
> > + if (sub[i])
> > + wl_subsurface_destroy(sub[i]);
> > +
> > + for (i = 0; i < ARRAY_LENGTH(surf); i++)
> > + if (surf[i])
> > + wl_surface_destroy(surf[i]);
> > +
> > + for (i = 0; i < ARRAY_LENGTH(bufs); i++)
> > + if (bufs[i])
> > + buffer_destroy(bufs[i]);
> > +}
> >
>
> I’m not knowledgeable enough on subsurface commit order to check the
> “real” stuff, but the rest looks pretty good. I can at least give:
> Acked-by: Quentin Glidic <sardemff7+git at sardemff7.net>
Cool, thanks.
pq
More information about the wayland-devel
mailing list