Mock timeout scenario for default_location#163
Mock timeout scenario for default_location#163christe6-osu wants to merge 8 commits intoryansurf:mainfrom
Conversation
Reviewer's GuideThis PR enhances the default_location implementation to gracefully handle request timeouts and other request exceptions by returning “No data,” and it extends the existing test suite to mock and verify these failure scenarios. Sequence diagram for default_location timeout handlingsequenceDiagram
participant User
participant default_location
participant requests
User->>default_location: Call default_location()
default_location->>requests: requests.get("https://ipinfo.io/json", timeout=10)
alt Timeout occurs
requests-->>default_location: Raise Timeout exception
default_location-->>User: Return "No data"
else Other RequestException occurs
requests-->>default_location: Raise RequestException
default_location-->>User: Return "No data"
else Success
requests-->>default_location: Response (status_code)
alt status_code == OK
default_location-->>User: Return location data
else status_code != OK
default_location-->>User: Return "No data"
end
end
Class diagram for default_location exception handlingclassDiagram
class default_location {
+default_location()
+requests.get()
+Exception handling: Timeout, RequestException
+Returns: location data or "No data"
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @christe6-osu - I've reviewed your changes - here's some feedback:
- In default_location(), consider catching HTTPError (or more generally RequestException) in addition to Timeout so that non-200 responses don’t raise uncaught exceptions.
- The test’s use of
if status_code == Timeoutto set a side effect is brittle; you could split the exception scenario into its own param or fixture to cleanly inject the Timeout exception.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Testing for the Timeout exception required adding code to src/api.py, which I realize may not be desirable. It is passing the pre-existing tests and the new Timeout test. However, I have not been able to get the application to work locally, so I cannot run everything to verify functionality. Please let me know how it looks and if there is anything you would like me to change. |
|
I see that sourcery brought up a couple issues that I had debated about but was unsure of the best approach. Should I add another except block that catches the general RequestException as it suggests? As for including side_effect in the parameters, I considered that but was not sure if having side_effect = None for the other tests would cause an issue. I can test this locally and push to include with the PR if it works. |
|
Hey @christe6-osu, I haven't had time to review this yet but wanted to let you know I've seen it! I'll get to it in the next few days |
Hey @christe6-osu, apologies for just getting to this. I'll go ahead and test this locally myself, but what is going wrong when you are trying to run it locally? edit: not sure what happened on your end, but i made a small change to the readme's setup section |
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
|
Sorry, I've been slammed but I am getting back around to this. I removed raise_for_status from src/api.py since it was catching every HTTP response other than 200, and added the general RequestException test that Sourcery recommended. As for running everything locally, I followed the instructions to run with Docker but without installing Poetry. When I run the container, it gets a ModuleNotFoundError for flask. I have been trying to look into it but my best guess at this point is that my OS or the virtual environment I am using for the project is causing an issue. |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @christe6-osu - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/api.py:55` </location>
<code_context>
- response = requests.get("https://ipinfo.io/json", timeout=10)
+ try:
+ response = requests.get("https://ipinfo.io/json", timeout=10)
+ except requests.exceptions.Timeout:
+ return "No data"
+ except requests.exceptions.RequestException:
</code_context>
<issue_to_address>
Catching Timeout before RequestException is redundant.
Since both exceptions are handled identically, you can just catch RequestException to simplify the code.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
try:
response = requests.get("https://ipinfo.io/json", timeout=10)
except requests.exceptions.Timeout:
return "No data"
except requests.exceptions.RequestException:
return "No data"
=======
try:
response = requests.get("https://ipinfo.io/json", timeout=10)
except requests.exceptions.RequestException:
return "No data"
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try: | ||
| response = requests.get("https://ipinfo.io/json", timeout=10) | ||
| except requests.exceptions.Timeout: | ||
| return "No data" | ||
| except requests.exceptions.RequestException: | ||
| return "No data" |
There was a problem hiding this comment.
suggestion: Catching Timeout before RequestException is redundant.
Since both exceptions are handled identically, you can just catch RequestException to simplify the code.
| try: | |
| response = requests.get("https://ipinfo.io/json", timeout=10) | |
| except requests.exceptions.Timeout: | |
| return "No data" | |
| except requests.exceptions.RequestException: | |
| return "No data" | |
| try: | |
| response = requests.get("https://ipinfo.io/json", timeout=10) | |
| except requests.exceptions.RequestException: | |
| return "No data" |
2 participants
General:
Added a test for the timeout scenario for default_location.
Modified default_location to include a try/except block to catch Timeout exceptions.
Code:
Summary by Sourcery
Add exception handling to the default_location function to return "No data" on request timeouts or other request errors, and expand tests to simulate and verify these error scenarios.
Enhancements:
Tests: