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

[Lukáš Hrázký]

On Thu, 2018-02-01 at 06:24 -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ý <lhrazky at redhat.com>
> > ---
> >  configure.ac                          |  3 +++
> >  src/mjpeg-fallback.cpp                |  5 ++++
> >  src/mjpeg-fallback.hpp                |  1 +
> >  src/unittests/Makefile.am             | 12 +++++++++
> >  src/unittests/test-mjpeg-fallback.cpp | 50
> >  +++++++++++++++++++++++++++++++++++
> >  5 files changed, 71 insertions(+)
> >  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
> >  
> 
> I like the idea that tests have to succeed but I'm thinking about
> RHEL7
> (which we need to support) and how to manage it taking into account
> that
> catch is currently not packaged there.
> 
> Is it currently in epel 7 packages but I don't know if is possible to
> use epel 7 in our case.
> 
> Maybe instead of the error we could have it disabled?
> 
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index 7aaa355..d6dcf74 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -186,6 +186,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 0e9ed6a..65c5fd3 100644
> > --- a/src/mjpeg-fallback.hpp
> > +++ b/src/mjpeg-fallback.hpp
> > @@ -24,6 +24,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;
> >      static bool Register(Agent* agent);
> >  private:
> 
> What do you mean with the TODO comment?

Sorry, forgot to reply to this. I mean we call it Options when you
parse it but Settings when you store it. Is it intentional? Should the
'getter' I added here be called Settings()?

> > diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> > index ef6c253..be25a0e 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 = \
> >  	test-hexdump \
> > +	test-mjpeg-fallback \
> >  	$(NULL)
> >  
> >  TESTS = \
> >  	hexdump.sh \
> > +	test-mjpeg-fallback \
> >  	$(NULL)
> >  
> >  noinst_PROGRAMS = \
> > @@ -32,6 +35,15 @@ test_hexdump_LDADD = \
> >  	../libstreaming-utils.a \
> >  	$(NULL)
> >  
> > +test_mjpeg_fallback_SOURCES = \
> > +	test-mjpeg-fallback.cpp \
> > +	../mjpeg-fallback.cpp \
> > +	../jpeg.cpp
> > +
> > +test_mjpeg_fallback_LDADD = \
> > +	$(X11_LIBS) \
> > +	$(JPEG_LIBS)
> > +
> >  EXTRA_DIST = \
> >  	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 0000000..08ed406
> > --- /dev/null
> > +++ b/src/unittests/test-mjpeg-fallback.cpp
> > @@ -0,0 +1,50 @@
> > +#define CATCH_CONFIG_MAIN
> > +#include "catch/catch.hpp"
> > +
> > +#include "mjpeg-fallback.hpp"
> > +
> > +namespace ssa = SpiceStreamingAgent;
> > +
> > +
> > +TEST_CASE("test parse options", "[mjpeg][options]")
> > +{
> > +    ssa::MjpegPlugin plugin;
> > +
> > +    SECTION("correct setup") {
> > +        std::vector<ssa::ConfigureOption> options = {
> > +            {"framerate", "20"},
> > +            {"mjpeg.quality", "90"},
> > +            {NULL, NULL}
> > +        };
> > +
> > +        plugin.ParseOptions(options.data());
> > +        ssa::MjpegSettings new_options = plugin.Options();
> > +        CHECK(new_options.fps == 20);
> > +        CHECK(new_options.quality == 90);
> > +    }
> > +
> > +    SECTION("unknown option") {
> > +        std::vector<ssa::ConfigureOption> options = {
> > +            {"wakaka", "10"},
> > +            {NULL, NULL}
> > +        };
> > +
> > +        REQUIRE_THROWS_WITH(
> > +            plugin.ParseOptions(options.data()),
> > +            "Invalid option 'wakaka'."
> > +        );
> > +    }
> > +
> > +    SECTION("invalid option value") {
> > +        std::vector<ssa::ConfigureOption> options = {
> > +            {"framerate", "10"},
> > +            {"mjpeg.quality", "toot"},
> > +            {NULL, NULL}
> > +        };
> > +
> > +        REQUIRE_THROWS_WITH(
> > +            plugin.ParseOptions(options.data()),
> > +            "Invalid value 'toot' for option 'mjpeg.quality'."
> > +        );
> > +    }
> > +}
> 
> Frediano