[Spice-devel] [PATCH spice-streaming-agent 0/4] first unit test and options parsing improvements

[Lukáš Hrázký]

On Thu, 2018-02-01 at 05:42 -0500, Frediano Ziglio wrote:
> > 
> > This series introduces a C++ unit test framework called Catch to
> > the
> > codebase, adds a simple unit test for the options parsing for the
> > mjpeg
> > plugin and improves on the option parsing code.
> > 
> > It depends on the header split-off from the previous patch set
> > ([PATCH
> > spice-streaming-agent 1/2] mjpeg plugin: split off the declaration
> > into
> > a header).
> > 
> > The unit test framework choice is a subject to discussion. Note
> > Catch is
> > quite new and it only has a package in Fedora 27.
> > 
> > 1/4 is there because otherwise I'd have to flip AC_LANG(C++) on and
> > off
> > later on.
> > 
> 
> In theory should work correctly.
> In practise there are some autoconf macros here and there that
> could have some issues, I would use
> 
> AC_LANG_PUSH([C++])
> <your code>
> AC_LANG_POP([C++])

Do you know which ones in particular? Would it not be better to leave
it as simple as possible and only complicate it if we encounter an
issue?

> > The tests in 2/4 do not pass until 4/4, TDD excersize :)
> > 
> 
> Usually we prefer test after the fix patch. This makes git bisect run
> scripts
> easier to code if they include a "make check".
> 
> I usually develop like test first and revert+check again before
> sending
> to ML.

I do not understand your wording here. If you want me to move the test
commits after the fix commit, I can. I could also write the test so
that it doesn't fail before the fix.

> > 3/4 is using BDD-style [1] test structure which I quite like, I
> > wrote
> > the tests in two steps so I'm including the patch for comparison.
> > 
> > Cheers,
> > Lukas
> > 
> > [1] https://en.wikipedia.org/wiki/Behavior-driven_development
> > 
> > Lukáš Hrázký (4):
> >   configure.ac: remove header check for stream-device.h
> >   mjpeg-fallback: unittest for the options parsing
> >   mjpeg-fallback-test: use BDD structure for test cases
> >   mjpeg-fallback: a more C++ way of handling options
> > 
> >  configure.ac                          |  7 ++---
> >  src/mjpeg-fallback.cpp                | 47 +++++++++++++++------
> > -------
> >  src/mjpeg-fallback.hpp                |  1 +
> >  src/unittests/Makefile.am             | 12 ++++++++
> >  src/unittests/test-mjpeg-fallback.cpp | 58
> >  +++++++++++++++++++++++++++++++++++
> >  5 files changed, 100 insertions(+), 25 deletions(-)
> >  create mode 100644 src/unittests/test-mjpeg-fallback.cpp
> > 
> 
> Frediano