Skip to content
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 python 3.8 tests under Windows #4463

Closed
wants to merge 8 commits into from
Closed

Conversation

@hh-h
Copy link
Contributor

hh-h commented Dec 20, 2019

What do these changes do?

It should fix tests for python 3.8 under Windows

Are there changes in behavior for the user?

No

Related issue number

No

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
asvetlov and others added 4 commits Nov 27, 2019
@hh-h hh-h requested a review from asvetlov as a code owner Dec 20, 2019
@hh-h hh-h changed the title [WIP] fix tests fix tests Dec 20, 2019
@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 20, 2019

Plz fill in the issue template/description/title with some meaningful explanation.

@hh-h

This comment has been minimized.

Copy link
Contributor Author

hh-h commented Dec 20, 2019

I'm trying to fix python 3.8 windows tests, but I don't understand how to run azure pipelines for PR, any help @webknjaz ?

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 20, 2019

I think, all CI checks start up automatically when sent against master. So probably it's because you send a PR against py38 branch. Switch it.

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 20, 2019

And you should still update the PR title and the description.

@hh-h

This comment has been minimized.

Copy link
Contributor Author

hh-h commented Dec 20, 2019

But there's no python 3.8 in master nor 3.7. Can I temporary include py38 here https://github.com/aio-libs/aiohttp/blob/master/.azure-pipelines/ci.yml#L4 ?

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 20, 2019

Your branch already contains commits from py38.

@hh-h hh-h changed the title fix tests [WIP] Fix python 3.8 tests under Windows Dec 20, 2019
@hh-h hh-h changed the base branch from py38 to 3.7 Dec 20, 2019
@hh-h hh-h changed the base branch from 3.7 to py38 Dec 20, 2019
@hh-h

This comment has been minimized.

Copy link
Contributor Author

hh-h commented Dec 20, 2019

Okay, I see two ways

  • create branch from 3.7, cherry-pick CI commit from py38 and add my changes
  • update ci.yml and temporary add py38 branch there for testing

What would you suggest?

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 20, 2019

No, I think that you shouldn't recreate the PR. Just change the base branch. I'll update it myself. But for the future — just click Edit right to the PR title and you'll see a UI control to do it.

@webknjaz webknjaz changed the base branch from py38 to master Dec 20, 2019
@@ -1443,7 +1443,7 @@ def make_handler(appname):
assert 413 == resp.status
resp_text = await resp.text()
assert 'Maximum request body size 1048576 exceeded, ' \
'actual body size 1048591' in resp_text
'actual body size' in resp_text

This comment has been minimized.

Copy link
@webknjaz

webknjaz Dec 20, 2019

Member

Why is it different?

This comment has been minimized.

Copy link
@hh-h

hh-h Dec 20, 2019

Author Contributor

this test I wanted to discuss with @asvetlov , this case is a little bit weird and error message of HTTPRequestEntityTooLarge is invalid on some circumstances, proof

server.py

from aiohttp import web


async def handle(request):
    await request.post()
    return web.Response()


app = web.Application(client_max_size=10)
app.add_routes([web.post("/", handle)])

web.run_app(app)

client.py

import asyncio
import json
from aiohttp import ClientSession


async def main():
    async with ClientSession() as session:
        a = {"test": 1024**2 * 'x'}
        print(len(json.dumps(a)))   # prints 1048588
        async with session.post("http://0.0.0.0:8080", data=a) as resp:
            print(await resp.text())  # prints Maximum request body size 10 exceeded, actual body size 261948 

asyncio.run(main())

261948 != 1048588

This comment has been minimized.

Copy link
@webknjaz

webknjaz Dec 20, 2019

Member

So this probably means that the doesn't read the whole response which may be a bug so we shouldn't ignore it like this. Or at least we need some assertion that this number is bigger.

This comment has been minimized.

Copy link
@hh-h

hh-h Dec 20, 2019

Author Contributor

That was my first thought, but I wanted to discuss it first.

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 20, 2019

Oh. And the trick is that you should only add/change your own commits and keep those that are derived from py38 unchanged. This will simplify merging your branch there or rebasing it avoiding merge conflicts.

@pytest.fixture
def selector_loop(): # type: ignore
if not PY_37:
policy = asyncio.get_event_loop_policy()

This comment has been minimized.

Copy link
@webknjaz

webknjaz Dec 20, 2019

Member

Won't 3.8 also hit this branch?

This comment has been minimized.

This comment has been minimized.

Copy link
@webknjaz

webknjaz Dec 20, 2019

Member

Oh, so the var name is unfortunate.

This comment has been minimized.

Copy link
@hh-h

hh-h Dec 20, 2019

Author Contributor

Yes, at first I got confused too...

This comment has been minimized.

Copy link
@webknjaz

webknjaz Dec 20, 2019

Member

so maybe ... import PY_38 as AT_LEAST_PY_38 then?

This comment has been minimized.

Copy link
@hh-h

hh-h Dec 20, 2019

Author Contributor

I think this is out of scope and the general idea of PY_3* should be reworked.

@@ -1469,7 +1469,7 @@ def make_handler(appname):
assert 413 == resp.status
resp_text = await resp.text()
assert 'Maximum request body size 2097152 exceeded, ' \
'actual body size 2097166' in resp_text
'actual body size' in resp_text

This comment has been minimized.

Copy link
@webknjaz

webknjaz Dec 20, 2019

Member

If the number is different, we need some explanation to comment here why this is happening. Is it possible to figure out the precise number that would appear here?

This comment has been minimized.

Copy link
@hh-h

hh-h Dec 20, 2019

Author Contributor

see my comment #4463 (comment)

This comment has been minimized.

Copy link
@hh-h

hh-h Dec 20, 2019

Author Contributor

Updated tests, WDYT @webknjaz ?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 20, 2019

Codecov Report

Merging #4463 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4463      +/-   ##
=========================================
+ Coverage   97.52%   97.6%   +0.07%     
=========================================
  Files          43      43              
  Lines        8865    8909      +44     
  Branches     1390    1403      +13     
=========================================
+ Hits         8646    8696      +50     
+ Misses         95      94       -1     
+ Partials      124     119       -5
Impacted Files Coverage Δ
aiohttp/pytest_plugin.py 97.61% <100%> (+0.34%) ⬆️
aiohttp/tracing.py 100% <0%> (ø) ⬆️
aiohttp/cookiejar.py 100% <0%> (ø) ⬆️
aiohttp/web_request.py 97.58% <0%> (ø) ⬆️
aiohttp/client_reqrep.py 97% <0%> (+0.01%) ⬆️
aiohttp/frozenlist.py 97.91% <0%> (+0.04%) ⬆️
aiohttp/web_response.py 97.71% <0%> (+0.22%) ⬆️
aiohttp/helpers.py 96.61% <0%> (+0.5%) ⬆️
aiohttp/web_protocol.py 92.69% <0%> (+0.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bf2330...e58d57e. Read the comment docs.

hh-h added 2 commits Dec 20, 2019
@hh-h hh-h changed the title [WIP] Fix python 3.8 tests under Windows Fix python 3.8 tests under Windows Dec 20, 2019
@hh-h hh-h force-pushed the hh-h:fix-tests branch from fd1c795 to d5ac3b4 Dec 20, 2019
@asvetlov

This comment has been minimized.

Copy link
Member

asvetlov commented Dec 20, 2019

This pull request has been mentioned on aio-libs. There might be relevant details there:

https://aio-libs.discourse.group/t/aiohttp-new-release/49/5

@asvetlov

This comment has been minimized.

Copy link
Member

asvetlov commented Dec 24, 2019

Thanks for the PR.
A part of the fixes is correct and can be applied.
The total switching from the proactor loop to select based is wrong, sorry.
The proactor event loop is the default on Python 3.8, aiohttp should work with it out of the box.
Unfortunately, tricks for emulation of starttls and sendfile don't work with proactor. #4268 and #4269 should be implemented for publishing Python 3.8 compatible release.

@hh-h

This comment has been minimized.

Copy link
Contributor Author

hh-h commented Dec 24, 2019

I had to add force selector_loop only for testing wrong loop, since proactor is default for windows after 3.8, as you mentioned. Only two tests should use force selector_loop, test_named_pipe_connector_wrong_loop and test_named_pipe_runner_wrong_loop, could you consider the new information or what should I do with PR?

@asvetlov

This comment has been minimized.

Copy link
Member

asvetlov commented Jan 18, 2020

Superseded by #4513

@hh-h thanks for the PR

@asvetlov asvetlov closed this Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.