Server instantiates things #183
Open
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
In order to improve the Thing lifecycle, we're going to move to having the server instantiate the Thing. This will allow us to inject arguments to `__init__` that simplify the set-up process. This commit changes `add_thing` to accept a class rather than an object, and adds `args` and `kwargs` parameters to enable values to be passed to `__init__`. This commit swaps the `path` for a `name` (in all known cases this simply removes the leading `/` from the string) in preparation for making a distinction between names and paths. This commit does not change `Thing` at all: that will come later. It also doesn't change the test code - that will touch many more files!
This should help catch code that called the old version of add_thing more easily. Also, we no longer want `/` in the thing names, so I've removed that tfrom the regex.
For now, DirectThingClient should locate Things by name. It will be removed in due course, once thing connections are implemented.
I've taken this opportunity to move nearly all of the server set-up into fixtures. It would be nice to deduplicate this code in the future, but I think it's already neater than it was. Some tests use things that are not connected to a server. These will require mocking in the future.
This updates the code that's included as separate python files. I will also do a search for `.add_thing` and update docstrings.
I can't believe there are only two mentions - but these are the only two I can find so far.
This now consistently defines the arguments and return type.
There were #noqa directives left over from when we were double-checking `D` codes with flake8 and ruff. Ruff works better, so the #noqa is no longer needed.
Each Thing is now provided with a ThingServerInterface which allows it to interact with the server. The ThingServerInterface is specific to each Thing, and is how the Thing obtains its name and path. This removes the need to pass around the BlockingPortal: the ThingServerInterface provides a method to call async code in the event loop on the server. I hope this is clearer. The ThingServerInterface takes care of making sure we're connected to a server: I've therefore been able to remove a lot of error-handling code and tests for NotConnectedToServerError. This will need to be tested for explicitly, on the ThingServerInterface.
This came to light during testing of the new ThingServerInterface.. There's no reason errors setting up the logger should be swallowed by a catch-all exception: errors there are a bug in LabThings. Also, setting up the logger after emitting an event meant that an exception raised there caused another exception because of the missing logger. I have moved the creation of the logger out of the `try:` block to fix both these issues.
I now reference ThingServerInterface rather than BlockingPortal when describing how to run async code in the event loop.
We no longer need `get_blocking_portal` and can use `ThingServerInterface` instead. This change in `MJPEGStreamDescriptor` will require code that adds frames to delete an argument.
Some tests (specifically those that spin up an HTTP server) are much slower than the rest. I have marked these as "slow" so the test suite may be run with `-m "not slow"` when repeatedly re-testing. Obviously we should run all the tests in CI, this is a convenience feature for development.
This removes the blocking portal argument, that's no longer needed.
Now that we require a ThingServerInterface to create a Thing, I have provided a convenience test harness, `create_thing_without_server()` to consistently and cleanly allow `Thing` subclasses to be instantiated in test code. This auto-generates a name and path for the Thing, and mocks the required functionality. I have removed some tests that were checking for conditions where the Thing was not connected. Things are now created in a connected state, so this isn't needed any more. I should add tests for ThingServerInterface, particularly to make sure that it raises sensible errors when the event loop is not running. That's for a future commit.
This is a full, bottom-up set of tests for ThingServerInterface, as well as the associted mock/test harness code.
I've added a couple of ignores, for thing instantiation. Given that this is very much dynamically typed code, I don't think it loses anything.
This change makes it possible to load settings during __init__, though it does not yet do so. As the settings path is always available, we can remove some error-checking code that handled the period between the Thing being created and it having a settings path to use: this period no longer exists.
I had the same logic in `ThingServer.path_for_thing` and also `ThingServerInterface.path`. I've now called the former from the latter. I've then duplicated the logic again in `MockThingServerInterface` but I am less concerned about that, as duplication in a mock is not such a problem. There is a test to check the mock server interface generates a sensible path, and a test that asking for the path of a Thing that's not on the server raises an error.
| ThingSubclass = TypeVar("ThingSubclass", bound="Thing") | ||
|
|
||
|
|
||
| def create_thing_without_server( |
There was a problem hiding this comment.
I would add an option to be able to set the settings path; as sometimes it is useful to be able to test how a Thing would respond when loading settings. For instance we need to do this in the hardware tests when we check that the exposure doesn't drift when we save settings and reload.
There was a problem hiding this comment.
Adding that option should not be a problem. Might need to tweak the mock class too, but again that's fairly simple. I've started the next phase of this MR series so won't do it right now, but this is absolutely something we can put in before we test against OpenFlexure.
2 tasks
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 PR changes the way Things are created: instead of passing an instance of a
Thingto the server, we pass the class and the arguments for__init__. This allows theThingServerto pass a parameter toThing.__init__which means theThingstarts up already attached to a server.Summary of changes
Things are added to the server by passing a class, i.e.server.add_thing("name", MyThing).Things are created for testing withcreate_thing_without_server(MyThing)which mocks the required resources.Things are referred to by name, not path. The name does not contain slashes, i.e./mythingbecomesmything.BlockingPortaldependency: it's replaced byself._thing_server_interface.start_async_task_soon()GetThingStatesdependency, it's replaced byself._thing_server_interface.get_thing_states().More details
The consequence of this is that
Thing._thing_server_interfaceallows key functions of the server to be accessed without using dependencies, in particular it allows metadata to be retrieved (removing the need forGetThingStates) and it allows async code to be run in the event loop (removing the need to pass around theBlockingPortal). It also centralises error handling logic relating to checking the event loop is running, which deduplicates code.As well as eliminating some dependencies, this change also simplifies the handling of settings: specifically, it means that a settings path is available as soon as
Thing.__init__has run, i.e. settings could, in principle, be used during initialisation. Currently, they are not loaded untilattach_to_serveris called, but this could be changed in a future tidy-up.Previously,
Thing._setting_storage_pathwas doing double duty as a way to disable saving settings during loading. This isn't possible if we use theThingServerInterfaceto find the path, so I've instead added a new attribute,_disable_saving_settingsto do that. Given that none of the tests failed when that functionality was broken, we may need to improve the testing and/or test carefully against the OpenFlexure codebase.This commit touches a lot of files, because all the test code has to use the new syntax for creating
Things and adding them to the server. Most of these changes are fairly trivial, but I've taken the opportunity to tidy up the tests a little, and make better use of fixtures.This PR is the first step to addressing #182. I anticipate merging it into a working branch, and stacking up a few PRs before doing a thorough test against the OpenFlexure server. However, if anyone is minded to review it, I think it's complete enough to merit that. If review comments appear after I've started work on the next phase (eliminating
DirectThingClient), it may make sense to fix them in a future MR on my working branch, rather than have to do lots of rebasing here.Incidental fixes
I did fix a couple of minor things as I went along. These could be split into pull requests on their own, but as they are pretty self-contained I have put them in their own commits in this PR. I've put the first line of commit messages here, more details are available in the commit message.
Mark slow tests to speed up repeated testing.adds@pytest.mark.slowto allow slow tests to be easily skipped.Create invocation loggers before starting actions.moves a few lines so that there's not a cascade of errors if something goes wrong invoking an action.Remove vestigial #noqa statementsdeletes some linter directives that are no longer needed.