Make the remote api mismatch message clearer (#687)

* Make the remote api message clearer

* Tweaked wording

* Extract api version checking to function

* Test the new remote_api checking function

* Review feedback

checkbox-ng/checkbox_ng/launcher/master.py

@@ -166,6 +166,48 @@ def invoked(self, ctx):
         else:
             print(_("\nConnection timed out."))
 
+    def check_remote_api_match(self):
+        """
+        Check that agent and controller are running on the same
+        REMOTE_API_VERSION else exit checkbox with an error
+        """
+        agent_api_version = self.sa.get_remote_api_version()
+        controller_api_version = RemoteSessionAssistant.REMOTE_API_VERSION
+
+        if agent_api_version == controller_api_version:
+            return
+
+        newer_msg = _(
+            "The controller that you are using is newer than the agent "
+            "you are trying to connect to.\n"
+            "To solve this, upgrade the agent to the controller version.\n"
+            "If you are unsure about the nomenclature or what any of this "
+            "means, see:\n"
+            "https://checkbox.readthedocs.io/en/latest/reference/"
+            "glossary.html\n\n"
+            "Error: (Agent version: {}, Controller version {})"
+        )
+
+        older_msg = _(
+            "The controller that you are using is older than the agent "
+            "you are trying to connect to.\n"
+            "To solve this, upgrade the controller to the agent version.\n"
+            "If you are unsure about the nomenclature or what any of this "
+            "means, see:\n"
+            "https://checkbox.readthedocs.io/en/latest/reference/"
+            "glossary.html\n\n"
+            "Error: (Agent version: {}, Controller version {})"
+        )
+
+        if controller_api_version > agent_api_version:
+            problem_msg = newer_msg
+        else:
+            problem_msg = older_msg
+
+        raise SystemExit(
+            problem_msg.format(agent_api_version, controller_api_version)
+        )
+
     def connect_and_run(self, host, port=18871):
         config = rpyc.core.protocol.DEFAULT_CONFIG.copy()
         config["allow_all_attrs"] = True
@@ -180,7 +222,7 @@ def connect_and_run(self, host, port=18871):
         #  check if ever disconnected
         ever_disconnected = False
         #  this to animate the dash
-        spinner = itertools.cycle('-\\|/')
+        spinner = itertools.cycle("-\\|/")
         #  this tracks the disconnection time
         disconnection_time = 0
         while True:
@@ -233,14 +275,9 @@ def quitter(msg):
                             " SUT!"
                         )
                     )
-                master_api_version = RemoteSessionAssistant.REMOTE_API_VERSION
-                if slave_api_version != master_api_version:
-                    raise SystemExit(
-                        _(
-                            "Remote API version mismatch. Service "
-                            "uses: {}. Remote uses: {}"
-                        ).format(slave_api_version, master_api_version)
-                    )
+
+                self.check_remote_api_match()
+
                 state, payload = self.sa.whats_up()
                 _logger.info("remote: Main dispatch with state: %s", state)
                 if printed_reconnecting and ever_disconnected:
@@ -297,7 +334,7 @@ def quitter(msg):
                     ever_disconnected = True
                     printed_reconnecting = True
                 print(next(spinner), end="\b", flush=True)
-                time.sleep(.25)
+                time.sleep(0.25)
             except KeyboardInterrupt:
                 interrupted = True
 

checkbox-ng/checkbox_ng/launcher/test_master.py

@@ -60,3 +60,43 @@ def test_invoked_ok(
             RemoteMaster.invoked(self_mock, ctx_mock)
 
         self.assertTrue(self_mock.connect_and_run.called)
+
+    @mock.patch("checkbox_ng.launcher.master.RemoteSessionAssistant")
+    def test_check_remote_api_match_ok(self, remote_assistant_mock):
+        """
+        Test that the check_remote_api_match function does not fail/crash
+        if the two versions match
+        """
+        self_mock = mock.MagicMock()
+        session_assistant_mock = mock.MagicMock()
+        self_mock.sa = session_assistant_mock
+
+        remote_assistant_mock.REMOTE_API_VERSION = 0
+        session_assistant_mock.get_remote_api_version.return_value = 0
+
+        RemoteMaster.check_remote_api_match(self_mock)
+
+    @mock.patch("checkbox_ng.launcher.master.RemoteSessionAssistant")
+    def test_check_remote_api_match_fail(self, remote_assistant_mock):
+        """
+        Test that the check_remote_api_match function exits checkbox
+        if the two versions don't match
+        """
+        self_mock = mock.MagicMock()
+        session_assistant_mock = mock.MagicMock()
+        self_mock.sa = session_assistant_mock
+
+        remote_assistant_mock.REMOTE_API_VERSION = 1
+        session_assistant_mock.get_remote_api_version.return_value = 0
+
+        with self.assertRaises(SystemExit):
+            # this should exit checkbox because the two versions are different
+            RemoteMaster.check_remote_api_match(self_mock)
+
+        remote_assistant_mock.REMOTE_API_VERSION = 0
+        session_assistant_mock.get_remote_api_version.return_value = 1
+
+        with self.assertRaises(SystemExit):
+            # this should also exit checkbox because the two versions are
+            # different
+            RemoteMaster.check_remote_api_match(self_mock)