fix/deactivate_in_converse/fallback (#452)

* fix/deactivate_in_converse/fallback

avoid the caveats documented in #451

companion to OpenVoiceOS/OVOS-workshop#199

* improve tests to account for message order

* tests

requirements/requirements.txt

@@ -12,7 +12,7 @@ ovos-plugin-manager<0.1.0, >=0.0.25
 ovos-config~=0.0,>=0.0.13a8
 ovos-lingua-franca>=0.4.7
 ovos-backend-client~=0.1.0
-ovos-workshop<0.1.0, >=0.0.16a26
+ovos-workshop<0.1.0, >=0.0.16a27
 # provides plugins and classic machine learning framework
 ovos-classifiers<0.1.0, >=0.0.0a53
 

test/end2end/session/test_complete_failure.py

@@ -90,9 +90,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that contexts are kept around
         for m in messages:

test/end2end/session/test_converse.py

@@ -78,9 +78,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that "session" is injected
         # (missing in utterance message) and kept in all messages
@@ -162,9 +161,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that "session" is injected
         # (missing in utterance message) and kept in all messages
@@ -431,9 +429,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify skill is no longer in active skills
         self.assertEqual(SessionManager.default_session.active_skills[0][0], self.other_skill_id)
@@ -471,9 +468,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify skill is again in active skills
         self.assertEqual(SessionManager.default_session.active_skills[0][0], self.skill_id)
@@ -526,9 +522,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         ######################################
         # STEP 8 - deactivate inside converse handler
@@ -556,16 +551,6 @@ def wait_for_n_messages(n):
             "intent.service.skills.deactivated",
             f"{self.skill_id}.deactivate",
             "ovos.session.update_default",
-
-            ###########
-            # TODO - activate is called here if converse return True
-            "intent.service.skills.activate",
-            "intent.service.skills.activated",
-            f"{self.skill_id}.activate",
-            "ovos.session.update_default",
-            # /TODO - ovos-workshop PR needed
-            ###########
-
             # needs ovos-workshop PR
             "skill.converse.response",  # conversed!
             # session updated
@@ -576,4 +561,7 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
+
 

test/end2end/session/test_fallback.py

@@ -77,9 +77,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that contexts are kept around
         for m in messages:
@@ -201,10 +200,8 @@ def wait_for_n_messages(n):
         wait_for_n_messages(len(expected_messages))
 
         self.assertEqual(len(expected_messages), len(messages))
-
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that contexts are kept around
         for m in messages:
@@ -241,4 +238,63 @@ def wait_for_n_messages(n):
 
         # test that active skills list has been updated
         for m in messages[10:]:
-            self.assertEqual(m.context["session"]["active_skills"][0][0], self.skill_id)
+            self.assertEqual(m.context["session"]["active_skills"][0][0], self.skill_id)
+
+    def test_deactivate_in_fallback(self):
+        messages = []
+
+        sess = Session("123")
+        sess.activate_skill(self.skill_id)  # skill is active
+
+        def new_msg(msg):
+            nonlocal messages
+            m = Message.deserialize(msg)
+            if m.msg_type in ["ovos.skills.settings_changed", "ovos.common_play.status"]:
+                return  # skip these, only happen in 1st run
+            messages.append(m)
+            print(len(messages), msg)
+
+        def wait_for_n_messages(n):
+            nonlocal messages
+            t = time.time()
+            while len(messages) < n:
+                sleep(0.1)
+                if time.time() - t > 10:
+                    raise RuntimeError("did not get the number of expected messages under 10 seconds")
+
+        self.core.bus.on("message", new_msg)
+
+        utt = Message("fallback_deactivate")
+        self.core.bus.emit(utt)  # set internal test skill flag
+        messages = []
+
+        utt = Message("recognizer_loop:utterance",
+                      {"utterances": ["deactivate fallback"]},
+                      {"session":sess.serialize()})
+        self.core.bus.emit(utt)
+
+        expected_messages = [
+            "recognizer_loop:utterance",
+            # skill is active, so we get converse events
+            f"{self.skill_id}.converse.ping",
+            "skill.converse.pong",
+            # FallbackV2
+            "ovos.skills.fallback.ping",
+            "ovos.skills.fallback.pong",
+            # skill executing
+            f"ovos.skills.fallback.{self.skill_id}.request",
+            f"ovos.skills.fallback.{self.skill_id}.start",
+            "enclosure.active_skill",
+            "speak",
+            # deactivate skill in fallback handler
+            "intent.service.skills.deactivate",
+            "intent.service.skills.deactivated",
+            f"{self.skill_id}.deactivate",
+            # activate events suppressed
+            f"ovos.skills.fallback.{self.skill_id}.response"
+        ]
+        wait_for_n_messages(len(expected_messages))
+
+        self.assertEqual(len(expected_messages), len(messages))
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])

test/end2end/session/test_fallback_v1.py

@@ -85,9 +85,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that contexts are kept around
         for m in messages:

test/end2end/session/test_sched.py

@@ -68,9 +68,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that "session" and "lang" is injected
         # (missing in utterance message) and kept in all messages
@@ -190,9 +189,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that "session" is the same in all message
         # (missing in utterance message) and kept in all messages

test/end2end/session/test_session.py

@@ -67,9 +67,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that "session" and "lang" is injected
         # (missing in utterance message) and kept in all messages
@@ -174,9 +173,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that contexts are kept around
         for m in messages:
@@ -290,9 +288,8 @@ def wait_for_n_messages(n):
 
         self.assertEqual(len(expected_messages), len(messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # verify that contexts are kept around
         for m in messages:

test/end2end/session/test_stop.py

@@ -120,9 +120,8 @@ def wait_for_n_messages(n):
 
         wait_for_n_messages(len(expected_messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # sanity check correct intent triggered
         self.assertEqual(messages[7].data["utterance"], "hello world")
@@ -294,9 +293,8 @@ def wait_for_n_messages(n):
 
         wait_for_n_messages(len(expected_messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # sanity check correct intent triggered
 
@@ -339,9 +337,8 @@ def wait_for_n_messages(n):
 
         wait_for_n_messages(len(expected_messages))
 
-        mtypes = [m.msg_type for m in messages]
-        for m in expected_messages:
-            self.assertTrue(m in mtypes)
+        for idx, m in enumerate(messages):
+            self.assertEqual(m.msg_type, expected_messages[idx])
 
         # confirm skill self.stop methods called
 

test/end2end/skill-ovos-fallback-unknown/__init__.py

@@ -3,8 +3,18 @@
 
 
 class UnknownSkill(FallbackSkill):
+    def initialize(self):
+        self._fallback_deactivate = False
+        self.add_event("fallback_deactivate",
+                       self.do_deactivate_fallback)
+
+    def do_deactivate_fallback(self, message):
+        self._fallback_deactivate = True
 
     @fallback_handler(priority=100)
     def handle_fallback(self, message):
         self.speak_dialog('unknown')
+        if self._fallback_deactivate:
+            self._fallback_deactivate = False
+            self.deactivate()
         return True