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 transient dependency version and add more tests for aiohttp #683

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

selcukcihan
Copy link
Contributor

@selcukcihan selcukcihan commented Apr 25, 2023

Description

While working on https://linear.app/serverless/issue/SC-905/python-sdk-incorrect-span-hierarchy I've noticed that the integration test aiohttp_requester was failing. I've noticed it fails against main as well. I could not trace it to a particular change we had. So I started looking at the dependencies.

I've traced the issue to a release in one of dependencies of aiohttp, named yarl. That repo has an issue that look similar to the problem we are facing: aio-libs/yarl#854

The problem is, url reported in the tracing callbacks https://docs.aiohttp.org/en/stable/tracing_reference.html lack the query string parameter.

This PR adds a unit test that reproduces this problem , fix is yet to be provided. fix is provided as well.

For reference, if we install yarl==1.8.2 the tests pass (previous version). If we use the latest version yarl==1.9.1 the tests fail https://pypi.org/project/yarl/#history

Testing done

Unit/integration tested

@selcukcihan
Copy link
Contributor Author

I've described the exact problem on yarl repo aio-libs/yarl#854 (comment)

@selcukcihan selcukcihan changed the title Draft: Enhance tests for aiohttp Fix transient dependency version and add more tests for aiohttp Apr 25, 2023
@selcukcihan
Copy link
Contributor Author

@medikoo ready for review please 🙏

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch 👍

@medikoo medikoo merged commit a9d215d into serverless:main Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants