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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]