[Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing

Tue Feb 20 09:31:52 UTC 2018

> On 20 Feb 2018, at 10:00, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> 
> On Mon, 2018-02-19 at 21:19 +0200, Uri Lublin wrote:
>> On 02/19/2018 06:47 PM, Lukáš Hrázký wrote:
>>> On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:
>>>> On 02/14/2018 06:37 PM, Christophe Fergeau wrote:
>>>>> On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
>>>>>>> 
>>>>>>>> On 14 Feb 2018, at 13:34, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>>>>>>>> 
>>>>>>>> Introduce a unit test framework (Catch) to the codebase and a simple
>>>>>>>> unit test for parsing the options of the mjpeg plugin.
>>>>>>>> 
>>>>>>>> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
>>>>>>>> ---
>>>>>>>> configure.ac                          |  3 ++
>>>>>>>> src/mjpeg-fallback.cpp                |  5 +++
>>>>>>>> src/mjpeg-fallback.hpp                |  1 +
>>>>>>>> src/unittests/.gitignore              |  5 +--
>>>>>>>> src/unittests/Makefile.am             | 15 +++++++++
>>>>>>>> src/unittests/test-mjpeg-fallback.cpp | 58
>>>>>>>> +++++++++++++++++++++++++++++++++++
>>>>>>>> 6 files changed, 85 insertions(+), 2 deletions(-)
>>>>>>>> create mode 100644 src/unittests/test-mjpeg-fallback.cpp
>>>>>>>> 
>>>>>>>> diff --git a/configure.ac b/configure.ac
>>>>>>>> index 8795dae..5aab662 100644
>>>>>>>> --- a/configure.ac
>>>>>>>> +++ b/configure.ac
>>>>>>>> @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
>>>>>>>>      AC_MSG_ERROR([C99 compiler is required.])
>>>>>>>> fi
>>>>>>>> AC_PROG_CXX
>>>>>>>> +AC_LANG(C++)
>>>>>>>> AX_CXX_COMPILE_STDCXX_11
>>>>>>>> AC_PROG_INSTALL
>>>>>>>> AC_CANONICAL_HOST
>>>>>>>> @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
>>>>>>>>      AC_MSG_ERROR([libjpeg not found]))
>>>>>>>> AC_SUBST(JPEG_LIBS)
>>>>>>>> 
>>>>>>>> +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find Catch
>>>>>>>> dependency header (catch/catch.hpp)])])
>>>>>>> 
>>>>>>> Instead of an error, shouldn’t we instead fallback to not compiling the unit
>>>>>>> tests? Possibly a warning?
>>>>>>> 
>>>>>> 
>>>>>> Good point but I would suggest a follow up and an explicit --I-dont-really-want-unittests
>>>>>> option, I agree people should run tests.
>>>>>> Another follow up is a patch for the spec file.
>>>>> 
>>>>> Alternatively, this could go with a --with-catch flag (or
>>>>> --enable-unittest or any names which fits you), and
>>>>> 1) if there is nothing specified, then we silently enable/disable tests
>>>>> depending on the availability of the catch
>>>>> 2) if --with-catch is specified, then we error out if it's not found
>>>>> 3) if --without-catch is used, then we never use it.
>>>>> 
>>>>> Then we add --with-catch to autogen.sh, and all developers will have to
>>>>> go through extra hoops not to use it.
>>>>> 
>>>>> Christophe
>>>>> 
>>>> 
>>>> I agree that's a good alternative.
>>> 
>>> I prefer Frediano's variant. Nobody is forced to use autogen.sh and
>>> also I don't think anybody would expect autogen.sh to modify the
>>> default configure behaviour. I don't think it's a good idea.
>> 
>> If users prefer to not run autogen.sh that's ok.
>> It provides defaults options for developers.
>> For example, I do not expect users to run configure with
>>  --enable-maintainer-mode too.
>> Most users will use configure directly from the tarball.
>> 
>> Users can choose what options they want to enable/disable
>> 
>>> 
>>> For example, on Gentoo, which doesn't care for autogen.sh and runs
>>> autotools build automatically, the default behaviour (unless the person
>>> writing the ebuild would notice) would be dependent on Catch being
>>> present in the system. This can create subtle inconsistencies, which
>>> aren't critical in this case, but still unnecessary.
>> 
>> So if on Gentoo (or another distribution) there exists no catch
>> package, they are forced to either add this package or search
>> for how to disable it.
> 
> Yes. The packager is forced to resolve the issue by either providing
> the dependency or explicitly disabling the tests. In contrast to the
> tests being "quietly" skipped, if Catch is not present, unless he would
> notice it.

I’d be more comfortable with a warning at the end of configure stating that we did not find catch, so unit tests can’t be run, like we do for example for a few other optional missing components such as codecs. A warning is also more friendly than a hard failure.

To summarize, like Christophe, I would like to see a —with-catch option, but I would like to see it default to yes, unless we cannot find catch, in which case:
- If the option —with-catch was specified, configure fails
- If the option was set to default, we get a warning, we can build, we can’t test.

(Maybe this is really what Christophe had in mind too, not sure).

> If he isn't dilligent and doesn't notice, the behaviour of
> the package will depend on the presence of Catch on the target system (
> on Gentoo packages are compiled on users' systems) and since the
> packager didn't test one of the variants, it could potentially fail on
> the user.

I would like to understand what you mean with “the behaviour of the package”. Only the ability to run tests, right?

> 
> Packagers at least on Gentoo are used to enabling/disabling things in
> configure. I don't think it's a hurdle for them to disable the tests if
> indeed they don't have the package (and I think most distros have it -
> Gentoo does).
> 
>> This package is only required if the user wants
>> to run the tests.
>> Those tests do not run by default either.
>> One have to "make check" for the tests to run.
> 
> It is desirable to run the tests during packaging. I saw Frediano post
> a patch the other day for I think the Fedora package to run `make
> check` during the packaging? IIRC `make check` is run by default by
> Debian packaging. I am not sure, but would think on Gentoo `make check`
> is also run by default.
> 
> I would just like to turn the tests on for users by default. (assuming
> they are working well) We could make something like "You can use
> './configure --disable-unittests' to skip the unit tests." part of the
> error message if Catch is not found to make it easier for them?
> 
> Lukas