Re: [Qemu-devel] [PATCH 2/2] Acceptance tests: add SPICE protocol check

[Cleber Rosa]

On Fri, Jun 28, 2019 at 05:54:37PM -0300, Wainer dos Santos Moschetta wrote:
> 
> On 06/21/2019 03:09 AM, Cleber Rosa wrote:
> > This fires a QEMU binary with SPICE enabled, and does a basic
> > handshake, doing a basic client/server interaction and protocol
> > validation.
> > 
> > Signed-off-by: Cleber Rosa <address@hidden>
> > ---
> >   .travis.yml               |  5 +++-
> >   tests/Makefile.include    |  6 +++++
> >   tests/acceptance/spice.py | 54 +++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 64 insertions(+), 1 deletion(-)
> >   create mode 100644 tests/acceptance/spice.py
> > 
> > diff --git a/.travis.yml b/.travis.yml
> > index aeb9b211cd..6c9257a459 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -231,7 +231,7 @@ matrix:
> >       # Acceptance (Functional) tests
> >       - env:
> > -        - CONFIG="--python=/usr/bin/python3 
> > --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu"
> > +        - CONFIG="--python=/usr/bin/python3 --enable-spice 
> > --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu"
> >           - TEST_CMD="make check-acceptance"
> >         after_failure:
> >           - cat tests/results/latest/job.log
> > @@ -240,6 +240,9 @@ matrix:
> >             packages:
> >               - python3-pip
> >               - python3.5-venv
> > +            - libspice-protocol-dev
> > +            - libspice-server-dev
> > +
> >       # Using newer GCC with sanitizers
> >       - addons:
> >           apt:
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index 4c97da2878..7fc2d28099 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -1129,6 +1129,12 @@ AVOCADO_SHOW=app
> >   # Additional tags that are added to each occurence of "--filter-by-tags"
> >   AVOCADO_EXTRA_TAGS := ,-flaky
> > +# At last one test require spice to be enabled, allow it to be excluded
> > +# if it's not enabled
> > +ifneq ($(findstring y,"$(CONFIG_SPICE)"),y)
> > +AVOCADO_EXTRA_TAGS := $(AVOCADO_EXTRA_TAGS),-spice
> > +endif
> > +
> 
> Cleber, what about that improvement to avocado_qemu you were developing to
> parse the configure files then expose the enabled/disabled features to test
> code? Do you still plan to push it and so this proposal is just temporary?
>

That was actually a prototype that was done *before* the days of
"avocado_qemu"[1].  While the main reason for it to not have moved forward
back then was the requirement of a build environment, I believe we can
adapt some of the lessons learned there into a generic set of features
for the test runner.

Basically:

 * a generic capability mechanism should be present, with possibly many
   implementations (looking at the build environment is clearly one)

 * Jobs should be able to include/exlude tests based on capabilities
   (akin to how we're using tags)

 * for some other cases, tests should also be given a chance to loop at
   capabilities and decided to abort (cancel) at run time.

Having said that, I think we can start with the tools that we have,
which should serve to make the scope of those future enhancements and
features even clearer and better defined.

Regards,
- Cleber.

[1] - https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg06757.html

> >   AVOCADO_TAGS=$(patsubst 
> > %-softmmu,--filter-by-tags=arch:%$(AVOCADO_EXTRA_TAGS), $(filter 
> > %-softmmu,$(TARGET_DIRS)))
> >   ifneq ($(findstring v2,"v$(PYTHON_VERSION)"),v2)
> > diff --git a/tests/acceptance/spice.py b/tests/acceptance/spice.py
> > new file mode 100644
> > index 0000000000..aa22b1992d
> > --- /dev/null
> > +++ b/tests/acceptance/spice.py
> > @@ -0,0 +1,54 @@
> > +# Simple functional tests for SPICE functionality
> > +#
> > +# Copyright (c) 2019 Red Hat, Inc.
> > +#
> > +# Author:
> > +#  Cleber Rosa <address@hidden>
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > +# later.  See the COPYING file in the top-level directory.
> > +
> > +import socket
> > +import struct
> > +
> > +from avocado_qemu import Test
> > +from avocado.utils.network import find_free_port
> > +
> > +
> > +class Spice(Test):
> > +
> > +    def test_protocol(self):
> > +        """
> > +        :avocado: tags=quick
> > +        :avocado: tags=spice
> > +        """
> > +        port = find_free_port(5001, 5500, sequent=False)
> > +        self.vm.add_args('-nodefaults', '-S',
> > +                         '-spice', 'port=%d,disable-ticketing' % port)
> > +        self.vm.launch()
> > +
> > +        RED_MAGIC = 0x51444552
> > +        MAJOR_VERSION = 0x2
> > +
> > +        client = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> > +        client.connect(('127.0.0.1', port))
> > +        red_link_mess = struct.pack('<10I',
> > +                                    RED_MAGIC,      # magic
> > +                                    MAJOR_VERSION,  # major version
> > +                                    0x0,            # minor version
> > +                                    0x18,           # size in bytes from 
> > here
> > +                                    0x0,            # connection id
> > +                                    0x1,            # channel type 
> > RED_CHANNEL_MAIN
> > +                                    0x0,            # channel id
> > +                                    0x0,            # number of common caps
> > +                                    0x0,            # number of channel 
> > caps
> > +                                    0x14)           # caps offset from size
> > +        client.send(red_link_mess)
> > +
> > +        RED_LINK_REPLY_BASE_FMT = '<5I'  # magic, major, minor, size, error
> > +        red_link_reply = 
> > client.recv(struct.calcsize(RED_LINK_REPLY_BASE_FMT))
> > +        (magic, major, minor, size, error) = 
> > struct.unpack_from(RED_LINK_REPLY_BASE_FMT,
> > +                                                                
> > red_link_reply)
> > +        self.assertEqual(magic, RED_MAGIC, "Mismatch of MAGIC number")
> > +        self.assertEqual(major, MAJOR_VERSION, "Mismatch of major protocol 
> > version")
> > +        self.assertEqual(error, 0x0, "Unexpected error reported by server")
> 
> That test case looks good to me.
> 
> - Wainer