[Spice-devel] [PATCH spice-streaming-agent v2 6/9] Add a unit test for the stream port

[Lukáš Hrázký]

On Wed, 2018-05-16 at 12:47 -0400, Frediano Ziglio wrote:
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> >  src/unittests/.gitignore           |  1 +
> >  src/unittests/Makefile.am          |  9 ++++-
> >  src/unittests/test-stream-port.cpp | 69
> >  ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 78 insertions(+), 1 deletion(-)
> >  create mode 100644 src/unittests/test-stream-port.cpp
> > 
> > diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
> > index 22f1335..ef9e31b 100644
> > --- a/src/unittests/.gitignore
> > +++ b/src/unittests/.gitignore
> > @@ -2,4 +2,5 @@
> >  /test-*.log
> >  /test-*.trs
> >  /test-mjpeg-fallback
> > +/test-stream-port
> >  /test-suite.log
> > diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> > index 0fc6d50..67f9c42 100644
> > --- a/src/unittests/Makefile.am
> > +++ b/src/unittests/Makefile.am
> > @@ -16,11 +16,12 @@ AM_CFLAGS = \
> >  check_PROGRAMS = \
> >  	hexdump \
> >  	test-mjpeg-fallback \
> > +	test-stream-port \
> >  	$(NULL)
> >  
> >  TESTS = \
> >  	test-hexdump.sh \
> > -	test-mjpeg-fallback \
> > +	test-stream-port \
> >  	$(NULL)
> >  
> 
> Why did you remove test-mjpeg-fallback ?

By accident! I'll put it back, don't know how that happened.

> >  noinst_PROGRAMS = \
> > @@ -46,6 +47,12 @@ test_mjpeg_fallback_LDADD = \
> >  	$(JPEG_LIBS) \
> >  	$(NULL)
> >  
> > +test_stream_port_SOURCES = \
> > +	test-stream-port.cpp \
> > +	../stream-port.cpp \
> > +	../error.cpp \
> > +	$(NULL)
> > +
> >  EXTRA_DIST = \
> >  	test-hexdump.sh \
> >  	hexdump1.in \
> > diff --git a/src/unittests/test-stream-port.cpp
> > b/src/unittests/test-stream-port.cpp
> > new file mode 100644
> > index 0000000..3f9dadf
> > --- /dev/null
> > +++ b/src/unittests/test-stream-port.cpp
> > @@ -0,0 +1,69 @@
> 
> Didn't notice previously.
> You should add the copyright/file header.

Yeah, I forgot it here, will add it.

> > +#define CATCH_CONFIG_MAIN
> > +#include <catch/catch.hpp>
> > +#include <sys/socket.h>
> > +
> > +#include "stream-port.hpp"
> > +
> > +
> > +namespace ssa = spice::streaming_agent;
> > +
> > +/*
> > + * Note the semantics of a socketpair may be different from the virtio port
> > + * that is actually used for the real interface.
> > + */
> > +SCENARIO("test basic IO on the stream port", "[port][io]") {
> > +    GIVEN("An open port (socketpair)") {
> > +        int fd[2];
> > +        const char *src_buf = "brekeke";
> > +        const size_t src_size = strlen(src_buf);
> > +
> > +        socketpair(AF_LOCAL, SOCK_STREAM, 0, fd);
> > +
> > +        WHEN("reading data in one go") {
> > +            CHECK(write(fd[0], src_buf, src_size) == src_size);
> > +            char buf[10];
> > +            ssa::read_all(fd[1], buf, src_size);
> > +            CHECK(std::string(buf, src_size) == src_buf);
> > +        }
> > +
> > +        WHEN("reading data in two steps") {
> > +            CHECK(write(fd[0], src_buf, src_size) == src_size);
> > +            char buf[10];
> > +            ssa::read_all(fd[1], buf, 3);
> > +            CHECK(std::string(buf, 3) == "bre");
> > +            ssa::read_all(fd[1], buf, 4);
> > +            CHECK(std::string(buf, 4) == "keke");
> > +        }
> > +
> > +        WHEN("writing data") {
> > +            ssa::write_all(fd[1], src_buf, src_size);
> > +            char buf[10];
> > +            CHECK(read(fd[0], buf, src_size) == src_size);
> > +            CHECK(std::string(buf, src_size) == src_buf);
> > +        }
> > +
> > +        WHEN("closing the remote end and trying to read") {
> > +            CHECK(write(fd[0], src_buf, src_size) == src_size);
> > +            char buf[10];
> > +            ssa::read_all(fd[1], buf, 3);
> > +            CHECK(std::string(buf, 3) == "bre");
> > +            CHECK(close(fd[0]) == 0);
> > +            ssa::read_all(fd[1], buf, 4);
> > +            CHECK(std::string(buf, 4) == "keke");
> > +            // TODO blocks infinitely, we should recognize the remote end is
> > closed
> > +            //ssa::read_all(fd[1], buf, 1);
> > +        }
> > +
> > +        WHEN("closing the remote end and trying to write") {
> > +            ssa::write_all(fd[1], src_buf, src_size);
> > +            char buf[10];
> > +            CHECK(close(fd[0]) == 0);
> > +            // TODO causes a SIGPIPE
> > +            //ssa::write_all(fd[1], src_buf, src_size);
> > +        }
> 
> This is testing nothing actually, I would add the complete scenario
> on following patch

Well, it documents the failing test case, if it wasn't for the
discrepancy in behaviour of the virtio port and the socketpair, it
could also be used to investigate the bug (before it was fixed) easily
in the test case. But if you insist, I'll remove it.

Cheers,
Lukas

> > +
> > +        // clean up the descriptors in case they are still open
> > +        close(fd[0]);
> > +        close(fd[1]);
> > +    }
> > +}
> 
> Frediano