[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, will do.

> > +#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 a broken test case that is fixed by the following
patch. If it wasn't for the discrepancy between the virtio port and the
socketpair, it could also be used to easily investigate the bug in the
test case (before it was fixed) if someone wanted to. 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