Raise better errors in observe_property, and test them. #175
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Barecheck - Code coverage reportTotal: 93.48%Your code coverage diff: 0.15% ▴ Uncovered files and lines
|
Merged
rwb27
force-pushed
the
better-observeproperty-errors
branch
from
baccfff to
ed03371
Compare
julianstirling
requested changes
Aug 19, 2025
julianstirling
approved these changes
Aug 26, 2025
This raises a helpful error if observe_property is called with an argument that isn't a DataProperty. Usually, this will be a functional property, which isn't observable. Currently, tests fail because the websocket doesn't handle errors.
This allows us to test specifically that we're raising the right error when a websocket tries to observe a non-observable property.
This now sends an error response to the websocket in response to an `observeproperty` or `observeaction` message with an invalid or non-observable target. The error is tested for in test_websocket.py.
There was no test for observing something that wasn't a property, or something that wasn't an action. These are added.
I've added a default to the `getattr` call, so that we don't raise `AttributeError` if we attempt to observe something that's not an attribute. Instead, we will raise KeyError, which is the same as if we attempted to observe something that's not a property. I think it makes sense not to distinguish between missing entirely and not-a-property: the client shouldn't be aware if something's a Python attribute that isn't exposed as an affordance. I raise KeyError because I'm thinking the string passed in is being used as a key to look up properties.
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.
I've added actions, and now test all possible status messages.
In response to PR comments I've added more test cases that are not observable properties or actions. These should use the same code path as "non_property".
rwb27
force-pushed
the
better-observeproperty-errors
branch
from
6c0d018 to
8c27f65
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This raises a helpful error if observe_property is called with an argument that isn't a DataProperty. Usually, this will be a functional property, which isn't observable.
To-do before merging:
Closes #169