From 70b518ecf9028aaff572e0a166076bc0716b7b5c Mon Sep 17 00:00:00 2001 From: aaronayres35 <36972686+aaronayres35@users.noreply.github.com> Date: Tue, 27 Oct 2020 07:37:28 -0500 Subject: [PATCH] Fix Unregister service value error bug (#345) * adding a failing regression test * fix failing test by adding a listener for unregistered services * add a new test which used to pass which now fails after my attempted fix * flake8 * only respond to unregistered services that were originally registered via the CorePlugin's service_offers extension point * add setup and teardown so that neew test actually fails without added fix * suggested change: have plugin handle exception when attempting to unregister a service would fail because it has already been unregistered * adding a clarifying comment * flake8 * making suggested changes from code review --- envisage/core_plugin.py | 1 - envisage/plugin.py | 8 ++- envisage/tests/test_core_plugin.py | 86 ++++++++++++++++++++++++------ 3 files changed, 78 insertions(+), 17 deletions(-) diff --git a/envisage/core_plugin.py b/envisage/core_plugin.py index ecd2ba44e..5e59f3c84 100644 --- a/envisage/core_plugin.py +++ b/envisage/core_plugin.py @@ -138,7 +138,6 @@ def _service_offers_changed(self, event): def start(self): """ Start the plugin. """ - # Load all contributed preferences files into the application's root # preferences node. self._load_preferences(self.preferences) diff --git a/envisage/plugin.py b/envisage/plugin.py index ca2c2bc02..f58647299 100644 --- a/envisage/plugin.py +++ b/envisage/plugin.py @@ -289,7 +289,13 @@ def unregister_services(self): service_ids.reverse() for service_id in service_ids: - self.application.unregister_service(service_id) + try: + self.application.get_service_from_id(service_id) + except ValueError: + # the service may have already been individually unregistered + pass + else: + self.application.unregister_service(service_id) # Just in case the plugin is started again! self._service_ids = [] diff --git a/envisage/tests/test_core_plugin.py b/envisage/tests/test_core_plugin.py index c2e5a273b..05345277c 100644 --- a/envisage/tests/test_core_plugin.py +++ b/envisage/tests/test_core_plugin.py @@ -16,10 +16,9 @@ from pkg_resources import resource_filename # Enthought library imports. -from envisage.api import Application, Plugin +from envisage.api import Application, CorePlugin, Plugin from envisage.api import ServiceOffer -from traits.api import Interface, List - +from traits.api import HasTraits, Interface, List, on_trait_change, Str # This module's package. PKG = "envisage.tests" @@ -37,8 +36,6 @@ class CorePluginTestCase(unittest.TestCase): def test_service_offers(self): """ service offers """ - from envisage.api import CorePlugin - class IMyService(Interface): pass @@ -81,8 +78,6 @@ def _my_service_factory(self, **properties): def test_dynamically_added_service_offer(self): """ dynamically added service offer """ - from envisage.api import CorePlugin - class IMyService(Interface): pass @@ -137,10 +132,6 @@ def _my_service_factory(self, **properties): def test_preferences(self): """ preferences """ - # The core plugin is the plugin that offers the preferences extension - # point. - from envisage.api import CorePlugin - class PluginA(Plugin): id = "A" preferences = List(contributes_to="envisage.preferences") @@ -162,10 +153,6 @@ def _preferences_default(self): def test_dynamically_added_preferences(self): """ dynamically added preferences """ - # The core plugin is the plugin that offers the preferences extension - # point. - from envisage.api import CorePlugin - class PluginA(Plugin): id = "A" preferences = List(contributes_to="envisage.preferences") @@ -187,3 +174,72 @@ def _preferences_default(self): # Make sure we can get one of the preferences. self.assertEqual("42", application.preferences.get("enthought.test.x")) + + # regression test for enthought/envisage#251 + def test_unregister_service_offer(self): + """ Unregister a service that is contributed to the + "envisage.service_offers" extension point while the application is + running. + """ + + class IJunk(Interface): + trash = Str() + + class Junk(HasTraits): + trash = Str("garbage") + + class PluginA(Plugin): + # The Ids of the extension points that this plugin contributes to. + SERVICE_OFFERS = "envisage.service_offers" + + service_offers = List(contributes_to=SERVICE_OFFERS) + + def _service_offers_default(self): + + a_service_offer = ServiceOffer( + protocol=IJunk, + factory=self._create_junk_service, + ) + + return [a_service_offer] + + def _create_junk_service(self): + """ Factory method for the 'Junk' service. """ + + return Junk() + + @on_trait_change("application:started") + def _unregister_junk_service(self): + # only 1 service is registered so it has service_id of 1 + self.application.unregister_service(1) + + application = TestApplication( + plugins=[CorePlugin(), PluginA()], + ) + + # Run it! + application.run() + + def test_unregister_service(self): + """ Unregister a service which was registered on the application + directly, not through the CorePlugin's extension point. CorePlugin + should not do anything to interfere. """ + + class IJunk(Interface): + trash = Str() + + class Junk(HasTraits): + trash = Str("garbage") + + some_junk = Junk() + + application = TestApplication( + plugins=[CorePlugin()], + ) + + application.start() + + some_junk_id = application.register_service(IJunk, some_junk) + application.unregister_service(some_junk_id) + + application.stop()