-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add async networking libraries support #210
Changes from 8 commits
543d175
00ad1dc
e5b6ec1
5f88377
a4cc84d
30470be
c2d662a
7892057
398dd24
bad2e41
9ceb3dd
cad0c92
3bab585
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 |
---|---|---|
@@ -1,4 +1,6 @@ | ||
requests==2.23.0 | ||
pytest-asyncio == 0.15.1 | ||
aiohttp==3.8.1 | ||
pytest>=4.6.0 | ||
responses>=0.8.1 | ||
setuptools>=38.2.4 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
"""PyTest Fixtures""" | ||
import asyncio | ||
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. You are importing direct asyncio package, while in dev dependencies is pytest-asyncio plugin. Therefore tests are still failing. Btw, why you prefer the asyncio/aiohttp directly as a dependency and not the pytest plugins, if it is used for testing purposes only anyway? My guess would be that pytest plugin are used by many and doing IMHO the same stuff you are doing by hand - according to readme of both plugins. In this case I think is best to use standard solution of the problem instead of own implementation. I it simple code at the moment, I know, no actual problem merging as is, by me. Just a open discussion. I have read the comment "we remain of the idea that it is more useful to use pytest-asyncio + aiohttp" - response is there. But on separate topic just asyncio vs pytest-asyncio - it is anyway just about providing basic wrappers around event loop in the pytest fixtures, right? 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 forgot to update dev-requirements.txt now you shouldn't have any problems with the test anymore. We have removed some useless code. 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. Yeah, with 3bab585 I have passing tests, only with one warning.
|
||
import logging | ||
import os | ||
import pytest | ||
|
@@ -139,3 +140,10 @@ def type_date_time(): | |
@pytest.fixture | ||
def type_date_time_offset(): | ||
return Types.from_name('Edm.DateTimeOffset') | ||
|
||
|
||
@pytest.fixture | ||
def event_loop(): | ||
loop = asyncio.new_event_loop() | ||
yield loop | ||
loop.run_until_complete(asyncio.sleep(0.1, loop=loop)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
"""PyOData Client tests""" | ||
from unittest.mock import patch, AsyncMock | ||
|
||
import aiohttp | ||
import pytest | ||
from aiohttp import web | ||
|
||
import pyodata.v2.service | ||
from pyodata import Client | ||
from pyodata.exceptions import PyODataException, HttpError | ||
from pyodata.v2.model import ParserError, PolicyWarning, PolicyFatal, PolicyIgnore, Config | ||
|
||
SERVICE_URL = '' | ||
|
||
|
||
async def test_invalid_odata_version(): | ||
"""Check handling of request for invalid OData version implementation""" | ||
|
||
with pytest.raises(PyODataException) as e_info: | ||
async with aiohttp.ClientSession() as client: | ||
await Client.build_async_client(SERVICE_URL, client, 'INVALID VERSION') | ||
|
||
assert str(e_info.value).startswith('No implementation for selected odata version') | ||
|
||
|
||
async def test_create_client_for_local_metadata(metadata): | ||
"""Check client creation for valid use case with local metadata""" | ||
|
||
async with aiohttp.ClientSession() as client: | ||
service_client = await Client.build_async_client(SERVICE_URL, client, metadata=metadata) | ||
|
||
assert isinstance(service_client, pyodata.v2.service.Service) | ||
assert service_client.schema.is_valid == True | ||
|
||
assert len(service_client.schema.entity_sets) != 0 | ||
|
||
|
||
def generate_metadata_response(headers=None, body=None, status=200): | ||
async def metadata_repsonse(request): | ||
return web.Response(status=status, headers=headers, body=body) | ||
|
||
return metadata_repsonse | ||
|
||
|
||
@pytest.mark.parametrize("content_type", ['application/xml', 'application/atom+xml', 'text/xml']) | ||
async def test_create_service_application(aiohttp_client, metadata, content_type): | ||
"""Check client creation for valid MIME types""" | ||
|
||
app = web.Application() | ||
app.router.add_get('/$metadata', generate_metadata_response(headers={'content-type': content_type}, body=metadata)) | ||
client = await aiohttp_client(app) | ||
|
||
service_client = await Client.build_async_client(SERVICE_URL, client) | ||
|
||
assert isinstance(service_client, pyodata.v2.service.Service) | ||
|
||
# one more test for '/' terminated url | ||
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. mark at least as TODO to grab more attention in future PR if not done. |
||
|
||
service_client = await Client.build_async_client(SERVICE_URL + '/', client) | ||
|
||
assert isinstance(service_client, pyodata.v2.service.Service) | ||
assert service_client.schema.is_valid | ||
|
||
|
||
async def test_metadata_not_reachable(aiohttp_client): | ||
"""Check handling of not reachable service metadata""" | ||
|
||
app = web.Application() | ||
app.router.add_get('/$metadata', generate_metadata_response(headers={'content-type': 'text/html'}, status=404)) | ||
client = await aiohttp_client(app) | ||
|
||
with pytest.raises(HttpError) as e_info: | ||
await Client.build_async_client(SERVICE_URL, client) | ||
|
||
assert str(e_info.value).startswith('Metadata request failed') | ||
|
||
|
||
async def test_metadata_saml_not_authorized(aiohttp_client): | ||
"""Check handling of not SAML / OAuth unauthorized response""" | ||
|
||
app = web.Application() | ||
app.router.add_get('/$metadata', generate_metadata_response(headers={'content-type': 'text/html; charset=utf-8'})) | ||
client = await aiohttp_client(app) | ||
|
||
with pytest.raises(HttpError) as e_info: | ||
await Client.build_async_client(SERVICE_URL, client) | ||
|
||
assert str(e_info.value).startswith('Metadata request did not return XML, MIME type:') | ||
|
||
|
||
@patch('warnings.warn') | ||
async def test_client_custom_configuration(mock_warning, aiohttp_client, metadata): | ||
"""Check client creation for custom configuration""" | ||
|
||
namespaces = { | ||
'edmx': "customEdmxUrl.com", | ||
'edm': 'customEdmUrl.com' | ||
} | ||
|
||
custom_config = Config( | ||
xml_namespaces=namespaces, | ||
default_error_policy=PolicyFatal(), | ||
custom_error_policies={ | ||
ParserError.ANNOTATION: PolicyWarning(), | ||
ParserError.ASSOCIATION: PolicyIgnore() | ||
}) | ||
|
||
app = web.Application() | ||
app.router.add_get('/$metadata', generate_metadata_response(headers={'content-type': 'application/xml'},body=metadata)) | ||
client = await aiohttp_client(app) | ||
|
||
with pytest.raises(PyODataException) as e_info: | ||
await Client.build_async_client(SERVICE_URL, client, config=custom_config, namespaces=namespaces) | ||
|
||
assert str(e_info.value) == 'You cannot pass namespaces and config at the same time' | ||
|
||
service = await Client.build_async_client(SERVICE_URL, client, namespaces=namespaces) | ||
|
||
mock_warning.assert_called_with( | ||
'Passing namespaces directly is deprecated. Use class Config instead', | ||
DeprecationWarning | ||
) | ||
assert isinstance(service, pyodata.v2.service.Service) | ||
assert service.schema.config.namespaces == namespaces | ||
|
||
service = await Client.build_async_client(SERVICE_URL, client, config=custom_config) | ||
|
||
assert isinstance(service, pyodata.v2.service.Service) | ||
assert service.schema.config == custom_config |
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 have already updated as first commit to the async-feature branch the dev-dependency.txt
pytest-aiohttp>=1.0.4
.Wouldn't explicit pytest plugin by better than generic aiohttp library? Just asking what dependency is better, doesn't seem right to have both.
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.
Still have same 8 errors running pytest
TypeError: sleep() got an unexpected keyword
even with the newly added dev-requirements.Everything is passing in your local environment?
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.
We don't have errors in our test, I forgot the requirements. We added our dev-requirements.txt. We don't have aiohttp test because we want to stay generic without aiohttp dependency.
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 must say I am not understanding the last comment at all.
what does "We don't have aiohttp test because we want to stay generic without aiohttp dependency." mean? Especially when you commited
aiohttp==3.8.1
to the dev-dependencies already? Do you want that or not?And should the
pytest-aiohttp
from me, which already is in theasync-feature
branch dev-requirements.txt stay or not? Should the async tests be based on pytest-aiohttp plugin or pytest-asyncio +aiohttp ? I expected we will go pytest plugins way.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.
Sorry if we were not clear, I try to summarize our considerations why they led me not to choose pytest-aiohttp:
We have rewritten the tests anyway using pytest-aiohttp but we remain of the idea that it is more useful to use pytest-asyncio + aiohttp.
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.
Question: is your code in client.py/service.py compatible with
httpx
python package?Or does it expect
aiohttp.ClientSession()
to be provided in theClient.build_async_client
only?If it is more open (best possible outcome, I don't actually know about any other than
requests
library being used with pyodata, see for example #154 (comment) ), then why not add both integration tests withaiohttp
andhttpx
packages?Another possibility is that you are ONLY talking about the aiohttp usage in the tests. But then I am still curious about the pyodata package (with this PR) compatibility matrix with async python http networking libraries.
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.
We should check, in theory it should work with httpx as well.