-
Notifications
You must be signed in to change notification settings - Fork 28
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
Updated tests to use context managers #294
Conversation
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
059e31d
to
4f651e0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
==========================================
+ Coverage 89.71% 89.81% +0.09%
==========================================
Files 53 53
Lines 2674 2680 +6
==========================================
+ Hits 2399 2407 +8
+ Misses 275 273 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 pretty good to me!
with Console(test_profile) as console: | ||
self.console = console |
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.
For integration tests, it doesn't work to use a with
statement in the setUp
block because the request.Session
object gets deleted immediately before tests can run.
I think the following would work instead:
with Console(test_profile) as console: | |
self.console = console | |
self.console = Console(test_profile) | |
self.addCleanup(lambda: self.console.__exit__(None, None, None)) |
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.
So, the requests.Session
object gets deleted in each integeration test
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.
That's right, we want to delete the requests.Session
object for each integration test, but need to wait until after the test runs which can be accomplished by addCleanup
.
ab97f01
to
4f651e0
Compare
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
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.
changes LGTM! 😋
just one minor comment
Co-authored-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com> Signed-off-by: Aaditya Sinha <75474786+aadityasinha-dotcom@users.noreply.github.com>
What It Does
Resolves #288
How to Test
Review Checklist
I certify that I have:
Additional Comments