[PATCH wayland 3/3] tests: add scanner tests

[Emilio Pozuelo Monfort]

On 11/11/16 15:56, Pekka Paalanen wrote:
> On Thu, 10 Nov 2016 12:18:55 +0200
> Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 
>> On Thu, 10 Nov 2016 11:11:51 +0100
>> Emilio Pozuelo Monfort <pochu27 at gmail.com> wrote:
>>
>>> On 10/11/16 10:57, Pekka Paalanen wrote:  
>>>> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>>>
>>>> Add tests that ensure that wayland-scanner output for a given input does
>>>> not change unexpectedly. This makes it very easy to review
>>>> wayland-scanner patches.
>>>>
>>>> Before, when patches were proposed for wayland-scanner, I had to
>>>> build wayland without the patches, save the generated files into a
>>>> temporary directory, apply the patches, build again, and diff the old
>>>> vs. new generated file.
>>>>
>>>> No more. Now whenever someone makes intentional changes to
>>>> wayland-scanner's output, he will also have to patch the example output
>>>> files to match. That means that reviewers see the diff of the generated
>>>> files straight from the patch itself. Verifying the diff is true is as
>>>> easy as 'make check'.
>>>>
>>>> The tests use separate example XML files instead of wayland.xml
>>>> directly, so that wayland.xml can be updated without fixing scanner
>>>> tests, avoiding the churn.
>>>>
>>>> example.xml starts as a copy of wayland.xml. If wayland.xml starts using
>>>> new wayland-scanner features, they should be copied into example.xml
>>>> again to be covered by the tests.
>>>>
>>>> This patch relies on the previous patch to actually add all the data
>>>> files for input and reference output.
>>>>
>>>> The scanner output is fed through sed to remove parts that are allowed
>>>> to vary: the scanner version string.
>>>>
>>>> Task: https://phabricator.freedesktop.org/T3313
>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>>> ---
>>>>  .gitignore            |  1 +
>>>>  Makefile.am           | 34 +++++++++++++++++++++++++++++++++-
>>>>  tests/scanner-test.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 85 insertions(+), 1 deletion(-)
>>>>  create mode 100755 tests/scanner-test.sh
>>>>
>>>> diff --git a/.gitignore b/.gitignore
>>>> index 33e809c..8da9861 100644
>>>> --- a/.gitignore
>>>> +++ b/.gitignore
>>>> @@ -38,6 +38,7 @@ ctags
>>>>  /missing
>>>>  /stamp-h1
>>>>  /test-driver
>>>> +/tests/output/
>>>>  Makefile
>>>>  Makefile.in
>>>>  exec-fd-leak-checker
>>>> diff --git a/Makefile.am b/Makefile.am
>>>> index d35231c..cd21915 100644
>>>> --- a/Makefile.am
>>>> +++ b/Makefile.am
>>>> @@ -168,7 +168,15 @@ if ENABLE_CPP_TEST
>>>>  built_test_programs += cpp-compile-test
>>>>  endif
>>>>  
>>>> -TESTS = $(built_test_programs)
>>>> +AM_TESTS_ENVIRONMENT =							\
>>>> +	export WAYLAND_SCANNER='$(top_builddir)/wayland-scanner'	\
>>>> +	TEST_DATA_DIR='$(top_srcdir)/tests/data'			\
>>>> +	TEST_OUTPUT_DIR='$(top_builddir)/tests/output'			\
>>>> +	SED=$(SED)							\
>>>> +	;
>>>> +
>>>> +TESTS = $(built_test_programs)			\
>>>> +	tests/scanner-test.sh
>>>>  
>>>>  check_PROGRAMS =				\
>>>>  	$(built_test_programs)			\
>>>> @@ -245,4 +253,28 @@ os_wrappers_test_LDADD = libtest-runner.la
>>>>  
>>>>  exec_fd_leak_checker_SOURCES = tests/exec-fd-leak-checker.c
>>>>  exec_fd_leak_checker_LDADD = libtest-runner.la
>>>> +
>>>> +scanner_test_data_files =				\
>>>> +	$(top_srcdir)/tests/data/example.xml		\
>>>> +	$(top_srcdir)/tests/data/example-client.h	\
>>>> +	$(top_srcdir)/tests/data/example-server.h	\
>>>> +	$(top_srcdir)/tests/data/example-code.c		\
>>>> +	$(top_srcdir)/tests/data/small.xml		\
>>>> +	$(top_srcdir)/tests/data/small-code.c		\
>>>> +	$(top_srcdir)/tests/data/small-client.h		\
>>>> +	$(top_srcdir)/tests/data/small-server.h		\
>>>> +	$(top_srcdir)/tests/data/small-code-core.c	\
>>>> +	$(top_srcdir)/tests/data/small-client-core.h	\
>>>> +	$(top_srcdir)/tests/data/small-server-core.h
>>>> +
>>>> +tests/scanner-test.sh:					\
>>>> +	$(top_builddir)/wayland-scanner			\
>>>> +	$(scanner_test_data_files)    
>>>
>>> Dependencies should be in the same line. Otherwise it's easy to confuse them for
>>> rules.  
>>
>> Hmm, yeah, a different indentation scheme from rules would be in place indeed.
>>
>> I changed it to this:
>>
>> tests/scanner-test.sh: $(top_builddir)/wayland-scanner	\
>> 		       $(scanner_test_data_files)
> 
> Wait... why did I put scanner_test_data_files as scanner-test.sh
> dependencies...
> 
> scanner-test.sh is a script, it's not built from these. The data files
> are not built either. The script runs unconditionally on 'make check'.
> So, no need to have the data files as deps, AFAIU.

Indeed.

> However, the dependency to wayland-scanner is needed, because the
> script wants to run it, and it gets built.
> 
> It should be enough to put the data files just in EXTRA_DIST and
> without $(top_srcdir).
> 
> Right?

Yeah, that sounds right.

Cheers,
Emilio