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

@@ -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)

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 = []

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()