feat: add tests #25
feat: add tests #25
Conversation
There was a problem hiding this comment.
I think the remaining TODO can wait a bit. I don't expect the upcoming planned features to change those code paths, so in the interest of time and shipping those feature, I think we can plan to deal with the TODO when moving to datagouv-client.
abulte
left a comment
abulte
left a comment
There was a problem hiding this comment.
Feels like we're covered 🏰
| def as_dict(self) -> dict[str, Any]: | ||
| return { | ||
| "id": self.id, | ||
| "slug": self.slug, | ||
| "name": self.name, | ||
| } |
There was a problem hiding this comment.
Could be done through __dict__?
There was a problem hiding this comment.
What do you have in mind?
I don't think you're supposed to play with __dict__ directly (quick web search suggests we shouldn't). Dataclasses having an asdict function is a pretty strong hint IMO.
| "organization": self.organization.as_dict(), | ||
| } | ||
|
|
||
| def with_owner(self, organization: Organization) -> Self: |
There was a problem hiding this comment.
Confusing if we ever introduce the owner for data.gouv.fr
There was a problem hiding this comment.
My plan was to later extend to with_owner(self, owner: User | Organization) or similar, so we don't have two different methods for attributing ownership.
Datagouv uses "owner" for both "general ownership" (e.g. the Owned mixin) and "individual owner" (i.e. a user, as opposed to an org), so I figured I'd try to use different terms. But you're right that "owner" does mean "user" in the API responses, so maybe we're stuck with that definition and should use something else for my notion of "individual or organizational owner".
Some other ideas: with_ownership, with_proprietor, ...? Any better idea?
tests/datagouv_mocks.py
Outdated
| return cls().with_owner(organization) | ||
|
|
||
| @classmethod | ||
| def some_owned(cls, n: int, organizations: list[Organization]) -> list[Self]: |
There was a problem hiding this comment.
| def some_owned(cls, n: int, organizations: list[Organization]) -> list[Self]: | |
| def many_owned(cls, n: int, organizations: list[Organization]) -> list[Self]: |
Or I expect at least one of them to not be owned.
There was a problem hiding this comment.
I reworked those builder methods, so now it's one/some for all types. Does this make things clearer?
See also #25 (comment) about sticking with "some".
tests/conftest.py
Outdated
| # TODO: support reset=True | ||
|
|
||
| for element_class in ElementClass: | ||
| target_elements = target_universe.elements(element_class) |
There was a problem hiding this comment.
Elements is a but confusing (objects ids in test code), but since it stems from a universe, we can probably keep it like this.
There was a problem hiding this comment.
I'm not sure I understand what you mean. There are elements (of class Element) and object ids, and an element as a reference to an object id. I.e.:
upcoming_elements = upcoming_universe.elements(element_class)
upcoming_object_ids = {e.object.id for e in upcoming_elements}
What test code are you referring to?
tests/datagouv_mocks.py
Outdated
| return cls() | ||
|
|
||
| @classmethod | ||
| def some(cls, n: int) -> list[Self]: |
There was a problem hiding this comment.
| def some(cls, n: int) -> list[Self]: | |
| def many(cls, n: int) -> list[Self]: |
There was a problem hiding this comment.
I like some better, because "many" suggests "a lot" and "several" suggests "at least two", and here n can take any value (didn't see a reason to enforce a floor value).
Does it matter now that #25 (comment) is addressed?
| existing_universe.clone() | ||
| .add_elements(datasets[2], dataservices[1]) | ||
| .remove_elements(datasets[1]) |
There was a problem hiding this comment.
Right? :)
I'm not entirely sure I should use "elements" here, as it could be confused with instances of class Element (is that what you meant as "test code" in #25 (comment)?). But we do talk about datasets & co as topic "elements". I feel there's still a lot of naming confusion around topics, and we'd make discussion easier by clearing up the mess...
Since I renamed the datagouv object type superclasses, maybe {add,remove}_records would be clearer? (I'm not 100% sure "record" is the perfect term, but at least we'll be consistent)
There was a problem hiding this comment.
It feels non-standard for the bulk of the real testing (ie assertions) to happen in the fixture rather than in the tests themselves. But I guess we don't have a simple way to move the assertions here?
There was a problem hiding this comment.
Feels the same to me, but I don't see how I can work around the fact that responses constructs bundle mocks and assertions together. Moving the few direct asserts (those testing the organizations-*.json files) IMO would make it even less clear since you'd have those in the main test (to be redefined each time) but the responses-related asserts would be hidden.
So I don't see how we could have the usual clear-cut test sequence of 1) declare objects and mocks 2) call function under test 3) declare asserts.
Maybe renaming run_mock_feed to something like mock_feed_and_assert or can help? I initially wanted to use test_feed but that would be too confusing with all the test_ functions.
Prep work for opendatateam/udata-front-kit#961.
Tagging as "feat" since we have some misc changes on the main path (sorting universe additions and removals). I don't expect it'll matter, but you never know 🐛
I'll make a follow-up PR to refactor the datagouv API to return actual objects instead of dicts so we can have tighter typing (and prepare for datagouv-client, see below). Then we should be done with the prep work for opendatateam/udata-front-kit#961 and ecolabdata/ecospheres#802.
Once all this lands I'll look into replacing our homemade datagouv API wrapper with datagouv-client, and hopefully move the datagouv mocks over there. So if you have suggestions on improving the mocks, we can start the discussion here :)