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

Tue Feb 20 09:55:27 UTC 2018

On Tue, 2018-02-20 at 10:31 +0100, Christophe de Dinechin wrote:
> > 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?

This is with regards to Gentoo in particular. It means that, if the
packager doesn't set the --with-catch flag you described in the ebuild,
and a user installs the package on his system, one of two scenarios
will happen (suppose Catch was not added to build deps - or it was, but
for whatever reason it wasn't found by configure):

1) the user has Catch installed, the tests will be run after
compilation

2) the user doesn't have Catch installed, the tests will not be run
after compilation

This is not particularly good in itself. If the packager was really
sloppy, he only tested one of the scenarios, depending on whether he
had Catch installed himself or not. So the other scenario is untested
and can potentially be broken.

This is kind of contrived, but I guess what I'm trying to express is
that adding variance to the build result depending on a system state
adds unnecessary potential for problems.

Cheers,
Lukas

> > 
> > 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
> 
> 