Add aiohttp tests to CI #1415
Add aiohttp tests to CI #1415
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
CodSpeed Performance ReportMerging #1415 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1415 +/- ##
=======================================
Coverage 96.06% 96.06%
=======================================
Files 31 31
Lines 5772 5772
Branches 345 345
=======================================
Hits 5545 5545
Misses 201 201
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Awesome, thanks!
Would you consider testing against maintained aiohttp releases as well?
I see two options:
- Install aiohttp from PyPI tarball. It has tests, maybe tarball could be combined with wheels to avoid compilation steps.
- Install from GitHub maintained branch.
For any approach editing workflow file for updating a list of supported aiohttp versions is totally fine.
I don't know which is better. Maybe fast aiohttp installation time could be a priority.
It could be implemented in the following PR, sure.
What is your opinion?
I think both options are acceptable if they achieve the same result. Downloading tarball and whl from pypi may be better since it can avoid compiling and building steps. |
.github/workflows/aiohttp.yml
Outdated
| - name: Update pip, wheel, setuptools, build, twine | ||
| run: | | ||
| python -m pip install -U pip wheel setuptools build twine |
There was a problem hiding this comment.
Nothing short of build seems to be necessary here.
There was a problem hiding this comment.
Nothing short of
buildseems to be necessary here.
I just realized that we can remove this entire step, since the necessary upgrades are already ensured by the makefile command.
There was a problem hiding this comment.
If we want to make this check required in pull requests, it must be integrated into the main CI/CD workflow.
There was a problem hiding this comment.
I don't mind too much, but running a different project's test suite kind of feels to me like it's cleaner to me to be in a separate file. We can still add this as a separate required status in the repo settings.
.github/workflows/aiohttp.yml
Outdated
| - name: Run dev_mode tests | ||
| run: python -X dev -m pytest -m dev_mode | ||
| shell: bash |
There was a problem hiding this comment.
We probably don't need to run the dev_mode tests here
There was a problem hiding this comment.
Yeah, there's no harm in running it, but probably not doing anything useful for yarl. I assume that came from the llhttp workflow which definitely does need this to test strict/lax parsing differences.
So, one reason you probably want to stick to checking out the aiohttp code, is when yarl decides to break something in aiohttp on-purpose, you won't be able to get the tests passing on a released version of aiohttp. By checking out the code from the repo, we can adjust the tests on aiohttp to allow the new behaviour and then rerun the tests here against the new aiohttp commit. |
What do these changes do?
Test aiohttp with new changes in yarl to prevent unexpected breaking changes.
Are there changes in behavior for the user?
Related issue number
fixes #534
Checklist