Repository: enthought/traits-futures

Issue:
Check test coverage
0.1.0 had 100% test coverage, but a lot of code has moved around since then. We should check the coverage before the 0.2.0 release (partly just to check for any pieces of code that became unused but weren't deleted).

Discussion:
rahulporuri: I might not be able to review this before EoD today. Is that acceptable?
mdickinson: > I might not be able to review this before EoD today. Is that acceptable?

Yes, of course!
rahulporuri: Still LGTM
mdickinson: > Still LGTM

Thanks. (Still) waiting for Appveyor ...

Patch:
diff --git a/docs/source/guide/advanced.rst b/docs/source/guide/advanced.rst
index c6f9405..cbea979 100644
--- a/docs/source/guide/advanced.rst
+++ b/docs/source/guide/advanced.rst
@@ -163,7 +163,7 @@ background task type:
    substitutions
 
 .. |BaseFuture| replace:: :class:`~.BaseFuture`
-.. |dispatch_message| replace:: :meth:`~.BaseFuture.dispatch_message`
+.. |dispatch_message| replace:: :meth:`~.BaseFuture._dispatch_message`
 .. |exception| replace:: :attr:`~traits_futures.i_future.IFuture.exception`
 .. |HasStrictTraits| replace:: :class:`~traits.has_traits.HasStrictTraits`
 .. |IFuture| replace:: :class:`~.IFuture`
diff --git a/traits_futures/base_future.py b/traits_futures/base_future.py
index 6a44e4a..8eb52f7 100644
--- a/traits_futures/base_future.py
+++ b/traits_futures/base_future.py
@@ -17,9 +17,7 @@ from traits.api import (
     Bool,
     Callable,
     Enum,
-    Event,
     HasStrictTraits,
-    on_trait_change,
     Property,
     provides,
     Str,
@@ -116,12 +114,6 @@ class BaseFuture(HasStrictTraits):
     #: it will be consistent with the ``state``.
     done = Property(Bool())
 
-    #: Event trait providing custom messages from the background task.
-    #: Subclasses of ``BaseFuture`` can listen to this trait and interpret
-    #: the messages in whatever way they like. Each message takes the
-    #: form ``(message_type, message_args)``.
-    message = Event(Tuple(Str(), Any()))
-
     @property
     def result(self):
         """
@@ -142,11 +134,11 @@ class BaseFuture(HasStrictTraits):
             If the task is still executing, or was cancelled, or raised an
             exception instead of returning a result.
         """
-        if self._state != COMPLETED:
+        if self.state != COMPLETED:
             raise AttributeError(
                 "No result available. Task has not yet completed, "
                 "or was cancelled, or failed with an exception. "
-                "Task state is {}".format(self._state)
+                "Task state is {}".format(self.state)
             )
         return self._result
 
@@ -171,7 +163,7 @@ class BaseFuture(HasStrictTraits):
             If the task is still executing, or was cancelled, or completed
             without raising an exception.
         """
-        if self._state != FAILED:
+        if self.state != FAILED:
             raise AttributeError(
                 "No exception information available. Task has "
                 "not yet completed, or was cancelled, or completed "
@@ -199,11 +191,15 @@ class BaseFuture(HasStrictTraits):
                 "Can only cancel a waiting or executing task. "
                 "Task state is {}".format(self.state)
             )
-        self._cancel()
         self._user_cancelled()
 
-    @on_trait_change("message")
-    def dispatch_message(self, message):
+    # Semi-private methods ####################################################
+
+    # These methods represent the state transitions in response to external
+    # events. They're used by the FutureWrapper, but are not intended for use
+    # by the users of Traits Futures.
+
+    def _dispatch_message(self, message):
         """
         Automate dispatch of different types of message.
 
@@ -216,7 +212,14 @@ class BaseFuture(HasStrictTraits):
 
         If the future is already in ``CANCELLING`` state, no message is
         dispatched.
+
+        Parameters
+        ----------
+        message : tuple(str, object)
+            Message from the background task, in the form (message_type,
+            message_args).
         """
+
         if self._state == CANCELLING_AFTER_STARTED:
             # Ignore messages that arrive after a cancellation request.
             return
@@ -229,12 +232,6 @@ class BaseFuture(HasStrictTraits):
                 "Unexpected custom message in state {!r}".format(self._state)
             )
 
-    # Semi-private methods ####################################################
-
-    # These methods represent the state transitions in response to external
-    # events. They're used by the FutureWrapper, but are not intended for use
-    # by the users of Traits Futures.
-
     def _task_started(self, none):
         """
         Update state when the background task has started processing.
@@ -290,9 +287,11 @@ class BaseFuture(HasStrictTraits):
         state.
         """
         if self._state == INITIALIZED:
+            self._cancel()
             self._cancel = None
             self._state = CANCELLING_BEFORE_STARTED
         elif self._state == EXECUTING:
+            self._cancel()
             self._cancel = None
             self._state = CANCELLING_AFTER_STARTED
         else:
diff --git a/traits_futures/i_parallel_context.py b/traits_futures/i_parallel_context.py
index 81a5a7a..7b5da62 100644
--- a/traits_futures/i_parallel_context.py
+++ b/traits_futures/i_parallel_context.py
@@ -57,18 +57,6 @@ class IParallelContext(abc.ABC):
             the ``set`` and ``is_set`` methods from that API.
         """
 
-    @abc.abstractmethod
-    def queue(self):
-        """
-        Return a shareable queue suitable for this context.
-
-        Returns
-        -------
-        queue : queue-like
-            A queue that can be shared safely with workers. This package
-            relies only on the ``put`` and ``get`` methods of the queue.
-        """
-
     @abc.abstractmethod
     def message_router(self):
         """
diff --git a/traits_futures/multithreading_context.py b/traits_futures/multithreading_context.py
index e2ad4e8..b1e2936 100644
--- a/traits_futures/multithreading_context.py
+++ b/traits_futures/multithreading_context.py
@@ -13,7 +13,6 @@ Context providing multithreading-friendly worker pools, events, and routers.
 """
 
 import concurrent.futures
-import queue
 import threading
 
 from traits_futures.i_parallel_context import IParallelContext
@@ -55,17 +54,6 @@ class MultithreadingContext(IParallelContext):
         """
         return threading.Event()
 
-    def queue(self):
-        """
-        Return a shareable queue suitable for this context.
-
-        Returns
-        -------
-        queue : queue-like
-            A queue that can be shared safely with workers.
-        """
-        return queue.Queue()
-
     def message_router(self):
         """
         Return a message router suitable for use in this context.
diff --git a/traits_futures/wrappers.py b/traits_futures/wrappers.py
index 7f30227..72f927b 100644
--- a/traits_futures/wrappers.py
+++ b/traits_futures/wrappers.py
@@ -72,15 +72,12 @@ class FutureWrapper(HasStrictTraits):
         message_kind, message = message
 
         if message_kind == CUSTOM:
-            self.future.message = message
-        elif message_kind == CONTROL:
+            self.future._dispatch_message(message)
+        else:
+            assert message_kind == CONTROL
             message_type, message_arg = message
             method_name = "_task_{}".format(message_type)
             getattr(self.future, method_name)(message_arg)
-        else:
-            raise RuntimeError(
-                "Unrecognised message kind: {}".format(message_kind)
-            )
 
 
 class BackgroundTaskWrapper:


Test patch:
diff --git a/traits_futures/tests/common_future_tests.py b/traits_futures/tests/common_future_tests.py
index ebdd577..66f28e5 100644
--- a/traits_futures/tests/common_future_tests.py
+++ b/traits_futures/tests/common_future_tests.py
@@ -19,6 +19,12 @@ from traits_futures.exception_handling import marshal_exception
 from traits_futures.future_states import CANCELLABLE_STATES, DONE_STATES
 
 
+def dummy_cancel_callback():
+    """
+    Dummy callback for cancellation, that does nothing.
+    """
+
+
 class FutureListener(HasStrictTraits):
     """ Record state changes to a given future. """
 
@@ -55,7 +61,7 @@ class CommonFutureTests:
 
         # Record state when any of the three traits changes.
         future = self.future_class()
-        future._executor_initialized(lambda: None)
+        future._executor_initialized(dummy_cancel_callback)
 
         future.on_trait_change(record_states, "cancellable")
         future.on_trait_change(record_states, "done")
@@ -74,7 +80,7 @@ class CommonFutureTests:
 
     def test_cancellable_and_done_success(self):
         future = self.future_class()
-        future._executor_initialized(lambda: None)
+        future._executor_initialized(dummy_cancel_callback)
 
         listener = FutureListener(future=future)
 
@@ -86,7 +92,7 @@ class CommonFutureTests:
 
     def test_cancellable_and_done_failure(self):
         future = self.future_class()
-        future._executor_initialized(lambda: None)
+        future._executor_initialized(dummy_cancel_callback)
 
         listener = FutureListener(future=future)
 
@@ -98,7 +104,7 @@ class CommonFutureTests:
 
     def test_cancellable_and_done_cancellation(self):
         future = self.future_class()
-        future._executor_initialized(lambda: None)
+        future._executor_initialized(dummy_cancel_callback)
 
         listener = FutureListener(future=future)
 
@@ -111,7 +117,7 @@ class CommonFutureTests:
 
     def test_cancellable_and_done_early_cancellation(self):
         future = self.future_class()
-        future._executor_initialized(lambda: None)
+        future._executor_initialized(dummy_cancel_callback)
 
         listener = FutureListener(future=future)
 
@@ -127,20 +133,24 @@ class CommonFutureTests:
     # The BaseFuture processes four different messages: started / raised /
     # returned messages from the task, and a possible cancellation message from
     # the user. We denote these with the letters S, X (for eXception), R and C,
-    # and add machinery to test various combinations.
+    # and add machinery to test various combinations. We also write I to
+    # denote initialization of the future.
 
     def test_invalid_message_sequences(self):
-        # A complete run must always involve "started, raised" or "started,
-        # returned" in that order. In addition, a single cancellation is
-        # possible at any time before the end of the sequence.
+        # A future must always be initialized before anything else happens, and
+        # then a complete run must always involve "started, raised" or
+        # "started, returned" in that order. In addition, a single cancellation
+        # is possible at any time before the end of the sequence.
         complete_valid_sequences = {
-            "SR",
-            "SX",
-            "CSR",
-            "CSX",
-            "SCR",
-            "SCX",
+            "ISR",
+            "ISX",
+            "ICSR",
+            "ICSX",
+            "ISCR",
+            "ISCX",
         }
+
+        # Systematically generate invalid sequences of messages.
         valid_initial_sequences = {
             seq[:i]
             for seq in complete_valid_sequences
@@ -150,30 +160,39 @@ class CommonFutureTests:
             seq[:i] + msg
             for seq in valid_initial_sequences
             for i in range(len(seq) + 1)
-            for msg in "CRSX"
+            for msg in "ICRSX"
         }
         invalid_sequences = continuations - valid_initial_sequences
+
+        # Check that all invalid sequences raise StateTransitionError
         for sequence in invalid_sequences:
             with self.subTest(sequence=sequence):
                 with self.assertRaises(StateTransitionError):
                     self.send_message_sequence(sequence)
 
+        # Check all complete valid sequences.
+        for sequence in complete_valid_sequences:
+            with self.subTest(sequence=sequence):
+                future = self.send_message_sequence(sequence)
+                self.assertTrue(future.done)
+
     def test_interface(self):
         future = self.future_class()
         self.assertIsInstance(future, IFuture)
 
     def send_message(self, future, message):
         """ Send a particular message to a future. """
-        if message == "S":
+        if message == "I":
+            future._executor_initialized(dummy_cancel_callback)
+        elif message == "S":
             future._task_started(None)
         elif message == "X":
             future._task_raised(self.fake_exception())
         elif message == "R":
             future._task_returned(23)
-        elif message == "C":
-            future._user_cancelled()
         else:
-            raise ValueError("Invalid message: {}".format(message))
+            assert message == "C"
+            future._user_cancelled()
 
     def send_message_sequence(self, messages):
         """ Create a new future, and send the given message sequence to it. """
diff --git a/traits_futures/tests/test_base_future.py b/traits_futures/tests/test_base_future.py
new file mode 100644
index 0000000..9cde384
--- /dev/null
+++ b/traits_futures/tests/test_base_future.py
@@ -0,0 +1,102 @@
+# (C) Copyright 2018-2020 Enthought, Inc., Austin, TX
+# All rights reserved.
+#
+# This software is provided without warranty under the terms of the BSD
+# license included in LICENSE.txt and may be redistributed only under
+# the conditions described in the aforementioned license. The license
+# is also available online at http://www.enthought.com/licenses/BSD.txt
+#
+# Thanks for using Enthought open source!
+
+"""
+Tests for ability to subclass the BaseFuture base class.
+"""
+
+import unittest
+
+from traits.api import Int, List
+
+from traits_futures.base_future import BaseFuture, StateTransitionError
+from traits_futures.tests.common_future_tests import CommonFutureTests
+
+
+def dummy_cancel_callback():
+    """
+    Dummy callback for cancellation, that does nothing.
+    """
+
+
+class PingFuture(BaseFuture):
+    """
+    BaseFuture subclass that interpretes "ping"
+    messages from the background.
+    """
+    #: Accumulate ping messages
+    pings = List(Int)
+
+    def _process_ping(self, arg):
+        """
+        Process a 'ping' message.
+        """
+        self.pings.append(arg)
+
+
+class TestBaseFuture(CommonFutureTests, unittest.TestCase):
+    def setUp(self):
+        self.future_class = PingFuture
+
+    def test_normal_lifecycle(self):
+        future = self.future_class()
+        future._executor_initialized(dummy_cancel_callback)
+        future._task_started(None)
+        future._dispatch_message(("ping", 123))
+        future._dispatch_message(("ping", 999))
+        future._task_returned(1729)
+
+        self.assertEqual(future.pings, [123, 999])
+
+    def test_ping_after_cancellation_is_ignored(self):
+        message = ("ping", 32)
+
+        future = self.future_class()
+        future._executor_initialized(dummy_cancel_callback)
+
+        future._task_started(None)
+        future._user_cancelled()
+
+        future._dispatch_message(message)
+        future._task_returned(1729)
+
+        self.assertEqual(future.pings, [])
+
+    def test_impossible_ping(self):
+        # Custom messages should only ever arrive when a future is
+        # in EXECUTING or CANCELLING states.
+        message = ("ping", 32)
+
+        future = self.future_class()
+
+        with self.assertRaises(StateTransitionError):
+            future._dispatch_message(message)
+
+        future._executor_initialized(dummy_cancel_callback)
+
+        with self.assertRaises(StateTransitionError):
+            future._dispatch_message(message)
+
+        future._task_started(None)
+        future._task_returned(1729)
+
+        with self.assertRaises(StateTransitionError):
+            future._dispatch_message(message)
+
+    def test_impossible_ping_cancelled_task(self):
+        message = ("ping", 32)
+
+        future = self.future_class()
+        future._executor_initialized(dummy_cancel_callback)
+
+        future._user_cancelled()
+
+        with self.assertRaises(StateTransitionError):
+            future._dispatch_message(message)
diff --git a/traits_futures/tests/test_traits_executor.py b/traits_futures/tests/test_traits_executor.py
index 6a86ce6..a0dd162 100644
--- a/traits_futures/tests/test_traits_executor.py
+++ b/traits_futures/tests/test_traits_executor.py
@@ -53,10 +53,6 @@ class TestTraitsExecutorCreation(GuiTestAssistant, unittest.TestCase):
         self._context = MultithreadingContext()
 
     def tearDown(self):
-        if hasattr(self, "executor"):
-            self.executor.stop()
-            self.wait_until_stopped(self.executor)
-            del self.executor
         self._context.close()
         GuiTestAssistant.tearDown(self)