-
Notifications
You must be signed in to change notification settings - Fork 656
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
Fix and improve tests for Python != 3.7 #95
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,7 +94,7 @@ def tearDown(self): | |
|
||
def start_response(self, status, response_headers, exc_info=None): | ||
# The span should have started already | ||
self.span_context_manager.__enter__.assert_called() | ||
self.span_context_manager.__enter__.assert_called_with() | ||
|
||
self.status = status | ||
self.response_headers = response_headers | ||
|
@@ -108,7 +108,9 @@ def validate_response(self, response, error=None): | |
self.span_context_manager.__exit__.assert_not_called() | ||
self.assertEqual(value, b"*") | ||
except StopIteration: | ||
self.span_context_manager.__exit__.assert_called() | ||
self.span_context_manager.__exit__.assert_called_with( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too bad we need this change. 🙁 Which Python version requires it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need it for 3.5 and 3.4. |
||
None, None, None | ||
) | ||
break | ||
|
||
self.assertEqual(self.status, "200 OK") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,7 +145,9 @@ async def main(): | |
__all__ = ['Context'] | ||
|
||
|
||
Context: typing.Optional[BaseRuntimeContext] | ||
Context = ( # pylint: disable=invalid-name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes me wonder if we shouldn't relax the pylint rule for "constants" (maybe even to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's done. Please review b05f347. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related PR: #108 |
||
None | ||
) # type: typing.Optional[BaseRuntimeContext] | ||
|
||
try: | ||
from .async_context import AsyncRuntimeContext | ||
|
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.
Nice. However, we really don't need to have
3.8-dev
, and specially thenightly
one, IHMO (even with the allow_failures flag) ;)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.
Why not?
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 don't think there's any benefit IHMO.
dev
can change relatively a lot. I don't think there's any value in having it here honestly (unless we were married to some internal or prototype feature of it).Anyway, I wouldn't totally be against this, so I will leave others give their opinion ;)
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.
Python 3.8 is almost there, so it makes sense to have tests for it.
In addition, I believe that it's always better to know about any incompatibilities sooner rather than later.
Especially if running the tests costs nothing.
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 does make builds slower. But I have no strong opinion on
dev
either. I also believe thatnightly
is too much, things can still change there so if we fix an error innightly
and thennightly
changes to make the fix unnecessary, we wasted time.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.
Hi @carlosalberto and @Oberon00,
I've removed
nightly
for now.