feat: typed testing relation getters#2326
feat: typed testing relation getters#2326james-garner-canonical wants to merge 9 commits intocanonical:mainfrom
Conversation
tonyandrewmeyer
left a comment
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
Currently:
rel = state.get_relation(id)
assert isinstance(rel, testing.Relation)With this PR:
rel = state_get_regular_relation(id)With the other proposal in the ticket:
rel = state.get_relation(id, kind=testing.Relation)Or no helper:
rel = next(iter(r for r in state.relations if r.id ==id and isinstance(r, testing.Relation))I'm not a big fan of expanding the State API surface (it's already pretty large) with 3 or 6 new methods (and I guess this effectively deprecates get_relation?), but all the asserts are annoying, and the kind= overload solution does feel unpythonic. Presumably most of the time that you're asserting on a relation, it does matter which type it is as well, since you want a specific databag that lives in differently named attributes.
So I think I'm on board with the non-plural ones (tempting to officially deprecate get_relation, so that it's effectively only adding two. I'm much less convinced about the plurals.
| return state_relation | ||
| raise KeyError(f'relation: id={relation} not found in the State') | ||
|
|
||
| def get_regular_relation(self, relation: int, /) -> Relation: |
There was a problem hiding this comment.
I don't really love the word "regular here", but I'm also not sure what to suggest instead.
There was a problem hiding this comment.
Yeah I don't love it either. I went with it because the Juju docs on relations basically go Peer, Non-Peer, Subordinate, Non-Subordinate, and then:
A non-subordinate relation (aka ‘regular’) is a non-peer relation where the applications are both principal.
I'm guessing that's why Dima suggested it too. Open to alternatives though.
| r for r in self.relations if _normalise_name(r.endpoint) == normalized_endpoint | ||
| ) | ||
|
|
||
| def get_regular_relations(self, endpoint: str) -> tuple[Relation, ...]: |
There was a problem hiding this comment.
I'm not convinced about adding the plural/endpoint ones. I'm pretty sure when we've looked at use of this in the past, it's almost always code like:
state_in = State(..., relations={relation})
state_out = ctx.run(..., state=state_in)
assert state_out.get_relations('endpoint')[0]...And that is cleaner as:
state_in = State(..., relations={relation})
state_out = ctx.run(..., state=state_in)
assert state_out.get_relation(relation.id)...I had a quick look over a few random selections in charms, and they were all of that type. I'm sure there are exceptions, but this is already adding 3 new methods for getting by ID - I'm not convinced there is enough value to add another 3 as well.
There was a problem hiding this comment.
Makes sense. I'll let @dimaqq weigh in too, but I'm happy to remove the plural ones.
There was a problem hiding this comment.
I suppose my one hesitation would be that getting relations by endpoint name rather than by ID feels more like the right level of expression for state-transition tests. But the current usage argument is pretty compelling.
Co-authored-by: Tony Meyer <tony.meyer@gmail.com>
| def _get_typed_relation(self, relation: int, kind: type[_RelationType]) -> _RelationType: | ||
| rel = self.get_relation(relation) | ||
| if not isinstance(rel, kind): | ||
| raise TypeError( |
There was a problem hiding this comment.
I wonder if this should be:
- a
TypeError(wrong type of relation about to be returned), or - a
ValueError(wrongrelation: intargument passed in)
| for rel in rels: | ||
| if not isinstance(rel, kind): |
There was a problem hiding this comment.
A zero-length return is valid, isn't it?
Then there's a possibility of a false positive, that is not raising an exception when we would have if there was a relation.
|
Overall I'm not opposed to it. |
3 participants
This PR adds more narrowly typed methods alongside
get_relationandget_relations, likeget_subordinate_relationandget_peer_relations. These are typed as returning the correct relation type, rather than justRelationBase, removing the need to narrow the relation type afterwards in tests. If the relation type is incorrect, aTypeErroris raised.Resolves #2243.