Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle 500 Internal Server Error with RedcapError #263

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

angus-lherrou
Copy link

Fixes #261.

Also raises a RedcapError from any JSONDecodeError in the response.json() call, but this can be reverted if it's too out of scope for this PR.

@angus-lherrou
Copy link
Author

@pwildenhain I would like to add a unit test for this but I'm not sure how to mock an internal server error in the testing framework

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #263 (ab45bd9) into master (a507d1a) will decrease coverage by 0.35%.
The diff coverage is 71.42%.

@@             Coverage Diff             @@
##            master     #263      +/-   ##
===========================================
- Coverage   100.00%   99.65%   -0.35%     
===========================================
  Files           19       19              
  Lines          568      573       +5     
===========================================
+ Hits           568      571       +3     
- Misses           0        2       +2     
Impacted Files Coverage Δ
redcap/request.py 97.22% <71.42%> (-2.78%) ⬇️

@pwildenhain
Copy link
Collaborator

@pwildenhain I would like to add a unit test for this but I'm not sure how to mock an internal server error in the testing framework

Yea the unit testing framework is a lot to take in at first, but it's actually pretty neat, and could be good to learn if you're ever looking to add unit tests for one of your own API wrapper packages

I would recommend building in a fake json decode error and fake 500 response into some handlers for the survey project test fixture (found in tests/unit/test_survey_project.py at the top)

You'll see in that fixture that it's dynamically looking for request handlers, which you can find for the survey project fixture at the end of tests/unit/callback_utils.py

I suggest adding a few more handlers for things like DAGs, users, etc and then raising a json decode error in one of them and returning a 500 (instead of a 201) in the other one.

Then add tests for that survey project where you try to access those records and use pytest.raises(RedCapError) to ensure that they were caught

Let me know if this is unclear, or if you are not interested in adding tests and I can try to add them in.

Also sorry for the crap formatting I'm writing this on mobile

@angus-lherrou
Copy link
Author

Thanks, I will take a crack at it when I have a chance this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected Behavior: 500 Internal Server Error unhandled by PyCap
2 participants