Merge pull request #2503 from forslund/bugfix/converse-error

Bugfix Converse error

mycroft/skills/intent_service.py

@@ -19,7 +19,6 @@ from adapt.engine import IntentDeterminationEngine
 from adapt.intent import IntentBuilder
 
 from mycroft.configuration import Configuration
-from mycroft.messagebus.message import Message
 from mycroft.util.lang import set_active_lang
 from mycroft.util.log import LOG
 from mycroft.util.parse import normalize
@@ -124,7 +123,6 @@ class ContextManager:
             if entity['origin'] != last or entity['origin'] == '':
                 depth += 1
             last = entity['origin']
-            print(depth)
 
         result = []
         if len(missing_entities) > 0:
@@ -198,14 +196,11 @@ class IntentService:
                     self.handle_vocab_manifest)
 
     def update_skill_name_dict(self, message):
-        """
-            Messagebus handler, updates dictionary of if to skill name
-            conversions.
-        """
+        """Messagebus handler, updates dict of id to skill name conversions."""
         self.skill_names[message.data['id']] = message.data['name']
 
     def get_skill_name(self, skill_id):
-        """ Get skill name from skill ID.
+        """Get skill name from skill ID.
 
         Args:
             skill_id: a skill id as encoded in Intent handlers.
@@ -236,6 +231,7 @@ class IntentService:
             return False
 
     def handle_converse_error(self, message):
+        LOG.error(message.data['error'])
         skill_id = message.data["skill_id"]
         if message.data["error"] == "skill id does not exist":
             self.remove_active_skill(skill_id)
@@ -246,14 +242,26 @@ class IntentService:
                 self.active_skills.remove(skill)
 
     def add_active_skill(self, skill_id):
+        """Add a skill or update the position of an active skill.
+
+        The skill is added to the front of the list, if it's already in the
+        list it's removed so there is only a single entry of it.
+
+        Arguments:
+            skill_id (str): identifier of skill to be added.
+        """
         # search the list for an existing entry that already contains it
         # and remove that reference
-        self.remove_active_skill(skill_id)
-        # add skill with timestamp to start of skill_list
-        self.active_skills.insert(0, [skill_id, time.time()])
+        if skill_id != '':
+            self.remove_active_skill(skill_id)
+            # add skill with timestamp to start of skill_list
+            self.active_skills.insert(0, [skill_id, time.time()])
+        else:
+            LOG.warning('Skill ID was empty, won\'t add to list of '
+                        'active skills.')
 
     def update_context(self, intent):
-        """ Updates context with keyword from the intent.
+        """Updates context with keyword from the intent.
 
         NOTE: This method currently won't handle one_of intent keywords
               since it's not using quite the same format as other intent
@@ -272,8 +280,7 @@ class IntentService:
                 self.context_manager.inject_context(context_entity)
 
     def send_metrics(self, intent, context, stopwatch):
-        """
-        Send timing metrics to the backend.
+        """Send timing metrics to the backend.
 
         NOTE: This only applies to those with Opt In.
         """
@@ -291,7 +298,7 @@ class IntentService:
                           {'intent_type': 'intent_failure'})
 
     def handle_utterance(self, message):
-        """ Main entrypoint for handling user utterances with Mycroft skills
+        """Main entrypoint for handling user utterances with Mycroft skills
 
         Monitor the messagebus for 'recognizer_loop:utterance', typically
         generated by a spoken interaction but potentially also from a CLI
@@ -386,7 +393,7 @@ class IntentService:
             LOG.exception(e)
 
     def _converse(self, utterances, lang, message):
-        """ Give active skills a chance at the utterance
+        """Give active skills a chance at the utterance
 
         Args:
             utterances (list):  list of utterances
@@ -403,7 +410,7 @@ class IntentService:
                                   1] <= self.converse_timeout * 60]
 
         # check if any skill wants to handle utterance
-        for skill in self.active_skills:
+        for skill in copy(self.active_skills):
             if self.do_converse(utterances, skill[0], lang, message):
                 # update timestamp, or there will be a timeout where
                 # intent stops conversing whether its being used or not
@@ -412,7 +419,7 @@ class IntentService:
         return False
 
     def _adapt_intent_match(self, raw_utt, norm_utt, lang):
-        """ Run the Adapt engine to search for an matching intent
+        """Run the Adapt engine to search for an matching intent
 
         Args:
             raw_utt (list):  list of utterances
@@ -484,7 +491,7 @@ class IntentService:
         self.engine.intent_parsers = new_parsers
 
     def handle_add_context(self, message):
-        """ Add context
+        """Add context
 
         Args:
             message: data contains the 'context' item to add
@@ -505,7 +512,7 @@ class IntentService:
         self.context_manager.inject_context(entity)
 
     def handle_remove_context(self, message):
-        """ Remove specific context
+        """Remove specific context
 
         Args:
             message: data contains the 'context' item to remove
@@ -515,7 +522,7 @@ class IntentService:
             self.context_manager.remove_context(context)
 
     def handle_clear_context(self, message):
-        """ Clears all keywords from context """
+        """Clears all keywords from context """
         self.context_manager.clear_context()
 
     def handle_get_adapt(self, message):

mycroft/skills/padatious_service.py

@@ -82,6 +82,10 @@ class PadatiousService(FallbackSkill):
         self.registered_intents = []
         self.registered_entities = []
 
+    def make_active(self):
+        """Override the make active since this is not a real fallback skill."""
+        pass
+
     def train(self, message=None):
         padatious_single_thread = Configuration.get()[
             'padatious']['single_thread']

test/unittests/skills/test_intent_service.py

@@ -112,6 +112,45 @@ class ConversationTest(TestCase):
         # Check that a skill responded that it could handle the message
         self.assertTrue(result)
 
+    def test_converse_error(self):
+        """Check that all skill IDs in the active_skills list are called.
+        even if there's an error.
+        """
+        def response(message, return_msg_type):
+            c64 = Message(return_msg_type, {'skill_id': 'c64_skill',
+                                            'result': False})
+            amiga = Message(return_msg_type,
+                            {'skill_id': 'amiga_skill',
+                             'error': 'skill id does not exist'})
+            atari = Message(return_msg_type, {'skill_id': 'atari_skill',
+                                              'result': False})
+            msgs = {'c64_skill': c64,
+                    'atari_skill': atari,
+                    'amiga_skill': amiga}
+
+            return msgs[message.data['skill_id']]
+
+        self.intent_service.add_active_skill('amiga_skill')
+        self.intent_service.bus.wait_for_response.side_effect = response
+
+        hello = ['hello old friend']
+        utterance_msg = Message('recognizer_loop:utterance',
+                                data={'lang': 'en-US',
+                                      'utterances': hello})
+        result = self.intent_service._converse(hello, 'en-US', utterance_msg)
+
+        # Check that the active skill list was updated to set the responding
+        # Skill first.
+
+        # Check that a skill responded that it couldn't handle the message
+        self.assertFalse(result)
+
+        # Check that each skill in the list of active skills were called
+        call_args = self.intent_service.bus.wait_for_response.call_args_list
+        sent_skill_ids = [call[0][0].data['skill_id'] for call in call_args]
+        self.assertEqual(sent_skill_ids,
+                         ['amiga_skill', 'c64_skill', 'atari_skill'])
+
     def test_reset_converse(self):
         """Check that a blank stt sends the reset signal to the skills."""
         def response(message, return_msg_type):