Refactor requests out to a private function
Opened this issue · 3 comments
Thinking about my missing one of the required changes to headers changes in #84 which meant get_events()
didnn't authenticate...
I know it's difficult for this kind of library to run e.g. a proper fully-automated test suite on push/merge, as it depends on user authentication, but maybe a first step would be to have a standard script which can be run in local dev environment, includes logging and covers as much of the functionality as possible? (e.g. get names of all groups; use one of the group IDs to get one group; get a few events; use one of the group IDs to get one group)
I am kind of putting some of this together anyway for my own assurance, but I don't really use chat or write events, so it's not going to be complete... but I think I'll pull something together and comment what should be added.
(I know it's possible to unit test some of the behaviour, by e.g. mocking responses, but I think that would come later, as probably needs refactoring first.)
I agree.
Some kind of testing would be good.
In this specific case, I think another small refactor should be done, that moves all the requests out to a private function, to ensure they are all doing the same.
Right now, there are multiple variations, for example:
Group uses the auth_headers, sets a self.variable and returns it
url = f"{self.api_url}groups/"
async with self.clientsession.get(url, headers=self.auth_headers) as r:
self.groups = await r.json()
return self.groups
Chat uses some private headers and returns the await r.json()
url = f"{self.chat_url}/chats/?max=10"
headers = {"auth": self.auth}
async with self.clientsession.get(url, headers=headers) as r:
return await r.json()
Events does the same as Groups
async with self.clientsession.get(url, headers=self.auth_headers) as r:
self.events = await r.json()
return self.events
UpdateEvents Uses its own headers, but returns self.events
(and seems to be missing the auth-headers)
headers = {"content-type": "application/json;charset=utf-8"}
async with self.clientsession.post(url, json=data, headers=headers) as r:
self.events_update = await r.json()
return self.events
I feel that these should probably all use a common _post()
and _get()
and be consistent in what they return and how they behave...
something like
def _get(url):
async with self.clientsession.get(url, headers=self.auth_headers) as r:
try:
result = await r.json()
except Whatever:
Handle exceptions
return None (or an empty dict?)
return result
So the other functions can simply do
self.events = self._get(url):
return self.events
And ensure that the headers contain all the necessary parameters, charset in the content-type etc.
I guess the inconsistency in headers
vs auth_headers
proves my point - I simply got distracted after making the amends that I had confidence I could manually test, and then forgot to come back and fix the rest... 🤨
The refactor would prevent that in future, allow unit testing of a key component and resolve a lot of the inconsistencies (a bit of a chicken and egg problem as what should be returned isn't always documented).
But as a private function, I don't think it shouldn't have any effect on (or block writing of) an end-to-end manually reviewed test of output from the public functions - I missed out that key word.
The original issue ("Add some form of standard test for assurance when making changes") is resolved. But have kept + renamed, as the refactor proposal remains.