feat: explicit message response to activate/deactivate skill (#369)

OpenVoiceOS · Oct 7, 2023 · c45143d · c45143d
1 parent 6132f04
commit c45143d
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 13 deletions.
diff --git a/ovos_core/skill_manager.py b/ovos_core/skill_manager.py
@@ -689,8 +689,10 @@ def deactivate_skill(self, message):
                 if message.data['skill'] == skill_loader.skill_id:
                     LOG.info("Deactivating skill: " + skill_loader.skill_id)
                     skill_loader.deactivate()
-        except Exception:
+                    self.bus.emit(message.response())
+        except Exception as err:
             LOG.exception('Failed to deactivate ' + message.data['skill'])
+            self.bus.emit(message.response({'error': f'failed: {err}'}))
 
     def deactivate_except(self, message):
         """Deactivate all skills except the provided."""
@@ -715,8 +717,10 @@ def activate_skill(self, message):
                 if (message.data['skill'] in ('all', skill_loader.skill_id)
                         and not skill_loader.active):
                     skill_loader.activate()
-        except Exception:
-            LOG.exception('Couldn\'t activate skill')
+                    self.bus.emit(message.response())
+        except Exception as err:
+            LOG.exception(f'Couldn\'t activate skill {message.data["skill"]}')
+            self.bus.emit(message.response({'error': f'failed: {err}'}))
 
     def stop(self):
         """Tell the manager to shutdown."""

diff --git a/test/unittests/skills/test_skill_manager.py b/test/unittests/skills/test_skill_manager.py
@@ -18,6 +18,7 @@
 from mycroft.skills.skill_loader import SkillLoader
 from mycroft.skills.skill_manager import SkillManager, UploadQueue
 from ..base import MycroftUnitTestBase
+from ovos_bus_client.message import Message
 
 
 class TestUploadQueue(TestCase):
@@ -153,14 +154,25 @@ def test_handle_paired(self):
             reload_skills_manifest=True)
 
     def test_deactivate_skill(self):
-        message = Mock()
-        message.data = dict(skill='test_skill')
+        message = Message("test.message", {'skill': 'test_skill'})
+        message.response = Mock()
         self.skill_manager.deactivate_skill(message)
-        self.skill_loader_mock.deactivate.assert_called_once_with()
+        self.skill_loader_mock.deactivate.assert_called_once()
+        message.response.assert_called_once()
+
+    @patch("ovos_utils.log.LOG.exception")
+    def test_deactivate_skill_exception(self, mock_exception_logger):
+        message = Message("test.message", {'skill': 'test_skill'})
+        message.response = Mock()
+        self.skill_loader_mock.deactivate.side_effect = Exception()
+        self.skill_manager.deactivate_skill(message)
+        self.skill_loader_mock.deactivate.assert_called_once()
+        message.response.assert_called_once()
+        mock_exception_logger.assert_called_once_with('Failed to deactivate test_skill')
 
     def test_deactivate_except(self):
-        message = Mock()
-        message.data = dict(skill='test_skill')
+        message = Message("test.message", {'skill': 'test_skill'})
+        message.response = Mock()
         self.skill_loader_mock.active = True
         foo_skill_loader = Mock(spec=SkillLoader)
         foo_skill_loader.skill_id = 'foo'
@@ -173,19 +185,37 @@ def test_deactivate_except(self):
         self.skill_manager.skill_loaders['test_skill'] = test_skill_loader
 
         self.skill_manager.deactivate_except(message)
-        foo_skill_loader.deactivate.assert_called_once_with()
-        foo2_skill_loader.deactivate.assert_called_once_with()
+        foo_skill_loader.deactivate.assert_called_once()
+        foo2_skill_loader.deactivate.assert_called_once()
         self.assertFalse(test_skill_loader.deactivate.called)
 
     def test_activate_skill(self):
-        message = Mock()
-        message.data = dict(skill='test_skill')
+        message = Message("test.message", {'skill': 'test_skill'})
+        message.response = Mock()
+        test_skill_loader = Mock(spec=SkillLoader)
+        test_skill_loader.skill_id = 'test_skill'
+        test_skill_loader.active = False
+
+        self.skill_manager.skill_loaders = {}
+        self.skill_manager.skill_loaders['test_skill'] = test_skill_loader
+
+        self.skill_manager.activate_skill(message)
+        test_skill_loader.activate.assert_called_once()
+        message.response.assert_called_once()
+
+    @patch("ovos_utils.log.LOG.exception")
+    def test_activate_skill_exception(self, mock_exception_logger):
+        message = Message("test.message", {'skill': 'test_skill'})
+        message.response = Mock()
         test_skill_loader = Mock(spec=SkillLoader)
+        test_skill_loader.activate.side_effect = Exception()
         test_skill_loader.skill_id = 'test_skill'
         test_skill_loader.active = False
 
         self.skill_manager.skill_loaders = {}
         self.skill_manager.skill_loaders['test_skill'] = test_skill_loader
 
         self.skill_manager.activate_skill(message)
-        test_skill_loader.activate.assert_called_once_with()
+        test_skill_loader.activate.assert_called_once()
+        message.response.assert_called_once()
+        mock_exception_logger.assert_called_once_with('Couldn\'t activate skill test_skill')