-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Improvement test_client can accept instance object. #1083
Conversation
Current coverage is 97.64% (diff: 100%)@@ master #1083 diff @@
==========================================
Files 28 28
Lines 6397 6403 +6
Methods 0 0
Messages 0 0
Branches 1085 1086 +1
==========================================
+ Hits 6246 6252 +6
Misses 79 79
Partials 72 72
|
if not isinstance(app_factory, Application): | ||
app = app_factory(loop, *args, **kwargs) | ||
else: | ||
app = app_factory |
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.
There must be assertion that app.loop
is the same as used in 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.
It means that there must be assertion like this?
assert app.loop is loop
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.
Yes!
Plus message like this "Application is attached to other event loop"
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.
Ok. Please check new commit.
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.
Also please make sure that args
and kwargs
are both empty.
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.
@asvetlov When I debug the test case, parameter as a create_app function also args and kwargs are both empty.
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.
I mean
if isinstance(app_factory, Application):
assert not args, "arg should be empty"
assert not kwargs, "kwargs should be empty"
....
It will prevent cases like: yield from test_client(app, 1, 2, param=3)
.
Looks good, |
@popravich How to check if assertion is raised? Just use try and except? |
with pytest.raises(AssertionError):
test_client(web.Application(loop=fake_loop) should work |
@popravich I added negative test case ! |
@@ -122,4 +131,4 @@ def make_app(loop): | |||
|
|||
""") | |||
result = testdir.runpytest('-p', 'no:sugar') | |||
result.assert_outcomes(passed=8, failed=1) | |||
result.assert_outcomes(passed=8, failed=2) |
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.
But why is it failing? If it fails it means that assertion is not raised.
Thanks! |
What do these changes do?
Improvement test_client can accept instance object as parameter.
Are there changes in behavior for the user?
No
Related issue number
#1066
Checklist