Rework and improve tests and documentation

I've gotten rid of the previous tests in favour of code that
tests the same things much more cleanly. Changes include:

1. Not using MyThing, instead using a minimal Thing
    defined in the module. This might be moved to a
    common things_to_test module at some point.
2. Using fixtures better, in particular making use of yield
    to ensure everything's cleaned up properly after each test,
    and to avoid the need for tests to call functions that do
    parts of the test (rather than just the set-up).
3. Docstrings to explain what all the tests are doing.

I also found and fixed a bug, where the websocket test for
observing an action hung indefinitely, as we were waiting for
a message that never came.

Frustratingly, there is no timeout option for Starlette test
websocket connections.

Author: rwb27

tests/test_websocket.py

@@ -1,44 +1,24 @@
 from fastapi.testclient import TestClient
-from labthings_fastapi.example_things import MyThing
 import pytest
 import labthings_fastapi as lt
 from labthings_fastapi.exceptions import PropertyNotObservableError
 
 
-@pytest.fixture
-def my_thing():
-    return MyThing()
-
-
-@pytest.fixture
-def server(my_thing):
-    server = lt.ThingServer()
-    server.add_thing(my_thing, "/my_thing")
-    return server
-
-
-def test_websocket_observeproperty(server):
-    with TestClient(server.app) as client:
-        with client.websocket_connect("/my_thing/ws") as ws:
-            ws.send_json(
-                {"messageType": "addPropertyObservation", "data": {"foo": True}}
-            )
-            test_str = "Abcdef"
-            client.put("/my_thing/foo", json=test_str)
-            message = ws.receive_json(mode="text")
-            assert message["data"]["foo"] == test_str
-            ws.close(1000)
+class ThingWithProperties(lt.Thing):
+    """A Thing with various different properties and actions.
 
+    This is used by the earlier tests, ensuring properties may
+    be observed.
+    """
 
-class ThingWithProperties(lt.Thing):
     dataprop: int = lt.property(default=0)
     non_property: int = 0
 
     @lt.property
     def funcprop(self) -> int:
         return 0
 
-    @lt.property
+    @funcprop.setter
     def set_funcprop(self, val: int) -> None:
         pass
 
@@ -48,134 +28,163 @@ def increment_dataprop(self):
         self.dataprop += 1
 
 
-def test_observing_dataprop(mocker):
-    """Check `observe_property` is OK on a data property.
-
-    This doesn't check the observation works, because we don't
-    have an event loop. It just checks the call doesn't raise
-    an error.
-    """
-    thing = ThingWithProperties()
-    thing.observe_property("dataprop", mocker.Mock())
+@pytest.fixture
+def thing():
+    """Instantiate and return a test Thing."""
+    return ThingWithProperties()
 
 
-def test_observing_dataprop_with_ws():
-    """Observe a data property with a websocket, and check it works."""
+@pytest.fixture
+def server(thing):
+    """Create a server, and add a MyThing test Thing to it."""
     server = lt.ThingServer()
-    server.add_thing(ThingWithProperties(), "/thing")
-    with TestClient(server.app) as client:
-        with client.websocket_connect("/thing/ws") as ws:
-            ws.send_json(
-                {"messageType": "addPropertyObservation", "data": {"dataprop": True}}
-            )
-            client.put("/thing/dataprop", json=1)
-            message = ws.receive_json(mode="text")
-            assert message["data"]["dataprop"] == 1
-            ws.close(1000)
+    server.add_thing(thing, "/thing")
+    return server
 
 
-def test_observing_funcprop(mocker):
-    """Check errors are raised if we observe an unsuitable property."""
-    thing = ThingWithProperties()
-    with pytest.raises(PropertyNotObservableError):
-        thing.observe_property("funcprop", mocker.Mock())
+@pytest.fixture
+def client(server):
+    """Yield a TestClient connected to the server."""
+    with TestClient(server.app) as client:
+        yield client
 
 
-def test_observing_funcprop_with_ws():
-    """Try to observe a functional property with a websocket.
+@pytest.fixture
+def ws(client):
+    """Yield a websocket connection to a server hosting a MyThing().
 
-    This should fail: functional properties are not observable.
+    This ensures the websocket is properly closed after the test, and
+    avoids lots of indent levels.
     """
-    server = lt.ThingServer()
-    server.add_thing(ThingWithProperties(), "/thing")
-    with TestClient(server.app) as client:
-        with client.websocket_connect("/thing/ws") as ws:
-            ws.send_json(
-                {"messageType": "addPropertyObservation", "data": {"funcprop": True}}
-            )
-            message = ws.receive_json(mode="text")
-            assert message["error"]["title"] == "Not Observable"
+    with client.websocket_connect("/thing/ws") as ws:
+        try:
+            yield ws
+        finally:
             ws.close(1000)
+            pass
+
 
+def test_observing_dataprop(thing, mocker):
+    """Check `observe_property` is OK on a data property.
 
-def test_observing_missing_prop(mocker):
-    """Check observing a non-existent property raises an error."""
-    thing = ThingWithProperties()
-    with pytest.raises(AttributeError):
-        thing.observe_property("missing_property", mocker.Mock())
+    This doesn't check the observation works, because we don't
+    have an event loop. It just checks the call doesn't raise
+    an error.
+    """
+    thing.observe_property("dataprop", mocker.Mock())
 
 
-def test_observing_not_prop(mocker):
-    """Check observing an attribute that's not a property raises an error."""
-    thing = ThingWithProperties()
-    with pytest.raises(KeyError):
-        thing.observe_property("non_property", mocker.Mock())
+@pytest.mark.parametrize(
+    argnames=["name", "exception"],
+    argvalues=[
+        ("funcprop", PropertyNotObservableError),
+        ("non_property", KeyError),
+        ("increment_dataprop", KeyError),
+        ("missing", KeyError),
+    ],
+)
+def test_observing_errors(thing, mocker, name, exception):
+    """Check errors are raised if we observe an unsuitable property."""
+    with pytest.raises(exception):
+        thing.observe_property(name, mocker.Mock())
 
 
-def test_observing_action(mocker):
+def test_observing_dataprop_with_ws(client, ws):
+    """Observe a data property with a websocket.
+
+    This tests the property's value gets notified when it is
+    set via PUT requests or via an action.
+    """
+    # Observe the property.
+    ws.send_json(
+        {
+            "messageType": "addPropertyObservation",
+            "data": {"dataprop": True},
+        }
+    )
+    for val in [1, 10, 0]:
+        # Set the property's value.
+        client.put("/thing/dataprop", json=val)
+        # Receive the message and check it's as expected.
+        message = ws.receive_json(mode="text")
+        assert message["messageType"] == "propertyStatus"
+        assert message["data"]["dataprop"] == val
+    # Increment the value with an action
+    client.post("/thing/increment_dataprop")
+    message = ws.receive_json(mode="text")
+    assert message["messageType"] == "propertyStatus"
+    assert message["data"]["dataprop"] == 1
+
+
+@pytest.mark.parametrize(
+    argnames=["name", "title", "status"],
+    argvalues=[
+        ("funcprop", "Not Observable", "403"),
+        ("non_property", "Not Found", "404"),
+        ("increment_dataprop", "Not Found", "404"),
+        ("missing", "Not Found", "404"),
+    ],
+)
+def test_observing_dataprop_error_with_ws(ws, name, title, status):
+    """Try to observe a functional/missing property with a websocket.
+
+    This should fail: functional properties are not observable.
+    """
+    # Observe the property.
+    ws.send_json(
+        {
+            "messageType": "addPropertyObservation",
+            "data": {name: True},
+        }
+    )
+    # Receive the message and check for the error.
+    message = ws.receive_json(mode="text")
+    assert message["error"]["title"] == title
+    assert message["error"]["status"] == status
+
+
+def test_observing_action(thing, mocker):
     """Check observing an action is successful."""
-    thing = ThingWithProperties()
     thing.observe_action("increment_dataprop", mocker.Mock())
 
 
-def test_observing_not_action(mocker):
+def test_observing_action_error(thing, mocker):
     """Check observing an attribute that's not an action raises an error."""
-    thing = ThingWithProperties()
     with pytest.raises(KeyError):
         thing.observe_action("non_property", mocker.Mock())
 
 
-def test_websocket_observeproperty_counter(server):
-    with TestClient(server.app) as client:
-        with client.websocket_connect("/my_thing/ws") as ws:
-            ws.send_json(
-                {"messageType": "addPropertyObservation", "data": {"counter": True}}
-            )
-            # Trigger the increment_counter action to change the counter value
-            client.post("/my_thing/increment_counter")
-
-            # Receive the message from the WebSocket
-            message = ws.receive_json(mode="text")
-            assert (
-                message["data"]["counter"] == 1
-            )  # Expect the counter to be 1 after increment
-            ws.close(1000)
-            my_thing.counter = 0  # Set counter back to 0
-
-
-def handle_websocket_messages(message):
-    if (
-        message["messageType"] == "actionStatus"
-        and message["data"]["status"] == "completed"
-    ):
-        return True
-    return False
-
-
-def test_websocket_observeaction(server, my_thing):
-    with TestClient(server.app) as client:
-        with client.websocket_connect("/my_thing/ws") as ws:
-            ws.send_json(
-                {"messageType": "addPropertyObservation", "data": {"counter": True}}
-            )
-            ws.send_json(
-                {
-                    "messageType": "addActionObservation",
-                    "data": {"slowly_increase_counter": True},
-                }
-            )
-
-            # Trigger the slowly_increase_counter action
-            client.post("/my_thing/slowly_increase_counter", json={"delay": 0})
-
-            # Listen for WebSocket messages and check for the completed action
-            action_completed = False
-            while not action_completed:
-                message = ws.receive_json()
-                print(f"Received message: {message}")
-
-                action_completed = handle_websocket_messages(message)
-                if action_completed:
-                    assert my_thing.counter == 60
-
-            my_thing.counter = 0  # Set counter back to 0
+def test_observing_action_with_ws(client, ws):
+    """Observe an action with a websocket, checking the status changes correctly."""
+    # Observe the property.
+    ws.send_json(
+        {
+            "messageType": "addActionObservation",
+            "data": {"increment_dataprop": True},
+        }
+    )
+    # Invoke the action (via HTTP)
+    client.post("/thing/increment_dataprop")
+    # We should see the status go through the expected sequence
+    for expected_status in ["pending", "running", "completed"]:
+        message = ws.receive_json(mode="text")
+        assert message["messageType"] == "actionStatus"
+        assert message["data"]["status"] == expected_status
+
+
+def test_observing_action_error_with_ws(ws):
+    """Try to observe something that's not an action, as an action.
+
+    This should fail: observeAction should only work on actions.
+    """
+    # Observe the property.
+    ws.send_json(
+        {
+            "messageType": "addActionObservation",
+            "data": {"non_property": True},
+        }
+    )
+    # Receive the message and check for the error.
+    message = ws.receive_json(mode="text")
+    assert message["error"]["title"] == "Not Found"
+    assert message["error"]["status"] == "404"