-
Notifications
You must be signed in to change notification settings - Fork 257
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
Support python 3.7 and 3.8 in travis CI #493
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 |
---|---|---|
|
@@ -2,6 +2,8 @@ language: python | |
|
||
python: | ||
- 3.6 | ||
- 3.7 | ||
- 3.8 | ||
|
||
env: | ||
matrix: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,7 +131,11 @@ async def test_create_pool_deprecations(mysql_params, loop): | |
warnings.simplefilter("always") | ||
async with pool.get() as conn: | ||
pass | ||
assert issubclass(w[-1].category, DeprecationWarning) | ||
# The first warning emitted is expected to be DeprecationWarning: | ||
# in the past, we used to check for the last one but this assumption | ||
# breaks under Python 3.7 that also emits a `ResourceWarning` when | ||
# executed with `PYTHONASYNCIODEBUG=1`. | ||
assert issubclass(w[0].category, DeprecationWarning) | ||
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. Why is this important? 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 is necessary for tests to pass on P3.7 with
It doesn't happen on P3.8 though. Here's related build result: 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. The ResourceWarning msg:
coming from 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. Okay, in this case, we should add a code comment explaining this magic of choosing which element to compare. 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. I have added a code comment. I have also added some explanation in the commit msg. 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. Honestly that 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. No, this happens only under 3.7 |
||
assert conn.closed | ||
|
||
async with create_pool(loop=loop, **mysql_params) as pool: | ||
|
@@ -149,9 +153,10 @@ async def test_sa_connection(table, mysql_params, loop): | |
connection = await engine.acquire() | ||
assert not connection.closed | ||
async with connection: | ||
ret = [] | ||
async for i in connection.execute(tbl.select()): | ||
ret.append(i) | ||
async with connection.execute(tbl.select()) as cursor: | ||
ret = [] | ||
async for i in cursor: | ||
ret.append(i) | ||
assert [(1, 'a'), (2, 'b'), (3, 'c')] == ret | ||
assert connection.closed | ||
|
||
|
@@ -194,21 +199,23 @@ async def test_sa_transaction_rollback(loop, mysql_params, table): | |
async def test_create_engine(loop, mysql_params, table): | ||
async with sa.create_engine(loop=loop, **mysql_params) as engine: | ||
async with engine.acquire() as conn: | ||
ret = [] | ||
async for i in conn.execute(tbl.select()): | ||
ret.append(i) | ||
assert [(1, 'a'), (2, 'b'), (3, 'c')] == ret | ||
async with conn.execute(tbl.select()) as cursor: | ||
ret = [] | ||
async for i in cursor: | ||
ret.append(i) | ||
assert [(1, 'a'), (2, 'b'), (3, 'c')] == ret | ||
|
||
|
||
@pytest.mark.run_loop | ||
async def test_engine(loop, mysql_params, table): | ||
engine = await sa.create_engine(loop=loop, **mysql_params) | ||
async with engine: | ||
async with engine.acquire() as conn: | ||
ret = [] | ||
async for i in conn.execute(tbl.select()): | ||
ret.append(i) | ||
assert [(1, 'a'), (2, 'b'), (3, 'c')] == ret | ||
async with conn.execute(tbl.select()) as cursor: | ||
ret = [] | ||
async for i in cursor: | ||
ret.append(i) | ||
assert [(1, 'a'), (2, 'b'), (3, 'c')] == ret | ||
|
||
|
||
@pytest.mark.run_loop | ||
|
@@ -218,7 +225,7 @@ async def test_transaction_context_manager(loop, mysql_params, table): | |
async with conn.begin() as tr: | ||
async with conn.execute(tbl.select()) as cursor: | ||
ret = [] | ||
async for i in conn.execute(tbl.select()): | ||
async for i in cursor: | ||
ret.append(i) | ||
assert [(1, 'a'), (2, 'b'), (3, 'c')] == ret | ||
assert cursor.closed | ||
|
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.
Looking at this test closer, we could probably just suppress all the warnings except for the
DeprecationWarning
instead of trying to guess on what position it'll be under the next Python version.Ref: https://docs.python.org/3/library/warnings.html#overriding-the-default-filter.
@MateuszCzubak WDYT?
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.
@webknjaz I think that makes a lot of sense but it will look weird to limit to
DeprecationWarning
just in this one place and not in the others. Maybe it would be better to switch all related tests like this in a "refactoring commit", before the actual p3.7/p3.8 compatibility 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.
Hm actually the are only 2 invokations of
warnings.simplefilter("always")
- I think it should be enough to change them both within the same commit - WDYT?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.
Well, if you want to switch all, this needs to go into a separate PR. Let me merge this one and then we can do a follow-up.