-
Notifications
You must be signed in to change notification settings - Fork 203
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
Fixes #3321 - Converts to pytest and improves coverage #3322
Conversation
That will make testing easier in certain casesby giving a github_data mock which has a meaning github_data.json() function.
This simplifies a bit the tests and it exposes only what we need in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few minor changes.
1. It converts all the tests to pytest format 2. It adds new tests for handling the scenario which were not previously tested (as revealed by coverage). issues.py is now 100% covered.
ok rebased. and pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Feel free to rename the client
method or not, up to you.
Thanks Karl, very nice improvements. |
This PR fixes issue #3321
Proposed PR background
Please provide enough information so that others can review your pull request:
This took a bit more time than I was expecting but I have figured out a couple of things.
This also should lead to more test organization. I need to open an issue so that we put the mock_api_response outside of test_api_urls. (probably in conftest.py if I understood well pytest)
Anyway.
It basically
.json()
method for the mocks.Now we have 100% coverage for the issues.py module. See #3205 for the work to do still.
ok let's see if we stay green or if we broke something.