Merge ~kissiel/checkbox-support:zapper-capabilities-in-proxy into checkbox-support:master

[Maciej Kisielewski]

This MR brings stuff to the Zapper Proxy.

First of all it adds missing unit tests.
It fixes the call in usb_set_state.
It adds the call for getting Zappers functionality.
And it changes the main "driver" of the script from ordinary main to a class, so other code of checkbox-support may use it directly, instead of spawning a new process.

[Paul Larson (pwlars)]

2 minor spelling issues, but I'll hit approve here with the assumption that you'll likely fix those :)

review: Approve

[Devices Certification Bot (ce-certification-qa)]

The merge was fine but running tests failed.

"10.38.105.54"
"10.38.105.108"
"10.38.105.197"
[xenial] [21:00:41] starting container
Device project added to xenial-testing
"10.38.105.54"
[xenial] [21:01:10] provisioning container
[bionic] [21:01:30] starting container
[focal] [21:01:40] starting container
Device project added to bionic-testing
Device project added to focal-testing
"10.38.105.145"
[bionic] [21:01:59] provisioning container
"10.38.105.235"
[focal] [21:02:11] provisioning container
[bionic] [21:02:38] Starting tests...
[bionic] Found a test script: ./requirements/container-tests-checkbox-support
[focal] [21:02:43] Starting tests...
[focal] Found a test script: ./requirements/container-tests-checkbox-support
[xenial] [21:02:56] Starting tests...
[xenial] Found a test script: ./requirements/container-tests-checkbox-support
[focal] [21:03:34] container-tests-checkbox-support: PASS
[focal] [21:03:34] Fixing file permissions in source directory
[bionic] [21:03:35] container-tests-checkbox-support: PASS
[bionic] [21:03:35] Fixing file permissions in source directory
[focal] [21:03:35] Destroying container
[bionic] [21:03:35] Destroying container
[xenial] [21:03:45] container-tests-checkbox-support: FAIL
[xenial] output: https://paste.ubuntu.com/p/VWVgK4f3Zc/
[xenial] [21:03:48] Fixing file permissions in source directory
[xenial] [21:03:48] Destroying container

review: Needs Fixing

diff --git a/checkbox_support/scripts/tests/test_zapper_proxy.py b/checkbox_support/scripts/tests/test_zapper_proxy.py
new file mode 100644
index 0000000..db3e754
--- /dev/null
+++ b/checkbox_support/scripts/tests/test_zapper_proxy.py
@@ -0,0 +1,129 @@
+# Copyright 2022 Canonical Ltd.
+# Written by:
+#   Maciej Kisielewski <[e-mail]>
+#
+# This is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3,
+# as published by the Free Software Foundation.
+#
+# This file is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this file.  If not, see <http://www.gnu.org/licenses/>.
+"""Tests for zapper_proxy module."""
+
+from unittest import TestCase
+from unittest.mock import Mock, patch
+
+from checkbox_support.scripts.zapper_proxy import ZapperControlV1
+
+
+class ZapperProxyV1Tests(TestCase):
+    """Unit tests for ZapperProxyV1 class."""
+
+    def setUp(self):
+        self._mocked_conn = Mock()
+        self._mocked_conn.root = Mock()
+
+    def test_usb_get_state_smoke(self):
+        """
+        Check if usb_get_state calls appropriate function on the rpyc client.
+
+        Current implementation on the service side uses mutable arguments for
+        returning values (C-style stuff that should be removed) this is why we
+        need the stateful side_effect below
+        """
+        def side_effect_fn(_, ret):
+            ret.append('ON')
+            return True
+        self._mocked_conn.root.zombiemux_get_state = Mock(
+            side_effect=side_effect_fn)
+        zapctl = ZapperControlV1(self._mocked_conn)
+
+        with patch('builtins.print') as mocked_print:
+            zapctl.usb_get_state(0)
+            mocked_print.assert_called_once_with(
+                'State for address 0 is ON')
+
+    def test_usb_get_state_fails(self):
+        """Check if usb_get_state quits with a proper message on failure."""
+        self._mocked_conn.root.zombiemux_get_state = Mock(return_value=False)
+        zapctl = ZapperControlV1(self._mocked_conn)
+        with self.assertRaises(SystemExit) as context:
+            zapctl.usb_get_state(0)
+        self.assertEqual(
+            context.exception.code, 'Failed to get state for address 0.')
+
+    def test_usb_set_state_smoke(self):
+        """
+        Check if usb_set_state calls appropriate functions on the rpyc client.
+        """
+        self._mocked_conn.root.zombiemux_set_state = Mock(return_value=True)
+        zapctl = ZapperControlV1(self._mocked_conn)
+        with patch('builtins.print') as mocked_print:
+            zapctl.usb_set_state(0, 'ON')
+            mocked_print.assert_called_once_with(
+                "State 'ON' set for the address 0.")
+
+    def test_usb_set_state_fails(self):
+        """Check if usb_set_state quits with a proper message on failure."""
+        self._mocked_conn.root.zombiemux_set_state = Mock(return_value=False)
+        zapctl = ZapperControlV1(self._mocked_conn)
+        with self.assertRaises(SystemExit) as context:
+            zapctl.usb_set_state(0, 'ON')
+        self.assertEqual(
+            context.exception.code, "Failed to set 'ON' state for address 0.")
+
+    def test_get_capabilities_one_cap(self):
+        """
+        Check if get_capabilities properly prints one record.
+
+        The record should be in Checkbox resource syntax and should not be
+        surrounded by any newlines.
+        """
+        ret_val = [{'foo': 'bar'}]
+        self._mocked_conn.root.get_capabilities = Mock(return_value=ret_val)
+        zapctl = ZapperControlV1(self._mocked_conn)
+        with patch('builtins.print') as mocked_print:
+            zapctl.get_capabilities()
+            mocked_print.assert_called_once_with('foo: bar')
+
+    def test_get_capabilities_empty(self):
+        """Check if get_capabilities prints nothing on no capabilities."""
+        ret_val = []
+        self._mocked_conn.root.get_capabilities = Mock(return_value=ret_val)
+        zapctl = ZapperControlV1(self._mocked_conn)
+        with patch('builtins.print') as mocked_print:
+            zapctl.get_capabilities()
+            mocked_print.assert_called_once_with('')
+
+    def test_get_capabilities_multiple_caps(self):
+        """
+        Check if get_capabilities properly prints multiple records.
+
+        The records should be in Checkbox resource syntax. Records should be
+        separated by an empty line.
+        """
+        ret_val = [{'foo': 'bar'}, {'baz': 'biz'}]
+        self._mocked_conn.root.get_capabilities = Mock(return_value=ret_val)
+        zapctl = ZapperControlV1(self._mocked_conn)
+        with patch('builtins.print') as mocked_print:
+            zapctl.get_capabilities()
+            mocked_print.assert_called_once_with('foo: bar\n\nbaz: biz')
+
+    def test_get_capabilities_one_cap_multi_rows(self):
+        """
+        Check if get_capabilities properly prints a record with multiple caps.
+
+        Each capability should be printed in a separate line.
+        No additional newlines should be printed.
+        """
+        ret_val = [{'foo': 'bar', 'foo2': 'bar2'}]
+        self._mocked_conn.root.get_capabilities = Mock(return_value=ret_val)
+        zapctl = ZapperControlV1(self._mocked_conn)
+        with patch('builtins.print') as mocked_print:
+            zapctl.get_capabilities()
+            mocked_print.assert_called_once_with('foo: bar\nfoo2: bar2')
diff --git a/checkbox_support/scripts/zapper_proxy.py b/checkbox_support/scripts/zapper_proxy.py
index afd998f..e591f71 100644
--- a/checkbox_support/scripts/zapper_proxy.py
+++ b/checkbox_support/scripts/zapper_proxy.py
@@ -57,10 +57,17 @@ class IZapperControl:
         :param str state: state to set on the USBMUX addon
         """
 
+    @abstractmethod
+    def get_capabilities(self):
+        """Get Zapper's setup capabilities in checkbox resource form."""
+
 
 class ZapperControlV1(IZapperControl):
-    """Control Zapper via RPyC using v1 of the API."""
+    """
+    Control Zapper via RPyC using v1 of the API.
 
+    :return str: list of capabilities in Checkbox resource form.
+    """
     def __init__(self, connection):
         self._conn = connection
 
@@ -73,28 +80,84 @@ class ZapperControlV1(IZapperControl):
         print("State for address {} is {}".format(address, ret[0]))
 
     def usb_set_state(self, address, state):
-        success = self._conn.root.zombiemux_get_state(address, state)
+        success = self._conn.root.zombiemux_set_state(address, state)
         if not success:
             raise SystemExit(
                 "Failed to set '{}' state for address {}.".format(
                     state, address))
         print("State '{}' set for the address {}.".format(state, address))
 
+    def get_capabilities(self):
+        capabilities = self._conn.root.get_capabilities()
+
+        def stringify_cap(cap):
+            return '\n'.join(
+                '{}: {}'.format(key, val) for key, val in sorted(cap.items()))
+        print('\n\n'.join(stringify_cap(cap) for cap in capabilities))
+
+
+class ControlVersionDecider:
+    """
+    This class helps establish which API version of Zapper Control to use.
+
+    Normally it would be just in the main function, but some of the clients of
+    this functionality may be internal code from checkbox-support, this class
+    makes it possible to use the code without spawning new python process
+    (processing args etc.)
+
+    It is a class and not a function, because we should inform the user about
+    missing RPyC module as soon as possible, in this case in the __init__.  The
+    RPyC is kept in the instance state, so later on it may be used to run the
+    actual functionality.
+    """
+    def __init__(self):
+        # to not make checkbox-support dependant on RPyC let's use one
+        # available in the system, and if it's not available let's try loading
+        # one provided by Checkbox. Real world usecase would be to run this
+        # program from within Checkbox, so chances for not finding it are
+        # pretty slim.
+        try:
+            self._rpyc = import_module('rpyc')
+        except ImportError:
+            try:
+                self._rpyc = import_module('plainbox.vendor.rpyc')
+            except ImportError as exc:
+                msg = "RPyC not found. Neither from sys nor from Checkbox"
+                raise SystemExit(msg) from exc
+
+    def decide(self, host):
+        """
+        Determine which version of Zapper Control API to use.
+
+        :param str host: Address of the Zapper host to connect to.
+        :returns IZapperControl: An appropriate ZapperControl instance.
+        """
+        conn = self._rpyc.connect(
+            host, 60000, config={"allow_all_attrs": True})
+        try:
+            version = conn.root.get_api_version()
+        except AttributeError:
+            # there was no "get_api_version" method on Zapper
+            # so this means the oldest version possible - 1
+            version = 1
+        # the following mapping could be replaced by something that generates
+        # a class name and tries looking it up in this module, but using dict
+        # feels simpler due to explicitness and can include classes defined in
+        # some other modules
+        control_cls = {
+            1: ZapperControlV1,
+        }.get(version, None)
+        if control_cls is None:
+            raise SystemExit((
+                "Zapper host returned unknown Zapper Control Version: {ver}\n"
+                "Implement ZapperControlV{ver} in checkbox_support!"
+            ).format(ver=version))
+        return control_cls(conn)
+
 
 def main():
     """Entry point."""
-    # to not make checkbox-support dependant on RPyC let's use one available in
-    # the system, and if it's not available let's try loading one provided by
-    # Checkbox. Real world usecase would be to run this program from within
-    # Checkbox, so chances for not finding it are pretty slim.
-    try:
-        rpyc = import_module('rpyc')
-    except ImportError:
-        try:
-            rpyc = import_module('plainbox.vendor.rpyc')
-        except ImportError as exc:
-            raise SystemExit(
-                "RPyC not found. Neither from sys nor from Checkbox") from exc
+    decider = ControlVersionDecider()
 
     # generate argparse from the interface of Zapper Control
     parser = AutoArgParser(cls=IZapperControl)
@@ -112,28 +175,7 @@ def main():
         raise SystemExit(
             "You have to provide Zapper host, either via '--host' or via "
             "ZAPPER_ADDRESS environment variable")
-    # connect and see which protol version should be used
-    conn = rpyc.connect(
-        host, 60000, config={"allow_all_attrs": True})
-    try:
-        version = conn.root.get_api_version()
-    except AttributeError:
-        # there was no "get_api_version" method on Zapper
-        # so this means the oldest version possible - 1
-        version = 1
-    # the following mapping could be replaced by something that generates
-    # a class name and tries looking it up in this module, but using dict
-    # feels simpler due to explicitness and can include classes defined in
-    # some other modules
-    control_cls = {
-        1: ZapperControlV1,
-    }.get(version, None)
-    if control_cls is None:
-        raise SystemExit((
-            "Zapper host returned unknown Zapper Control Version: {ver}\n"
-            "Implement ZapperControlV{ver} in checkbox_support!"
-        ).format(ver=version))
-    zapper_control = control_cls(conn)
+    zapper_control = decider.decide(host)
     parser.run(zapper_control)