Some unitttest for when APIs fail. #70
Conversation
There was a problem hiding this comment.
Your commit message is all on one line, making it hard to read, try making a header and a body, there are some great tips here: https://cbea.ms/git-commit/ (have a look at the "seven rules")
|
I promise to do better in the future! |
marvin_actions.py
Outdated
| @@ -579,5 +579,7 @@ def marvinCommit(row): | |||
| msg = None | |||
| if any(r in row for r in ["commit", "-m"]): | |||
| commitMsg = getCommit() | |||
| if commitMsg == "Du får komma på ett själv. Jag är trasig för tillfället!": | |||
There was a problem hiding this comment.
maybe we could fix getCommit() so that it returns the right value to begin with?
There was a problem hiding this comment.
Something like this?
def getCommit():
"""
Retrieves random commit message from whatthecommit.com/index.html
"""
try:
url = getString("commit", "url")
r = requests.get(url, timeout=5)
res = r.text.strip()
msg = "Använd detta meddelandet: '{}'".format(res)
return msg
except Exception:
return getString("commit", "error")
def marvinCommit(row):
"""
Display a random commit message
"""
msg = None
if any(r in row for r in ["commit", "-m"]):
commitMsg = getCommit()
return commitMsg
There was a problem hiding this comment.
yes, this is good. if we were going to reuse getCommit() somewhere, we might want to have it only return the commit message, or throw an exception, and do the string formatting in marvinCommit(). That way getCommit() acts more like an API, and marvinCommit() defines the interaction with marvin. but i don't think that will ever happen, so we don't need to go that far.
test_marvin_actions.py
Outdated
| def testVideoOfTodayFail(self): | ||
| """Tests that marvin says the right thing when he doesn't have a video for the day""" | ||
| with mock.patch("marvin_actions.getString") as gs: | ||
| gs.return_value = "" |
There was a problem hiding this comment.
will getString return an empty string if the key doesn't exist?
There was a problem hiding this comment.
Nope, it throws a KeyError. Maybe the code should be wrapped in a try/catch block?
There was a problem hiding this comment.
i mostly wanted the tests to test the actual behavior, rather than force execution of all code branches
test_marvin_actions.py
Outdated
| """Tests that marvin says the right thing when he doesn't have a video for the day""" | ||
| with mock.patch("marvin_actions.getString") as gs: | ||
| gs.return_value = "" | ||
| response = " Jag har ännu ingen passande video för denna dagen." |
There was a problem hiding this comment.
should the leading space really be there?
There was a problem hiding this comment.
The returned string has a leading space, for some reason.
def videoOfToday():
"""
Check what day it is and provide a url to a suitable video together with a greeting.
"""
dayNum = datetime.date.weekday(datetime.date.today()) + 1
msg = getString("weekdays", str(dayNum))
video = getString("video-of-today", str(dayNum))
if video:
msg += " En passande video är " + video
else:
msg += " Jag har ännu ingen passande video för denna dagen."
return msg
There was a problem hiding this comment.
maybe we should remove that in a separate PR then, leave it for now.
test_marvin_actions.py
Outdated
| """Tests that marvin returns the proper error message when nameday API is down""" | ||
| with mock.patch("marvin_actions.requests.get", side_effect=Exception('API Down!')) as r: | ||
| res = self.executeAction(marvin_actions.marvinNameday, 'namnsdag') | ||
| self.assertEqual(res, "Något gick snett, jag har inte en susning om någon har namnsdag idag eller inte.") |
There was a problem hiding this comment.
isn't it possible to use assertStringsOutput here?
test_marvin_actions.py
Outdated
| """Tests that marvin returns the proper error message when joke API is down""" | ||
| with mock.patch("marvin_actions.requests.get", side_effect=Exception('API Down!')) as r: | ||
| res = self.executeAction(marvin_actions.marvinJoke, 'joke') | ||
| self.assertEqual(res, "Chuck Norris har inte tid underhålla er idag!") |
There was a problem hiding this comment.
same here, assertStringsOutput?
test_marvin_actions.py
Outdated
| """Tests that marvin sends the proper message when get commit fails""" | ||
| with mock.patch("marvin_actions.requests.get", side_effect=Exception('API Down!')) as r: | ||
| res = self.executeAction(marvin_actions.marvinCommit, "vad skriver man efter commit -m?") | ||
| self.assertEqual(res, "Du får komma på ett själv. Jag är trasig för tillfället!") |
you can change the message and use force push to update the message |
|
Let me know when its ready to merge. @kh31d4r can let us know if its best practise to resolve each conversation here, or just say "its fine, we talked about it and agreed what to do". |
|
im not sure what is best practice for github workflows. personally, i like to keep the history clean by amending commits, but maybe it's the norm on github to fix comments in separate commits? |
|
I guess that when it comes to a PR, and one are asked to fix something in it. before merge, then it sounds like the content of the PR should be updated. It feels like "all in one place, one prupose". |
|
Yes, within the PR for sure. But by adding new commits to the PR or by updating the existing ones? |
Challe-P
changed the title
Some format to f-strings, some lines that were too long
Fixes sun functionality by changing to new API. Also adds unit tests for sun function.
Adds unit tests to check if the correct messages are returned if request function fails
No description provided.