Fix crash in ManifestBrowser when only hidden manifests are present (BugFix) (#1996)

* Fix crash in ManifestBrowser when only hidden manifests are present.

* The manifest screen is not shown even in interactive mode if all the manifests are hidde.

* Comply the code with Python 3.5.

* Adjust tests by putting them in a proper test classes + comments. Reduce code duplication.

* Rename ManifestBrowser.get_flattened_values, apply the same fix for _save_manifest in RemoteController, make tests more readable.

Author: gntzio

checkbox-ng/checkbox_ng/launcher/controller.py

@@ -628,24 +628,20 @@ def _save_manifest(self, interactive):
         if not manifest_repr:
             _logger.info("Skipping saving of the manifest")
             return
-        if interactive:
+
+        if interactive and ManifestBrowser.has_visible_manifests(
+            manifest_repr
+        ):
             # Ask the user the values
             to_save_manifest = ManifestBrowser(
                 "System Manifest:", manifest_repr
             ).run()
         else:
-            # Use the one provided in repr
-            # repr is question : [manifests]
-            #   manifest ex m1 is [conf_m1_1, conf_m1_2, ...]
-            # here we recover [conf_m1_1, conf_m1_2, ..., conf_m2_1, ...]
-            all_preconf = (
-                conf
-                for conf_list in manifest_repr.values()
-                for conf in conf_list
+            # Use the one provided in repr (either non-interactive or no visible manifests)
+            to_save_manifest = ManifestBrowser.get_flattened_values(
+                manifest_repr
             )
-            to_save_manifest = {
-                conf["id"]: conf["value"] for conf in all_preconf
-            }
+
         self.sa.save_manifest_json(json.dumps(to_save_manifest))
 
     def select_jobs(self, all_jobs):

checkbox-ng/checkbox_ng/launcher/subcommands.py

@@ -692,21 +692,20 @@ def _save_manifest(self, interactive):
         if not manifest_repr:
             _logger.info("Skipping saving of the manifest")
             return
-        if interactive:
+
+        if interactive and ManifestBrowser.has_visible_manifests(
+            manifest_repr
+        ):
             # Ask the user the values
             to_save_manifest = ManifestBrowser(
                 "System Manifest:", manifest_repr
             ).run()
         else:
-            # Use the one provided in repr
-            # repr is question : [manifests]
-            #   manifest ex m1 is [conf_m1_1, conf_m1_2, ...]
-            # here we recover [conf_m1_1, conf_m1_2, ..., conf_m2_1, ...]
-            to_save_manifest = {
-                conf["id"]: conf["value"]
-                for conf_list in manifest_repr.values()
-                for conf in conf_list
-            }
+            # Use the one provided in repr (either non-interactive or no visible manifests)
+            to_save_manifest = ManifestBrowser.get_flattened_values(
+                manifest_repr
+            )
+
         self.ctx.sa.save_manifest(to_save_manifest)
 
     def _pick_jobs_to_run(self):

checkbox-ng/checkbox_ng/launcher/test_controller.py

@@ -28,7 +28,7 @@
 from checkbox_ng.launcher.controller import is_hostname_a_loopback
 
 
-class ControllerTests(TestCase):
+class TestRemoteController(TestCase):
     @mock.patch("checkbox_ng.launcher.controller.is_hostname_a_loopback")
     @mock.patch("time.time")
     @mock.patch("builtins.print")
@@ -1063,54 +1063,98 @@ def test_start_session_runtime_error(self):
         with self.assertRaises(SystemExit) as _:
             RemoteController.start_session(self_mock)
 
-    def test__save_manifest_no_repr(self):
-        self_mock = mock.MagicMock()
-        self_mock.sa.get_manifest_repr_json.return_value = "{}"
-        RemoteController._save_manifest(self_mock, False)
-
     @mock.patch("checkbox_ng.launcher.controller.ManifestBrowser")
-    def test__save_manifest_interactive(self, manifest_browser_mock):
-        self_mock = mock.MagicMock()
-        manifest_repr = {"Question 1": [{"id": "conf1", "value": "val1"}]}
-        self_mock.sa.get_manifest_repr_json.return_value = json.dumps(
-            manifest_repr
-        )
-        to_save_manifest = {"conf1": "new_val"}
-        manifest_browser_mock.return_value.run.return_value = to_save_manifest
+    def test__save_manifest_interactive_with_visible_manifests(
+        self, mock_browser_class
+    ):
+        controller = RemoteController()
+        sa_mock = mock.MagicMock()
+        controller._sa = sa_mock
 
-        RemoteController._save_manifest(self_mock, True)
+        manifest_repr = {
+            "section1": [
+                {"id": "visible1", "value": 0, "hidden": False},
+                {"id": "visible2", "value": False, "hidden": False},
+            ]
+        }
+        sa_mock.get_manifest_repr_json.return_value = json.dumps(manifest_repr)
 
-        manifest_browser_mock.assert_called_once_with(
-            "System Manifest:", manifest_repr
-        )
-        self_mock.sa.save_manifest_json.assert_called_once_with(
-            json.dumps(to_save_manifest)
+        mock_browser = mock.MagicMock()
+        mock_browser.run.return_value = {
+            "visible1": 5,
+            "visible2": True,
+        }
+        mock_browser_class.return_value = mock_browser
+        mock_browser_class.has_visible_manifests.return_value = True
+
+        controller._save_manifest(interactive=True)
+
+        sa_mock.save_manifest_json.assert_called_with(
+            json.dumps({"visible1": 5, "visible2": True})
         )
 
-    def test__save_manifest_non_interactive(self):
-        self_mock = mock.MagicMock()
+    @mock.patch("checkbox_ng.launcher.controller.ManifestBrowser")
+    def test__save_manifest_interactive_no_visible_manifests(
+        self, mock_browser_class
+    ):
+        controller = RemoteController()
+        sa_mock = mock.MagicMock()
+        controller._sa = sa_mock
+
         manifest_repr = {
-            "Question 1": [
-                {"id": "conf1", "value": "val1"},
-                {"id": "conf2", "value": "val2"},
-            ],
-            "Question 2": [{"id": "conf3", "value": "val3"}],
+            "section1": [
+                {"id": "hidden1", "value": True, "hidden": True},
+                {"id": "hidden2", "value": 2, "hidden": True},
+            ]
+        }
+        sa_mock.get_manifest_repr_json.return_value = json.dumps(manifest_repr)
+        mock_browser_class.has_visible_manifests.return_value = False
+        mock_browser_class.get_flattened_values.return_value = {
+            "hidden1": True,
+            "hidden2": 2,
         }
-        self_mock.sa.get_manifest_repr_json.return_value = json.dumps(
-            manifest_repr
+
+        controller._save_manifest(interactive=True)
+
+        self.assertEqual(mock_browser_class.call_count, 0)
+        self.assertEqual(
+            mock_browser_class.has_visible_manifests.call_count, 1
+        )
+        self.assertEqual(mock_browser_class.get_flattened_values.call_count, 1)
+        self.assertEqual(sa_mock.save_manifest_json.call_count, 1)
+        sa_mock.save_manifest_json.assert_called_with(
+            json.dumps({"hidden1": True, "hidden2": 2})
         )
 
-        RemoteController._save_manifest(self_mock, False)
+    @mock.patch("checkbox_ng.launcher.controller.ManifestBrowser")
+    def test__save_manifest_non_interactive(self, mock_browser_class):
+        controller = RemoteController()
+        sa_mock = mock.MagicMock()
+        controller._sa = sa_mock
 
-        expected_to_save = {
-            "conf1": "val1",
-            "conf2": "val2",
-            "conf3": "val3",
+        manifest_repr = {
+            "section1": [
+                {"id": "manifest1", "value": False, "hidden": False},
+                {"id": "manifest2", "value": 7, "hidden": True},
+            ]
+        }
+        sa_mock.get_manifest_repr_json.return_value = json.dumps(manifest_repr)
+        mock_browser_class.get_flattened_values.return_value = {
+            "manifest1": False,
+            "manifest2": 7,
         }
-        self_mock.sa.save_manifest_json.assert_called_once_with(
-            json.dumps(expected_to_save)
+
+        controller._save_manifest(interactive=False)
+
+        sa_mock.save_manifest_json.assert_called_with(
+            json.dumps({"manifest1": False, "manifest2": 7})
         )
 
+    def test__save_manifest_no_repr(self):
+        self_mock = mock.MagicMock()
+        self_mock.sa.get_manifest_repr_json.return_value = "{}"
+        RemoteController._save_manifest(self_mock, False)
+
     def test_select_jobs_forced_with_manifest(self):
         self_mock = mock.MagicMock()
         self_mock.launcher.get_value.return_value = True

checkbox-ng/checkbox_ng/launcher/test_subcommands.py

@@ -43,6 +43,7 @@
     get_testplan_id_by_id,
     print_objs,
 )
+from checkbox_ng.urwid_ui import ManifestBrowser
 
 
 class TestSharedFunctions(TestCase):
@@ -780,6 +781,104 @@ def test_invoked_resume(self, load_config_mock):
 
         Launcher.invoked(self_mock, ctx_mock)
 
+    def test__save_manifest_no_or_empty_manifest_repr(self):
+        launcher = Launcher()
+        ctx_mock = MagicMock()
+        launcher.ctx = ctx_mock
+
+        cases = [
+            ("None", None),
+            ("Empty", {}),
+        ]
+
+        for case_name, manifest_repr in cases:
+            with self.subTest(case=case_name):
+                ctx_mock.sa.get_manifest_repr.return_value = manifest_repr
+                launcher._save_manifest(interactive=True)
+                self.assertEqual(ctx_mock.sa.save_manifest.call_count, 0)
+
+    @patch("checkbox_ng.launcher.subcommands.ManifestBrowser")
+    def test__save_manifest_interactive_with_visible_manifests(
+        self, mock_browser_class
+    ):
+
+        launcher = Launcher()
+        ctx_mock = MagicMock()
+        launcher.ctx = ctx_mock
+
+        manifest_repr = {
+            "section1": [
+                {"id": "visible1", "value": 0, "hidden": False},
+                {"id": "visible2", "value": False, "hidden": False},
+            ]
+        }
+        ctx_mock.sa.get_manifest_repr.return_value = manifest_repr
+
+        mock_browser = MagicMock()
+        mock_browser.run.return_value = {
+            "visible1": 5,
+            "visible2": True,
+        }
+        mock_browser_class.return_value = mock_browser
+        mock_browser_class.has_visible_manifests.return_value = True
+
+        launcher._save_manifest(interactive=True)
+
+        ctx_mock.sa.save_manifest.assert_called_with(
+            {"visible1": 5, "visible2": True}
+        )
+
+    @patch("checkbox_ng.launcher.subcommands.ManifestBrowser")
+    def test__save_manifest_interactive_no_visible_manifests(
+        self, mock_browser_class
+    ):
+        launcher = Launcher()
+        ctx_mock = MagicMock()
+        launcher.ctx = ctx_mock
+
+        manifest_repr = {
+            "section1": [
+                {"id": "hidden1", "value": True, "hidden": True},
+                {"id": "hidden2", "value": 2, "hidden": True},
+            ]
+        }
+        ctx_mock.sa.get_manifest_repr.return_value = manifest_repr
+        mock_browser_class.has_visible_manifests.return_value = False
+        mock_browser_class.get_flattened_values.return_value = {
+            "hidden1": True,
+            "hidden2": 2,
+        }
+
+        launcher._save_manifest(interactive=True)
+
+        ctx_mock.sa.save_manifest.assert_called_with(
+            {"hidden1": True, "hidden2": 2}
+        )
+
+    @patch("checkbox_ng.launcher.subcommands.ManifestBrowser")
+    def test__save_manifest_non_interactive(self, mock_browser_class):
+        launcher = Launcher()
+        ctx_mock = MagicMock()
+        launcher.ctx = ctx_mock
+
+        manifest_repr = {
+            "section1": [
+                {"id": "manifest1", "value": False, "hidden": False},
+                {"id": "manifest2", "value": 7, "hidden": True},
+            ]
+        }
+        ctx_mock.sa.get_manifest_repr.return_value = manifest_repr
+        mock_browser_class.get_flattened_values.return_value = {
+            "manifest1": False,
+            "manifest2": 7,
+        }
+
+        launcher._save_manifest(interactive=False)
+
+        ctx_mock.sa.save_manifest.assert_called_with(
+            {"manifest1": False, "manifest2": 7}
+        )
+
 
 @patch("os.makedirs", new=MagicMock())
 class TestLauncherReturnCodes(TestCase):