fix skill initialization compat + unittests (#95)

* Update base.py

* Update base.py

* typo

* move logic to metaclass

* unittest imports

* unittests

* better logs

* simplify

* automations

* more legacy compat

* more legacy compat tests

* more legacy compat tests

* keep compat

* logs

* bye mycroft

.github/workflows/build_tests.yml

@@ -51,4 +51,6 @@ jobs:
         with:
           # Ignore setuptools vulnerability we can't do much about
           ignore-vulns: |
-            GHSA-r9hx-vwmv-q579
+            GHSA-r9hx-vwmv-q579
+            PYSEC-2023-74
+            PYSEC-2022-43012

.github/workflows/coverage.yml

@@ -33,7 +33,7 @@ jobs:
       run: |
         pip install pytest
         pip install pytest-cov
-        pytest --cov=./test/unittests --cov-report=xml
+        pytest --cov=./ovos_workshop --cov-report=xml
     - name: Upload coverage to Codecov
       uses: codecov/codecov-action@v3
       with:

.github/workflows/unit_tests.yml

@@ -61,18 +61,6 @@ jobs:
           # NOTE: additional pytest invocations should also add the --cov-append flag
           #       or they will overwrite previous invocations' coverage reports
           #       (for an example, see OVOS Skill Manager's workflow)
-      - name: Replace ovos-core with mycroft-core
-        run: |
-          pip uninstall ovos-core -y
-          pip uninstall ovos-lingua-franca -y
-          pip install git+https://github.com/MycroftAI/mycroft-core
-          pip install .
-      - name: Run mycroft unittests
-        run: |
-          pytest --cov-append --cov=ovos_workshop --cov-report xml test/unittests
-          # NOTE: additional pytest invocations should also add the --cov-append flag
-          #       or they will overwrite previous invocations' coverage reports
-          #       (for an example, see OVOS Skill Manager's workflow)
       - name: Upload coverage
         env:
           CODECOV_TOKEN: ${{secrets.CODECOV_TOKEN}}

ovos_workshop/skills/base.py

@@ -200,30 +200,6 @@ class BaseSkill:
         bus (MycroftWebsocketClient): Optional bus connection
     """
 
-    def __new__(cls, *args, **kwargs):
-        if "skill_id" in kwargs and "bus" in kwargs:
-            skill_id = kwargs["skill_id"]
-            bus = kwargs["bus"]
-            try:
-                # skill follows latest best practices, accepts kwargs and does its own init
-                return super().__new__(cls, skill_id=skill_id, bus=bus)
-            except Exception as e:
-                LOG.info(e)
-            try:
-                # skill did not update its init method, let's do some magic to init it manually
-                skill = super().__new__(cls, *args, **kwargs)
-                skill._startup(bus, skill_id)
-                return skill
-            except Exception as e:
-                LOG.info(e)
-
-        # skill loader was not used to create skill object, log a warning and
-        # do the legacy init
-        LOG.warning(f"{cls.__name__} not fully inited, self.bus and "
-                    f"self.skill_id will only be available in self.initialize. "
-                    f"Pass kwargs `skill_id` and `bus` to resolve this.")
-        return super().__new__(cls)
-
     def __init__(self, name=None, bus=None, resources_dir=None,
                  settings: JsonStorage = None,
                  gui=None, enable_settings_manager=True,

ovos_workshop/skills/mycroft_skill.py

@@ -14,16 +14,87 @@
 #
 """Common functionality relating to the implementation of mycroft skills."""
 
+import inspect
 import shutil
 from abc import ABCMeta
-from os.path import join, exists
+from os.path import join, exists, dirname
 
 from ovos_utils.log import LOG
+
 from ovos_workshop.skills.base import BaseSkill, is_classic_core
 
 
 class _SkillMetaclass(ABCMeta):
-    """ To override isinstance checks we need to use a metaclass """
+    """
+    this metaclass ensures we can load skills like regular python objects
+    mycroft-core required a skill loader helper class, which created the skill and then finished object init
+    this means skill_id and bus are not available in init method, mycroft introduced a method named initialize meant for this
+
+    to make skills pythonic and standalone, this metaclass is used to auto init old skills and help in migrating to new standards
+
+    To override isinstance checks we also need to use a metaclass
+
+    TODO: remove compat ovos-core 0.2.0, including MycroftSkill class
+    """
+
+    def __call__(cls, *args, **kwargs):
+        from ovos_bus_client import MessageBusClient
+        from ovos_utils.messagebus import FakeBus
+        bus = None
+        skill_id = None
+
+        if "bus" not in kwargs:
+            for a in args:
+                if isinstance(a, MessageBusClient) or isinstance(a, FakeBus):
+                    bus = a
+                    LOG.warning(f"bus should be a kwarg, guessing {a} is the bus")
+                    break
+            else:
+                LOG.warning("skill initialized without bus!! this is legacy behaviour and"
+                            " requires you to call skill.bind(bus) or skill._startup(skill_id, bus)\n"
+                            "bus will be required starting on ovos-core 0.1.0")
+                return super().__call__(*args, **kwargs)
+
+        if "skill_id" in kwargs:
+            skill_id = kwargs.pop("skill_id")
+        if "bus" in kwargs:
+            bus = kwargs.pop("bus")
+        if not skill_id:
+            LOG.warning(f"skill_id should be a kwarg, please update {cls.__name__}")
+            if args and isinstance(args[0], str):
+                a = args[0]
+                if a[0].isupper():  # in mycroft name is CamelCase by convention, not skill_id
+                    LOG.debug(f"ambiguous skill_id, ignoring {a} as it appears to be a CamelCase name")
+                else:
+                    LOG.warning(f"ambiguous skill_id, assuming positional argument: {a}")
+                    skill_id = a
+
+            if not skill_id:
+                LOG.warning("skill initialized without skill_id!! this is legacy behaviour and"
+                            " requires you to call skill._startup(skill_id, bus)\n"
+                            "skill_id will be required starting on ovos-core 0.1.0")
+                return super().__call__(*args, **kwargs)
+
+                # by convention skill_id is the folder name
+                # usually repo.author
+                # TODO - uncomment once above is deprecated
+                #skill_id = dirname(inspect.getfile(cls)).split("/")[-1]
+                #LOG.warning(f"missing skill_id, assuming folder name convention: {skill_id}")
+
+        try:
+            # skill follows latest best practices, accepts kwargs and does its own init
+            return super().__call__(skill_id=skill_id, bus=bus, **kwargs)
+        except TypeError:
+            LOG.warning("legacy skill signature detected, attempting to init skill manually, "
+                        f"self.bus and self.skill_id will only be available in self.initialize.\n" +
+                        f"__init__ method needs to accept `skill_id` and `bus` to resolve this.")
+
+        # skill did not update its init method, let's do some magic to init it manually
+        # NOTE: no try: except because all skills must accept this initialization and we want exception
+        # this is what skill loader does internally
+        skill = super().__call__(*args, **kwargs)
+        skill._startup(bus, skill_id)
+        return skill
 
     def __instancecheck__(self, instance):
         if is_classic_core():

test/unittests/skills/test_mycroft_skill.py

@@ -20,8 +20,8 @@
 from os.path import join, dirname, abspath
 from unittest.mock import MagicMock, patch
 
-import pytest
-from adapt.intent import IntentBuilder
+#import pytest
+from ovos_utils.intents import IntentBuilder
 from ovos_bus_client import Message
 from ovos_config.config import Configuration
 
@@ -247,7 +247,7 @@ def check_register_decorators(self, result_list):
                          sorted(result_list, key=lambda d: sorted(d.items())))
         self.emitter.reset()
 
-    @pytest.mark.skip
+    #@pytest.mark.skip
     def test_register_decorators(self):
         """ Test decorated intents """
         path_orig = sys.path

test/unittests/test_skill.py

@@ -12,6 +12,66 @@
 from ovos_workshop.skill_launcher import SkillLoader
 
 
+class LegacySkill(CoreSkill):
+    def __init__(self, skill_name="LegacySkill", bus=None, **kwargs):
+        self.inited = True
+        self.initialized = False
+        self.startup_called = False
+        super().__init__(skill_name, bus, **kwargs)
+        # __new__ calls `_startup` so this should be defined in __init__
+        assert self.skill_id is not None
+
+    def initialize(self):
+        self.initialized = True
+
+    def _startup(self, bus, skill_id=""):
+        self.startup_called = True
+        self.initialize()
+
+
+class BadLegacySkill(LegacySkill):
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        print(self.bus)  # not set, exception in property
+
+
+class GoodLegacySkill(CoreSkill):
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        print(self.bus)  # maybe not set, exception in property
+
+
+class SpecificArgsSkill(OVOSSkill):
+    def __init__(self, skill_id="SpecificArgsSkill", bus=None, **kwargs):
+        self.inited = True
+        self.initialized = False
+        self.startup_called = False
+        super().__init__(skill_id=skill_id, bus=bus, **kwargs)
+        self.kwargs = kwargs
+
+    def initialize(self):
+        self.initialized = True
+
+    def _startup(self, bus, skill_id=""):
+        self.startup_called = True
+        self.initialize()
+
+
+class KwargSkill(OVOSSkill):
+    def __init__(self, **kwargs):
+        self.inited = True
+        self.initialized = False
+        self.startup_called = False
+        super().__init__(**kwargs)
+
+    def initialize(self):
+        self.initialized = True
+
+    def _startup(self, bus, skill_id=""):
+        self.startup_called = True
+        self.initialize()
+
+
 class TestSkill(unittest.TestCase):
     def setUp(self):
         self.bus = FakeBus()
@@ -101,3 +161,49 @@ def test_stop(self):
 
     def tearDown(self) -> None:
         self.skill.unload()
+
+
+class TestSkillNew(unittest.TestCase):
+    def test_legacy(self):
+        bus = FakeBus()
+
+        # a legacy skill accepts wrong args, but accepts kwargs
+        legacy = LegacySkill("LegacyName", bus, skill_id="legacy.mycroft")
+        self.assertTrue(legacy.inited)
+        self.assertTrue(legacy.initialized)
+        self.assertTrue(legacy.startup_called)
+        self.assertIsNotNone(legacy.skill_id)
+        self.assertEqual(legacy.bus, bus)
+
+        # a legacy skill not accepting args at all
+        with self.assertRaises(Exception) as ctxt:
+            BadLegacySkill()  # accesses self.bus in __init__
+        self.assertTrue("Accessed MycroftSkill.bus in __init__" in str(ctxt.exception))
+
+        legacynoargs = LegacySkill()  # no exception this time because bus is not used in init
+        self.assertTrue(legacynoargs.inited)
+        self.assertFalse(legacynoargs.initialized)
+        self.assertFalse(legacynoargs.startup_called)
+
+        # a legacy skill fully inited at once
+        legacy = GoodLegacySkill(skill_id="legacy.mycroft", bus=bus)  # accesses self.bus in __init__
+        self.assertEqual(legacy.skill_id, "legacy.mycroft")
+        self.assertEqual(legacy.bus, bus)
+
+    def test_load(self):
+        bus = FakeBus()
+        kwarg = KwargSkill(skill_id="kwarg", bus=bus)
+        self.assertTrue(kwarg.inited)
+        self.assertTrue(kwarg.initialized)
+        self.assertTrue(kwarg.startup_called)
+        self.assertEqual(kwarg.skill_id, "kwarg")
+        self.assertEqual(kwarg.bus, bus)
+
+        gui = Mock()
+        args = SpecificArgsSkill("args", bus, gui=gui)
+        self.assertTrue(args.inited)
+        self.assertTrue(args.initialized)
+        self.assertTrue(args.startup_called)
+        self.assertEqual(args.skill_id, "args")
+        self.assertEqual(args.bus, bus)
+        self.assertEqual(args.gui, gui)