Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
> > Hi, > > On Tue, Feb 20, 2018 at 10:00:41AM +0100, Lukáš Hrázký 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ý > > > > > > > > > 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ý > > > > > > > > > --- > > > > > > > > > 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. 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 > > packag
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Tue, Feb 20, 2018 at 12:22:52PM +0100, Lukáš Hrázký wrote: > On Tue, 2018-02-20 at 11:59 +0100, Victor Toso wrote: > > On Tue, Feb 20, 2018 at 11:45:33AM +0100, Lukáš Hrázký wrote: > > > > That tests are not enabled by default. If we enable it by default > > > > here I would expect to do the same for other Spice components. > > > > > > Ok, but I'm somewhat confused here. Enabling the tests is during > > > packaging - you run `make check`. Which means we are talking about... > > > Fedora packaging? Or am I getting something wrong? > > > > I'm talking about releases and the configure script with default > > options. Today we don't have tests enabled by default in our > > tarballs. > > > > 1) wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2 > > 2) tar -xvf spice-gtk-LATEST.tar.bz2 > > 3) cd spice-gtk-0.34 > > 4) ./configure > > 5) make > > # no tests, no extra dependencies. > > Thanks for the example, made it clear. I think you are mistaken here, > though: > > wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2 > tar -xvf spice-gtk-LATEST.tar.bz2 > cd spice-gtk-0.34 > ./configure > make check > > (the only difference being calling `make check` at the end) > > Here you have the tests. For spice-common (submodule) not spice-gtk. For spice-gtk, you need --enable-static and then, yes, make check will run tests for it. > the tests. Many packaging tools automatically detect there is a make > check target and call it at the appropriate point during the packaging. > > So you don't need to explicitly enable the tests in the tarball in any > way. The only issue, which we are discussing here, are extra testing > dependencies, that the tests may have and that you check for in > configure. > > So if someone builds the package and does not intend to run the tests, > the configure script may still require him to install the tests > dependencies. > > And as I break it down here for myself as well, I think I was wrong > with my previous argumentation for the different behaviour based on > presence of Catch in the system. Because the make check target will be > there anyway, it will just be broken if the dependency isn't there. > Correct? So in the unlikely case I'm not missing anything anymore, I > suppose Christophe's suggestion is ok. Sorry for the noise :) Yes, warning is fine by me too. signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Tue, 2018-02-20 at 11:59 +0100, Victor Toso wrote: > On Tue, Feb 20, 2018 at 11:45:33AM +0100, Lukáš Hrázký wrote: > > > That tests are not enabled by default. If we enable it by default > > > here I would expect to do the same for other Spice components. > > > > Ok, but I'm somewhat confused here. Enabling the tests is during > > packaging - you run `make check`. Which means we are talking about... > > Fedora packaging? Or am I getting something wrong? > > I'm talking about releases and the configure script with default > options. Today we don't have tests enabled by default in our > tarballs. > > 1) wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2 > 2) tar -xvf spice-gtk-LATEST.tar.bz2 > 3) cd spice-gtk-0.34 > 4) ./configure > 5) make > # no tests, no extra dependencies. Thanks for the example, made it clear. I think you are mistaken here, though: wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2 tar -xvf spice-gtk-LATEST.tar.bz2 cd spice-gtk-0.34 ./configure make check (the only difference being calling `make check` at the end) Here you have the tests. It is up to the packaging to explicitly call the tests. Many packaging tools automatically detect there is a make check target and call it at the appropriate point during the packaging. So you don't need to explicitly enable the tests in the tarball in any way. The only issue, which we are discussing here, are extra testing dependencies, that the tests may have and that you check for in configure. So if someone builds the package and does not intend to run the tests, the configure script may still require him to install the tests dependencies. And as I break it down here for myself as well, I think I was wrong with my previous argumentation for the different behaviour based on presence of Catch in the system. Because the make check target will be there anyway, it will just be broken if the dependency isn't there. Correct? So in the unlikely case I'm not missing anything anymore, I suppose Christophe's suggestion is ok. Sorry for the noise :) Lukas ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Tue, Feb 20, 2018 at 11:45:33AM +0100, Lukáš Hrázký wrote: > > That tests are not enabled by default. If we enable it by default > > here I would expect to do the same for other Spice components. > > Ok, but I'm somewhat confused here. Enabling the tests is during > packaging - you run `make check`. Which means we are talking about... > Fedora packaging? Or am I getting something wrong? I'm talking about releases and the configure script with default options. Today we don't have tests enabled by default in our tarballs. 1) wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2 2) tar -xvf spice-gtk-LATEST.tar.bz2 3) cd spice-gtk-0.34 4) ./configure 5) make # no tests, no extra dependencies. signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Tue, 2018-02-20 at 11:34 +0100, Victor Toso wrote: > On Tue, Feb 20, 2018 at 11:21:47AM +0100, Lukáš Hrázký wrote: > > On Tue, 2018-02-20 at 11:02 +0100, Victor Toso wrote: > > > IMHO, tests are a must for development and should be optional > > > on tarballs from releases. That means that packagers can > > > enable it and deal with whatever we do for testing if they > > > want to. > > > > Thing is, there shouldn't be much (if really anything) you have > > to deal with for unittests... It's certainly the case atm. > > For us. If we request Catch and Catch is not there for DISTROX, > they have to disable it in order to have build working or install > Catch. IMHO, for released tarballs one should enable testing if > they want to, not disable if they must. Let's agree we disagree on this one :) > > > More importantly is that we have tests running successfully on > > > some Linux distros (IMO, Fedora and Debian should be enough) and > > > that's why gitlab-ci is there for. > > > > You have different versions of dependencies on different > > distros, so Fedora and Debian are not entirely enough. And if > > you do it properly, having the benefit of the tests being run > > on each distro it's packaged for doesn't cost much... (I'd say > > the benefit is more for the distros, we are basically helping > > them by forcing the tests on them :D) > > I say Fedora and Debian due .rpm and .deb which several distros > might follow so we benefit on testing those two... but if we > care, then we should enable some unit test in the CI. Well, that's what CI is for, so we definitely should... > > > Also IMO, if we change the behavior of unit tests here, we > > > should do for all Spice components in order to be consistent. > > > > You mean the fact that we run them during packaging? > > That tests are not enabled by default. If we enable it by default > here I would expect to do the same for other Spice components. Ok, but I'm somewhat confused here. Enabling the tests is during packaging - you run `make check`. Which means we are talking about... Fedora packaging? Or am I getting something wrong? Cheers, Lukas ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Tue, Feb 20, 2018 at 11:21:47AM +0100, Lukáš Hrázký wrote: > On Tue, 2018-02-20 at 11:02 +0100, Victor Toso wrote: > > IMHO, tests are a must for development and should be optional > > on tarballs from releases. That means that packagers can > > enable it and deal with whatever we do for testing if they > > want to. > > Thing is, there shouldn't be much (if really anything) you have > to deal with for unittests... It's certainly the case atm. For us. If we request Catch and Catch is not there for DISTROX, they have to disable it in order to have build working or install Catch. IMHO, for released tarballs one should enable testing if they want to, not disable if they must. > > More importantly is that we have tests running successfully on > > some Linux distros (IMO, Fedora and Debian should be enough) and > > that's why gitlab-ci is there for. > > You have different versions of dependencies on different > distros, so Fedora and Debian are not entirely enough. And if > you do it properly, having the benefit of the tests being run > on each distro it's packaged for doesn't cost much... (I'd say > the benefit is more for the distros, we are basically helping > them by forcing the tests on them :D) I say Fedora and Debian due .rpm and .deb which several distros might follow so we benefit on testing those two... but if we care, then we should enable some unit test in the CI. > > Also IMO, if we change the behavior of unit tests here, we > > should do for all Spice components in order to be consistent. > > You mean the fact that we run them during packaging? That tests are not enabled by default. If we enable it by default here I would expect to do the same for other Spice components. Cheers, toso signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Tue, 2018-02-20 at 11:02 +0100, Victor Toso wrote: > Hi, > > On Tue, Feb 20, 2018 at 10:00:41AM +0100, Lukáš Hrázký 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ý > > > > > > > > > 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ý > > > > > > > > > --- > > > > > > > > > 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. 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 compil
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
Hi, On Tue, Feb 20, 2018 at 10:00:41AM +0100, Lukáš Hrázký 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ý > > > > > > > > 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ý > > > > > > > > --- > > > > > > > > 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. 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. > > 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 t
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Tue, 2018-02-20 at 10:31 +0100, Christophe de Dinechin wrote: > > On 20 Feb 2018, at 10:00, Lukáš Hrázký 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ý > > > > > > > > > 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ý > > > > > > > > > --- > > > > > > > > > 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
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Mon, Feb 19, 2018 at 09:19:18PM +0200, Uri Lublin wrote: > 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. NB: My understanding of https://blogs.gnome.org/desrt/2011/09/08/am_maintainer_mode-is-not-cool/ is that nobody should need to use --enable-maintainer-mode. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
> On 20 Feb 2018, at 10:00, Lukáš Hrázký 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ý 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ý --- 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
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
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ý 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ý > > > > > > > --- > > > > > > > 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. 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. 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 s
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
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ý 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ý --- 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. 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. Uri. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
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ý 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ý > > > > > --- > > > > > 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. 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. Lukas ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
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ý 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ý --- 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. Uri. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote: > > > > > On 14 Feb 2018, at 13:34, Lukáš Hrázký 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ý > > > --- > > > 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 signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
> > > On 14 Feb 2018, at 13:34, Lukáš Hrázký 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ý > > --- > > 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. Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Wed, 2018-02-14 at 16:08 +0100, Christophe de Dinechin wrote:
> > On 14 Feb 2018, at 13:34, Lukáš Hrázký 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ý
> > ---
> > 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?
Unless this is a real obstacle for someone, I'd rather just force the
unittests on people, otherwise, nobody (except for us I suppose) will
care for the warning if everything still works...
> > +
> > dnl
> > ===
> > dnl check compiler flags
> >
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index 1b51ee0..ae1777d 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -181,6 +181,11 @@ void MjpegPlugin::ParseOptions(const ConfigureOption
> > *options)
> > }
> > }
> >
> > +MjpegSettings MjpegPlugin::Options() const
> > +{
> > +return settings;
> > +}
> > +
> > SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
> > return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > }
> > diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> > index 04fa2eb..8496763 100644
> > --- a/src/mjpeg-fallback.hpp
> > +++ b/src/mjpeg-fallback.hpp
> > @@ -25,6 +25,7 @@ public:
> > FrameCapture *CreateCapture() override;
> > unsigned Rank() override;
> > void ParseOptions(const ConfigureOption *options);
> > +MjpegSettings Options() const; // TODO unify on Settings vs Options
> > SpiceVideoCodecType VideoCodecType() const;
> > private:
> > MjpegSettings settings = { 10, 80 };
> > diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
> > index af41c48..22f1335 100644
> > --- a/src/unittests/.gitignore
> > +++ b/src/unittests/.gitignore
> > @@ -1,4 +1,5 @@
> > /hexdump
> > -/test-hexdump.sh.log
> > -/test-hexdump.sh.trs
> > +/test-*.log
> > +/test-*.trs
> > +/test-mjpeg-fallback
> > /test-suite.log
> > diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> > index a70a4b4..bd079c3 100644
> > --- a/src/unittests/Makefile.am
> > +++ b/src/unittests/Makefile.am
> > @@ -2,6 +2,7 @@ NULL =
> >
> > AM_CPPFLAGS = \
> > -DRH_TOP_SRCDIR=\"$(abs_top_srcdir)\" \
> > + -I$(top_srcdir)/include \
> > -I$(top_srcdir)/src \
> > -I$(top_srcdir)/src/unittests \
> > $(SPICE_PROTOCOL_CFLAGS) \
> > @@ -14,10 +15,12 @@ AM_CFLAGS = \
> >
> > check_PROGRAMS = \
> > hexdump \
> > + test-mjpeg-fallback \
> > $(NULL)
> >
> > TESTS = \
> > test-hexdump.sh \
> > + test-mjpeg-fallback \
> > $(NULL)
> >
> > noinst_PROGRAMS = \
> > @@ -32,6 +35,18 @@ hexdump_LDADD = \
> > ../libstreaming-utils.a \
> > $(NULL)
> >
> > +test_mjpeg_fallback_SOURCES = \
> > + test-mjpeg-fallback.cpp \
> > + ../jpeg.cpp \
> > + ../mjpeg-fallback.cpp \
> > + ../static-plugin.cpp \
> > + $(NULL)
> > +
> > +test_mjpeg_fallback_LDADD = \
> > + $(X11_LIBS) \
> > + $(JPEG_LIBS) \
> > + $(NULL)
> > +
> > EXTRA_DIST = \
> > test-hexdump.sh \
> > hexdump1.in \
> > diff --git a/src/unittests/test-mjpeg-fallback.cpp
> > b/src/unittests/test-mjpeg-fallback.cpp
> > new file mode 100644
> > index 000..4a152fe
> > --- /dev/null
> > +++ b/src/unittests/test-mjpeg-fallback.cpp
> > @@ -0,0 +1,58 @@
> > +#define CATCH_CONFIG_MAIN
> > +#include "catch/catch.hpp"
> > +
> > +#include "mjpeg-fallback.hpp"
> > +
> > +namespace ssa = spice::streaming_agent;
> > +
> > +
> > +SCENARIO("test parsing mjpeg plugin options", "[mjpeg][options]") {
> > +GIVEN("A new mjpeg plugin") {
> > +ssa::MjpegPlugin plugin;
> > +
> > +WHEN("passing correct options") {
> > +std::vector options = {
> > +{"framerate", "20"},
> > +{"mjpeg.quality", "90
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
On Wed, 2018-02-14 at 09:22 -0500, Frediano Ziglio 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ý
> > ---
> > 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)])])
> > +
> > dnl
> > ===
> > dnl check compiler flags
> >
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index 1b51ee0..ae1777d 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -181,6 +181,11 @@ void MjpegPlugin::ParseOptions(const ConfigureOption
> > *options)
> > }
> > }
> >
> > +MjpegSettings MjpegPlugin::Options() const
> > +{
> > +return settings;
> > +}
> > +
> > SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
> > return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > }
> > diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> > index 04fa2eb..8496763 100644
> > --- a/src/mjpeg-fallback.hpp
> > +++ b/src/mjpeg-fallback.hpp
> > @@ -25,6 +25,7 @@ public:
> > FrameCapture *CreateCapture() override;
> > unsigned Rank() override;
> > void ParseOptions(const ConfigureOption *options);
> > +MjpegSettings Options() const; // TODO unify on Settings vs Options
> > SpiceVideoCodecType VideoCodecType() const;
> > private:
> > MjpegSettings settings = { 10, 80 };
> > diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
> > index af41c48..22f1335 100644
> > --- a/src/unittests/.gitignore
> > +++ b/src/unittests/.gitignore
> > @@ -1,4 +1,5 @@
> > /hexdump
> > -/test-hexdump.sh.log
> > -/test-hexdump.sh.trs
> > +/test-*.log
> > +/test-*.trs
> > +/test-mjpeg-fallback
> > /test-suite.log
> > diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> > index a70a4b4..bd079c3 100644
> > --- a/src/unittests/Makefile.am
> > +++ b/src/unittests/Makefile.am
> > @@ -2,6 +2,7 @@ NULL =
> >
> > AM_CPPFLAGS = \
> > -DRH_TOP_SRCDIR=\"$(abs_top_srcdir)\" \
> > + -I$(top_srcdir)/include \
> > -I$(top_srcdir)/src \
> > -I$(top_srcdir)/src/unittests \
> > $(SPICE_PROTOCOL_CFLAGS) \
> > @@ -14,10 +15,12 @@ AM_CFLAGS = \
> >
> > check_PROGRAMS = \
> > hexdump \
> > + test-mjpeg-fallback \
> > $(NULL)
> >
> > TESTS = \
> > test-hexdump.sh \
> > + test-mjpeg-fallback \
> > $(NULL)
> >
> > noinst_PROGRAMS = \
> > @@ -32,6 +35,18 @@ hexdump_LDADD = \
> > ../libstreaming-utils.a \
> > $(NULL)
> >
> > +test_mjpeg_fallback_SOURCES = \
> > + test-mjpeg-fallback.cpp \
> > + ../jpeg.cpp \
> > + ../mjpeg-fallback.cpp \
> > + ../static-plugin.cpp \
> > + $(NULL)
> > +
> > +test_mjpeg_fallback_LDADD = \
> > + $(X11_LIBS) \
> > + $(JPEG_LIBS) \
> > + $(NULL)
> > +
> > EXTRA_DIST = \
> > test-hexdump.sh \
> > hexdump1.in \
> > diff --git a/src/unittests/test-mjpeg-fallback.cpp
> > b/src/unittests/test-mjpeg-fallback.cpp
> > new file mode 100644
> > index 000..4a152fe
> > --- /dev/null
> > +++ b/src/unittests/test-mjpeg-fallback.cpp
> > @@ -0,0 +1,58 @@
>
> No config.h include ?
>
> There should be an header with copyright (forget to note before).
I missed that. Will add it.
> > +#define CATCH_CONFIG_MAIN
> > +#include "catch/catch.hpp"
> > +
> > +#include "mjpeg-fallback.hpp"
> > +
> > +namespace ssa = spice::streaming_agent;
> > +
> > +
> > +SCENARIO("test parsing mjpeg plugin options", "[mjpeg][options]") {
> > +GIVEN("A new mjpeg plugin") {
> > +ssa::MjpegPlugin plugin;
> > +
> > +WHEN("passing correct options") {
> > +std::vector options = {
> > +{"framerate", "20"},
> > +{"mjpeg.quality", "90"},
> > +{NULL, NULL}
> > +};
> > +
> > +plugin.ParseOptions(options.data());
> > +ssa::MjpegSettings new_options = plugin.Options();
> > +
>
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
> On 14 Feb 2018, at 13:34, Lukáš Hrázký 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ý
> ---
> 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?
> +
> dnl
> ===
> dnl check compiler flags
>
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 1b51ee0..ae1777d 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -181,6 +181,11 @@ void MjpegPlugin::ParseOptions(const ConfigureOption
> *options)
> }
> }
>
> +MjpegSettings MjpegPlugin::Options() const
> +{
> +return settings;
> +}
> +
> SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
> return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> }
> diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> index 04fa2eb..8496763 100644
> --- a/src/mjpeg-fallback.hpp
> +++ b/src/mjpeg-fallback.hpp
> @@ -25,6 +25,7 @@ public:
> FrameCapture *CreateCapture() override;
> unsigned Rank() override;
> void ParseOptions(const ConfigureOption *options);
> +MjpegSettings Options() const; // TODO unify on Settings vs Options
> SpiceVideoCodecType VideoCodecType() const;
> private:
> MjpegSettings settings = { 10, 80 };
> diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
> index af41c48..22f1335 100644
> --- a/src/unittests/.gitignore
> +++ b/src/unittests/.gitignore
> @@ -1,4 +1,5 @@
> /hexdump
> -/test-hexdump.sh.log
> -/test-hexdump.sh.trs
> +/test-*.log
> +/test-*.trs
> +/test-mjpeg-fallback
> /test-suite.log
> diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> index a70a4b4..bd079c3 100644
> --- a/src/unittests/Makefile.am
> +++ b/src/unittests/Makefile.am
> @@ -2,6 +2,7 @@ NULL =
>
> AM_CPPFLAGS = \
> -DRH_TOP_SRCDIR=\"$(abs_top_srcdir)\" \
> + -I$(top_srcdir)/include \
> -I$(top_srcdir)/src \
> -I$(top_srcdir)/src/unittests \
> $(SPICE_PROTOCOL_CFLAGS) \
> @@ -14,10 +15,12 @@ AM_CFLAGS = \
>
> check_PROGRAMS = \
> hexdump \
> + test-mjpeg-fallback \
> $(NULL)
>
> TESTS = \
> test-hexdump.sh \
> + test-mjpeg-fallback \
> $(NULL)
>
> noinst_PROGRAMS = \
> @@ -32,6 +35,18 @@ hexdump_LDADD = \
> ../libstreaming-utils.a \
> $(NULL)
>
> +test_mjpeg_fallback_SOURCES = \
> + test-mjpeg-fallback.cpp \
> + ../jpeg.cpp \
> + ../mjpeg-fallback.cpp \
> + ../static-plugin.cpp \
> + $(NULL)
> +
> +test_mjpeg_fallback_LDADD = \
> + $(X11_LIBS) \
> + $(JPEG_LIBS) \
> + $(NULL)
> +
> EXTRA_DIST = \
> test-hexdump.sh \
> hexdump1.in \
> diff --git a/src/unittests/test-mjpeg-fallback.cpp
> b/src/unittests/test-mjpeg-fallback.cpp
> new file mode 100644
> index 000..4a152fe
> --- /dev/null
> +++ b/src/unittests/test-mjpeg-fallback.cpp
> @@ -0,0 +1,58 @@
> +#define CATCH_CONFIG_MAIN
> +#include "catch/catch.hpp"
> +
> +#include "mjpeg-fallback.hpp"
> +
> +namespace ssa = spice::streaming_agent;
> +
> +
> +SCENARIO("test parsing mjpeg plugin options", "[mjpeg][options]") {
> +GIVEN("A new mjpeg plugin") {
> +ssa::MjpegPlugin plugin;
> +
> +WHEN("passing correct options") {
> +std::vector options = {
> +{"framerate", "20"},
> +{"mjpeg.quality", "90"},
> +{NULL, NULL}
> +};
> +
> +plugin.ParseOptions(options.data());
> +ssa::MjpegSettings new_options = plugin.Options();
> +
> +THEN("the options are set in the plugin") {
> +CHECK(new_options.fps == 20);
> +CHECK(new_options.quality == 90);
> +}
> +}
> +
> +WHEN("passing an unknown option") {
> +std::vector options = {
> +{"wakaka", "10"},
> +
Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
>
> 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ý
> ---
> 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)])])
> +
> dnl
> ===
> dnl check compiler flags
>
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 1b51ee0..ae1777d 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -181,6 +181,11 @@ void MjpegPlugin::ParseOptions(const ConfigureOption
> *options)
> }
> }
>
> +MjpegSettings MjpegPlugin::Options() const
> +{
> +return settings;
> +}
> +
> SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
> return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> }
> diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> index 04fa2eb..8496763 100644
> --- a/src/mjpeg-fallback.hpp
> +++ b/src/mjpeg-fallback.hpp
> @@ -25,6 +25,7 @@ public:
> FrameCapture *CreateCapture() override;
> unsigned Rank() override;
> void ParseOptions(const ConfigureOption *options);
> +MjpegSettings Options() const; // TODO unify on Settings vs Options
> SpiceVideoCodecType VideoCodecType() const;
> private:
> MjpegSettings settings = { 10, 80 };
> diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
> index af41c48..22f1335 100644
> --- a/src/unittests/.gitignore
> +++ b/src/unittests/.gitignore
> @@ -1,4 +1,5 @@
> /hexdump
> -/test-hexdump.sh.log
> -/test-hexdump.sh.trs
> +/test-*.log
> +/test-*.trs
> +/test-mjpeg-fallback
> /test-suite.log
> diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> index a70a4b4..bd079c3 100644
> --- a/src/unittests/Makefile.am
> +++ b/src/unittests/Makefile.am
> @@ -2,6 +2,7 @@ NULL =
>
> AM_CPPFLAGS = \
> -DRH_TOP_SRCDIR=\"$(abs_top_srcdir)\" \
> + -I$(top_srcdir)/include \
> -I$(top_srcdir)/src \
> -I$(top_srcdir)/src/unittests \
> $(SPICE_PROTOCOL_CFLAGS) \
> @@ -14,10 +15,12 @@ AM_CFLAGS = \
>
> check_PROGRAMS = \
> hexdump \
> + test-mjpeg-fallback \
> $(NULL)
>
> TESTS = \
> test-hexdump.sh \
> + test-mjpeg-fallback \
> $(NULL)
>
> noinst_PROGRAMS = \
> @@ -32,6 +35,18 @@ hexdump_LDADD = \
> ../libstreaming-utils.a \
> $(NULL)
>
> +test_mjpeg_fallback_SOURCES = \
> + test-mjpeg-fallback.cpp \
> + ../jpeg.cpp \
> + ../mjpeg-fallback.cpp \
> + ../static-plugin.cpp \
> + $(NULL)
> +
> +test_mjpeg_fallback_LDADD = \
> + $(X11_LIBS) \
> + $(JPEG_LIBS) \
> + $(NULL)
> +
> EXTRA_DIST = \
> test-hexdump.sh \
> hexdump1.in \
> diff --git a/src/unittests/test-mjpeg-fallback.cpp
> b/src/unittests/test-mjpeg-fallback.cpp
> new file mode 100644
> index 000..4a152fe
> --- /dev/null
> +++ b/src/unittests/test-mjpeg-fallback.cpp
> @@ -0,0 +1,58 @@
No config.h include ?
There should be an header with copyright (forget to note before).
> +#define CATCH_CONFIG_MAIN
> +#include "catch/catch.hpp"
> +
> +#include "mjpeg-fallback.hpp"
> +
> +namespace ssa = spice::streaming_agent;
> +
> +
> +SCENARIO("test parsing mjpeg plugin options", "[mjpeg][options]") {
> +GIVEN("A new mjpeg plugin") {
> +ssa::MjpegPlugin plugin;
> +
> +WHEN("passing correct options") {
> +std::vector options = {
> +{"framerate", "20"},
> +{"mjpeg.quality", "90"},
> +{NULL, NULL}
> +};
> +
> +plugin.ParseOptions(options.data());
> +ssa::MjpegSettings new_options = plugin.Options();
> +
> +THEN("the options are set in the plugin") {
> +CHECK(new_options.fps == 20);
> +CHECK(new_options.quality == 90);
> +}
> +}
> +
> +WHEN("passing an unknown option") {
> +std::vector options = {
> +{"wakaka", "10"},
> +{NULL, NULL}
> +
[Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing
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ý
---
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)])])
+
dnl ===
dnl check compiler flags
diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 1b51ee0..ae1777d 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -181,6 +181,11 @@ void MjpegPlugin::ParseOptions(const ConfigureOption
*options)
}
}
+MjpegSettings MjpegPlugin::Options() const
+{
+return settings;
+}
+
SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
return SPICE_VIDEO_CODEC_TYPE_MJPEG;
}
diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
index 04fa2eb..8496763 100644
--- a/src/mjpeg-fallback.hpp
+++ b/src/mjpeg-fallback.hpp
@@ -25,6 +25,7 @@ public:
FrameCapture *CreateCapture() override;
unsigned Rank() override;
void ParseOptions(const ConfigureOption *options);
+MjpegSettings Options() const; // TODO unify on Settings vs Options
SpiceVideoCodecType VideoCodecType() const;
private:
MjpegSettings settings = { 10, 80 };
diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
index af41c48..22f1335 100644
--- a/src/unittests/.gitignore
+++ b/src/unittests/.gitignore
@@ -1,4 +1,5 @@
/hexdump
-/test-hexdump.sh.log
-/test-hexdump.sh.trs
+/test-*.log
+/test-*.trs
+/test-mjpeg-fallback
/test-suite.log
diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
index a70a4b4..bd079c3 100644
--- a/src/unittests/Makefile.am
+++ b/src/unittests/Makefile.am
@@ -2,6 +2,7 @@ NULL =
AM_CPPFLAGS = \
-DRH_TOP_SRCDIR=\"$(abs_top_srcdir)\" \
+ -I$(top_srcdir)/include \
-I$(top_srcdir)/src \
-I$(top_srcdir)/src/unittests \
$(SPICE_PROTOCOL_CFLAGS) \
@@ -14,10 +15,12 @@ AM_CFLAGS = \
check_PROGRAMS = \
hexdump \
+ test-mjpeg-fallback \
$(NULL)
TESTS = \
test-hexdump.sh \
+ test-mjpeg-fallback \
$(NULL)
noinst_PROGRAMS = \
@@ -32,6 +35,18 @@ hexdump_LDADD = \
../libstreaming-utils.a \
$(NULL)
+test_mjpeg_fallback_SOURCES = \
+ test-mjpeg-fallback.cpp \
+ ../jpeg.cpp \
+ ../mjpeg-fallback.cpp \
+ ../static-plugin.cpp \
+ $(NULL)
+
+test_mjpeg_fallback_LDADD = \
+ $(X11_LIBS) \
+ $(JPEG_LIBS) \
+ $(NULL)
+
EXTRA_DIST = \
test-hexdump.sh \
hexdump1.in \
diff --git a/src/unittests/test-mjpeg-fallback.cpp
b/src/unittests/test-mjpeg-fallback.cpp
new file mode 100644
index 000..4a152fe
--- /dev/null
+++ b/src/unittests/test-mjpeg-fallback.cpp
@@ -0,0 +1,58 @@
+#define CATCH_CONFIG_MAIN
+#include "catch/catch.hpp"
+
+#include "mjpeg-fallback.hpp"
+
+namespace ssa = spice::streaming_agent;
+
+
+SCENARIO("test parsing mjpeg plugin options", "[mjpeg][options]") {
+GIVEN("A new mjpeg plugin") {
+ssa::MjpegPlugin plugin;
+
+WHEN("passing correct options") {
+std::vector options = {
+{"framerate", "20"},
+{"mjpeg.quality", "90"},
+{NULL, NULL}
+};
+
+plugin.ParseOptions(options.data());
+ssa::MjpegSettings new_options = plugin.Options();
+
+THEN("the options are set in the plugin") {
+CHECK(new_options.fps == 20);
+CHECK(new_options.quality == 90);
+}
+}
+
+WHEN("passing an unknown option") {
+std::vector options = {
+{"wakaka", "10"},
+{NULL, NULL}
+};
+
+THEN("ParseOptions throws an exception") {
+REQUIRE_THROWS_WITH(
+plugin.ParseOptions(options.data()),
+"Invalid option 'wakaka'."
+);
+}
+}
+
+WHEN("passing an invalid option value") {
+std::vector options = {
+{"fram
