feat: move testing side effects to the context manager #2010
Conversation
|
I think the failing tests are related to #1926, so will return to this after solving that. |
| def app_status_history(self) -> list[_EntityStatus]: | ||
| """A record of the app statuses the charm has set. | ||
| Assert that the charm has followed the expected path by checking the app |
There was a problem hiding this comment.
To avoid the doc duplication, should we trim all these docstrings down to just the one-line version, with a second paragraph that says "This field is deprecated, use instead."
| # set by Runtime.exec() in self._run() | ||
| self._output_state: State | None = None | ||
| # set if we're being used to create a context manager (None otherwise) | ||
| self._manager: Manager[CharmType] | None = None |
There was a problem hiding this comment.
I wonder, would it be simpler to create an internal context manager (and always set this) if one isn't being used explicitly? Then instead of keeping two lists for each property (one here and one on Manager) we could just refer to the Manager one in the property definition. It would also avoid duplication elsewhere when you're appending to the lists.
There was a problem hiding this comment.
I wonder, would it be simpler to create an internal context manager (and always set this) if one isn't being used explicitly? Then instead of keeping two lists for each property (one here and one on Manager) we could just refer to the Manager one in the property definition. It would also avoid duplication elsewhere when you're appending to the lists.
We do have to keep two lists, because they have different lifetimes (or we have to have one list and some sort of tracking mechanism to account for that).
It would be nice to avoid the duplication, and always creating the Manager object would allow for that. It felt a bit odd creating one that didn't get used for entering/exiting, but I guess it's just an object and that's fine.
Since they can't be the same underlying object (without some sort of index tracker and making the Manager ones properties as well), there does need to be some sort of duplicate append, but that could be centralised. The deprecation case could be something like:
items = self._unit_status_history.copy()
items.extend(self._manager.unit_status_history)
return itemsAnd just before creating a new Manager object, if there was an existing one, any items could be copied over to the persistent Context attribute.
This would be a bit less CPU efficient if you're using the deprecated form, but there's an easy solution for that, and it's unlikely to be significant I would think. It would be more memory efficient in both cases.
There was a problem hiding this comment.
Ah, thanks for explaining the details. In that case, I think what you have is fine.
| """ | ||
| return Manager(self, event, state) | ||
| mgr = Manager(self, event, state) | ||
| self._manager = mgr |
There was a problem hiding this comment.
Oh, it looks like maybe you're doing that (always creating a Manager) here?
There was a problem hiding this comment.
Oh, it looks like maybe you're doing that (always creating a Manager) here?
Yes, but this is in __call__, which is only used when doing the with ctx(event, state) as mgr form.
| ctx.on.secret_remove(secret, revision=old_revision), | ||
| State(leader=True, secrets={secret}), | ||
| ) | ||
| if source == 'context': |
There was a problem hiding this comment.
When there are parametrization plus large if/else blocks it also makes me wonder, "should this just be two separate test functions?"
Currently, the
testing.Contextobject serves two purposes:It's cleaner to keep these in separate objects. For most tests, the "side effects" are not interesting - these are typically used when testing a specific side effect, such as whether a critical entry was logged, whether the charm went into maintenance state before ending in another state, whether custom traces were emitted, and so on.
This PR keeps the ability to use
ctx.to get the side effects for backwards compatibility, but emits a deprecation warning when that occurs (this will only happen in tests, and is visible in typical pytest output, or could cause tests to fail if pytest is configured to treat deprecation warnings as errors).A new access method is provided for all the "side effects", from the context manager method of running an event. For example:
Fixes #1422