Closed kloczek closed 6 months ago
Here is pytest output:
The circular_imports tests appear to be failing to import. I'd compare with how our CI runs it (https://github.com/aio-libs/aiohttp/blob/master/.github/workflows/ci.yml#L128), but I suspect you'll need to install the package or tweak PYTHONPATH to get it to work correctly.
Then it looks like there is a DNS error, maybe that test should be mocked or something...
The unclosed socket warnings we do see occasionally, but haven't figured out how to fix yet. Not sure about the compress assertion..
suspect you'll need to install the package or tweak PYTHONPATH to get it to work correctly.
Can you explain what exactly you mean? 🤔
It's running a subprocess, and that subprocess can't find aiohttp. It's a little bit awkward the way it needs to run another process, and I remember needing to fiddle a bit to get another test working that way.
OK so what this descriprion has to do with $PYTHONPATH env variable? 🤔 Did you saw in ticket description that I've alredy heve been using that variable?
OK so what this descriprion has to do with $PYTHONPATH env variable?
Well, if it can't find it and you didn't install it, then you might need to add the source code directory to PYTHONPATH. Otherwise, maybe the code is not passing the variable through to the subprocess. Just some guesses as to why it's not finding aiohttp.
Well, if it can't find it and you didn't install it, then you might need to add the source code directory to PYTHONPATH. Otherwise, maybe the code is not passing the variable through to the subprocess. Just some guesses as to why it's not finding aiohttp.
Please read one mote time description of what I've exacly done. I'm already using $PYTHONPATH .
I read it the first time, it didn't help help me understand if it's pointing in the right place or not. Maybe I'm just not familiar with what sitearch/sitelib means. Either way, back to my first point, please look at how the CI runs the tests, it passes successfully there.
That procedure is now used on massive scale on packaging as rpm, deb and other types of package management software.
Maybe I'm just not familiar with what sitearch/sitelib means.
[tkloczko@pers-jacek SRPMS]$ python3 -Ic "import sysconfig; print(sysconfig.get_path('platlib'))"
/usr/lib64/python3.8/site-packages
[tkloczko@pers-jacek SRPMS]$ python3 -Ic "import sysconfig; print(sysconfig.get_path('purelib'))";
/usr/lib/python3.8/site-packages
This is why in first line of the build log which Ive copied is
+ PYTHONPATH=/home/tkloczko/rpmbuild/BUILDROOT/python-aiohttp-3.8.4-5.fc35.x86_64/usr/lib64/python3.8/site-packages:/home/tkloczko/rpmbuild/BUILDROOT/python-aiohttp-3.8.4-5.fc35.x86_64/usr/lib/python3.8/site-packages
In this case /home/tkloczko/rpmbuild/BUILDROOT/python-aiohttp-3.8.4-5.fc35.x86_64
it is my </install/prefix>.
It's running a subprocess, and that subprocess can't find aiohttp. It's a little bit awkward the way it needs to run another process, and I remember needing to fiddle a bit to get another test working that way.
I think that I know why it is.
Currently pytest shows deprecation warnings about use pkg_resources
module. This module is know that it messes with sys.path
.
Use of pkg_resources
needs to be dropped.
Just reteded aiohttp
in updated build env and found that pytest is failing in new unit
Use of
pkg_resources
needs to be dropped.
Right, if you have time to make a PR, that'd be great.
Just reteded
aiohttp
in updated build env and found that pytest is failing in new unit
Right, if you have time to make a PR, that'd be great.
Sorry I'm very busy 😞 Maybe next week ..
Just tested new 3.8.4. pytest is failing now in 10 units
Updated pytest for 3.8.6
Wait, hang on. I think you might need to change the pytest command. The strict tests are tagged with dev_mode, they should only be run under Python's development mode. The pytest config adds -m "not dev_mode"
by default. Seems like that's got lost when you run it.
Our CI does a 2nd pytest invocation that runs the dev_mode tests separately: https://github.com/aio-libs/aiohttp/blob/master/.github/workflows/ci-cd.yml#L246
Wait, hang on. I think you might need to change the pytest command. The strict tests are tagged with dev_mode, they should only be run under Python's development mode. The pytest config adds
-m "not dev_mode"
by default. Seems like that's got lost when you run it.
So should I run pytest the same way? 🤔
Well, you'll probably need to add -m "not dev_mode"
to what you currently run. My understanding is that it should have done this automatically from the config in setup.cfg, so not sure why that didn't work for you.
Then, if you want to run all the tests, you'll need to do a second pytest run with dev mode enabled, as per the CI code (dev_mode is <10 tests though, so it's not a major concern if you don't run it).
Running pytest with -m "not dev_mode"
do not fixes the issue.
Just tested 3.9.0.
Here is updated output
The strict tests are no longer running, so that part atleast has been fixed.
For the others:
tests/__init__.py
. The file has been removed again for 3.9.1, but could be added again in the future, as it is needed if we want to configure mypy on the tests. When the file is present, it seems to only find the C extensions if the package is installed (the CI runs pip install -e .
before running the tests).The strict tests are no longer running, so that part atleast has been fixed.
For the others:
- test_client_session_timeout_zero: I think I've seen this skipped in a downstream package due to it needing internet access. We should probably fix that to work without.
Many other test suites in other modules allows disable units which requires public network akces using network
pytest mark.
- test_requote_redirect_url_default: Probably unclosed socket from previous test failing.
If it is true it means that there are some dependencies between units. It is module with python extension which allows automatically expose such dependencies https://github.com/mrbean-bremen/pytest-find-dependencies/
- test_expires/test_max_age/test_cookie_jar_clear_expired: You've not installed time-machine (I think requirements/ will be included in sdist in 3.9.1, so you should install/check requirements/test.txt in future).
[tkloczko@pers-jacek aiohttp-3.9.0]$ grep -r time-machine
tests/test_cookiejar.py: reason="time_machine leverages CPython specific pointers https://github.com/adamchainz/time-machine/issues/305",
tests/test_cookiejar.py: reason="time_machine leverages CPython specific pointers https://github.com/adamchainz/time-machine/issues/305",
tests/test_cookiejar.py: reason="time_machine leverages CPython specific pointers https://github.com/adamchainz/time-machine/issues/305",
- test_send_compress_text: Not entirely sure, but maybe this is caused by a regression in 3.9.0 (with a low chance of it happening as a race condition). Should have 3.9.1 pushed shortly, so retest with that.
If you have any suggestions how can I try to diagnose this please let me know.
- test_cookie_jar_clear_domain/test_send_compress_text_notakeover: Unclosed socket could be from the previous test failing, maybe. But, I'm not sure how there can be an unclosed socket, as those tests don't appear to use sockets for anything...
As same as second point.
- test_http_parser.py: It can't find the C extensions. Apparently, this is because of the addition of
tests/__init__.py
. The file has been removed again for 3.9.1, but could be added again in the future, as it is needed if we want to configure mypy on the tests. When the file is present, it seems to only find the C extensions if the package is installed (the CI runspip install -e .
before running the tests).
C extension has been build and is packed into .whl which on next stage has been unpacked into /home/tkloczko/rpmbuild/BUILDROOT/python-aiohttp-3.9.0-2.fc35.x86_64/
build
output:
BTW: Looking on list of packaged into .whl archive first I think that .pyx and .pyi files should not be listed. Is that correct? 🤔
Many other test suites in other modules allows disable units which requires public network akces using
network
pytest mark.
I don't think that test should need a network connection. I've opened #7906.
If it is true it means that there are some dependencies between units.
No, it's a known pytest bug. If an error occurs due to garbage collection (in __del__()
), which is the case for these unclosed resources, they pretty much always happen in the setup of the next test.
The only thing we can do here, is try and make sure we correctly close all the resources within a test. But, as I said, I've no idea where those Unix sockets are even coming from...
tests/test_cookiejar.py: reason="time_machine leverages CPython specific pointers https://github.com/adamchainz/time-machine/issues/305",
I'm not sure what your point is here. If you're not using CPython, it should have been skipped. As it was not skipped, I'm assuming you are using CPython, and therefore forgot to install time-machine.
If you have any suggestions how can I try to diagnose this please let me know.
I'd just retest against 3.9 branch at the moment. I think we have the last regression fix merging in now, so I'll prepare a release in the next few hours.
which on next stage has been unpacked into
Yes, as I said, the problem is that it is not installed. I don't know why it makes such a difference to pytest. But, you can either install it, or manually delete tests/__init__.py
. Then it should find it correctly...
BTW: Looking on list of packaged into .whl archive first I think that .pyx and .pyi files should not be listed.
.pyi are typing stubs. If you don't include them, then the user would be missing some typing information.
.pyx could probably be removed, I don't think that's needed for a pre-compiled distribusion. If you'd like to make a PR for that, then that'd be great.
I don't think that test should need a network connection. I've opened #7906.
👍
If it is true it means that there are some dependencies between units.
No, it's a known pytest bug. If an error occurs due to garbage collection (in
__del__()
), which is the case for these unclosed resources, they pretty much always happen in the setup of the next test.The only thing we can do here, is try and make sure we correctly close all the resources within a test. But, as I said, I've no idea where those Unix sockets are even coming from...
I'm not sure what your point is here. If you're not using CPython, it should have been skipped. As it was not skipped, I'm assuming you are using CPython, and therefore forgot to install time-machine.
I'm pointing on two things:
If you have any suggestions how can I try to diagnose this please let me know.
I'd just retest against 3.9 branch at the moment. I think we have the last regression fix merging in now, so I'll prepare a release in the next few hours.
Thank you. If you have any commits/PRs which you want me to test before 3.9.1 release I'm ready to help with testing 😄
which on next stage has been unpacked into
Yes, as I said, the problem is that it is not installed. I don't know why it makes such a difference to pytest. But, you can either install it, or manually delete
tests/__init__.py
. Then it should find it correctly...BTW: Looking on list of packaged into .whl archive first I think that .pyx and .pyi files should not be listed.
.pyi are typing stubs. If you don't include them, then the user would be missing some typing information.
.pyx could probably be removed, I don't think that's needed for a pre-compiled distribusion. If you'd like to make a PR for that, then that'd be great.
OK I understand👍 I'm not sure about roles of those files. IIRC .pyx files are used by cython to create .c files so this is why I wrote that some of those files probably should not be installed 😄
In mean time I've started pytest --find-dependencies
and with installed pytest-find-dependencies
but because number of permutations to test is quite big probably full test will take even few hours 😋
As some units are failing it is not possible to perform full test however it was possible to find some dependencies
============================== Results ===============================
Run dependency analysis for 2661 tests.
Executed 13365 tests in 65 test runs.
The following tests are always failing and are excluded from the analysis:
tests/test_http_parser.py::test_c_parser_loaded
tests/test_client_session.py::test_client_session_timeout_zero
tests/test_cookiejar.py::test_cookie_jar_clear_expired
tests/test_http_parser.py::test_invalid_character[pyloop]
tests/test_http_parser.py::test_invalid_linebreak[pyloop]
tests/test_websocket_writer.py::test_send_compress_text
tests/test_web_server.py::test_unsupported_upgrade[pyloop]
tests/test_cookiejar.py::TestCookieJarSafe::test_expires
tests/test_cookiejar.py::TestCookieJarSafe::test_max_age
Dependent tests:
tests/test_cookiejar.py::test_cookie_jar_clear_domain depends on tests/test_cookiejar.py::test_cookie_jar_clear_expired
tests/test_client_session.py::test_requote_redirect_url_default depends on tests/test_client_session.py::test_client_session_timeout_zero
tests/test_websocket_writer.py::test_send_compress_text_notakeover depends on tests/test_websocket_writer.py::test_send_compress_text
tests/test_client_session.py::test_client_session_timeout_argument depends on tests/test_client_session.py::test_client_session_timeout_zero
tests/test_cookiejar.py::test_cookie_jar_clear_all depends on tests/test_cookiejar.py::test_cookie_jar_clear_expired
tests/test_websocket_writer.py::test_send_text_masked depends on tests/test_websocket_writer.py::test_send_compress_text
Dependency test FAILED
Will try to run second test with --deselect already failing units.
Just FTR .. tested 3.9.1and now more units are failing.
Well, other than import_time, which is probably just some random thing with your setup that caused it run slowly that time, the only new one is test_circular_imports.py, which appears to be because the C extensions aren't there? I'm pretty sure we didn't touch anything there in 3.9.1...
test_send_compress_text is still failing though. I'm not sure what's going on with that one. Would be great if you can dig into it a little. Maybe @bdraco has some ideas on how that could be failing.
the only new one is test_circular_imports.py, which appears to be because the C extensions aren't there?
We discussed these near the top of the thread actually: https://github.com/aio-libs/aiohttp/issues/7255#issuecomment-1511704934 So, there's no new failures here really.
test_send_compress_text is still failing though. I'm not sure what's going on with that one. Would be great if you can dig into it a little. Maybe @bdraco has some ideas on how that could be failing.
Maybe the compression results are different in your version of zlib? Maybe check what zlib.ZLIB_RUNTIME_VERSION
is?
Well, other than import_time, which is probably just some random thing with your setup that caused it run slowly that time, the only new one is test_circular_imports.py, which appears to be because the C extensions aren't there? I'm pretty sure we didn't touch anything there in 3.9.1...
May you explain what exactly it means and/or how van I try to diagnose that? 🤔
Maybe the compression results are different in your version of zlib? Maybe check what
zlib.ZLIB_RUNTIME_VERSION
is?
FYI I'm using zlib-ng.
The circular_imports tests appear to be failing to import. I'd compare with how our CI runs it (https://github.com/aio-libs/aiohttp/blob/master/.github/workflows/ci.yml#L128), but I suspect you'll need to install the package or tweak PYTHONPATH to get it to work correctly.
Then it looks like there is a DNS error, maybe that test should be mocked or something...
Cannot find what exactly needs to be resolvable here 🤔 So fat on scale of +12k of other build modules literally none had any DNS resolver issue.
Generally looking on test suite code if it would be anything related to module DSO much more units would be failing 🤔
FYI I'm using zlib-ng.
Well, I think that solves that mystery. Not sure if we can do anything with that test...
So fat on scale of +12k of other build modules literally none had any DNS resolver issue.
Oh, I just assumed you'd disabled internet access. Maybe the problem is something else...
Generally looking on test suite code if it would be anything related to module DSO much more units would be failing 🤔
Yeah, we went over it before. It has something to do with running python in a subprocess. When run in a subprocess, your system is failing to import the modules. Maybe it's similar to the tests/__init__.py
issue, and only works when the package is installed? If you can figure out how to get it working, and can recommend a change to those tests, we'll take a look.
FYI I'm using zlib-ng.
Well, I think that solves that mystery. Not sure if we can do anything with that test...
Last issue with zlib-ng was with perl +half year ago. Nothing else on scale 5.6k packages have aby issues. In other words it probability is still non-zero however IMO it is really low. How can I try to diagnose this to remove this cause from the list? 🤔
So fat on scale of +12k of other build modules literally none had any DNS resolver issue.
Oh, I just assumed you'd disabled internet access. Maybe the problem is something else...
Generally looking on test suite code if it would be anything related to module DSO much more units would be failing 🤔
Yeah, we went over it before. It has something to do with running python in a subprocess. When run in a subprocess, your system is failing to import the modules. Maybe it's similar to the
tests/__init__.py
issue, and only works when the package is installed? If you can figure out how to get it working, and can recommend a change to those tests, we'll take a look.
Correction +1.2k modules
[tkloczko@pers-jacek SPECS]$ ls python-*.spec -1 | wc -l
1213
I'm building in two sets of envs. Final prod builds are in without routing and DNS si available. Copied here pytest result was from staging env where is full public network access.
I expect zlib_ng
works fine as I use it in production https://github.com/bdraco/aiohttp-zlib-ng Its not unexpected that the raw bytes will be different on zlib_ng
.
The test should probably be changed to uncompress the result instead of verifying the raw bytes.
The test should probably be changed to uncompress the result instead of verifying the raw bytes.
That's one option. But, we did see a PR recently where the change in exact bytes was a red flag (as the proposed change would reduce compression effectiveness), so would be nice to keep.
Just FTR ..
Looks the same. We've reverted the use of time-machine in master, which will get the cookiejar tests back if you're still not going to bother installing that dependency in the meantime.
BTW testing circular imports .. pylint can be used for that https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/cyclic-import.html
Nevertheless I think that this test should not be parto of the test suite but more likely CI tests. Repeating that kind of test by many distro maintainers can be avoided by check one time the code and add that kind of test as part of the CI of the qualification tests of any commit or PR.
I explained somewhere already that those tests are important and correct.
I explained somewhere already that those tests are important and correct.
Really .. the best would be used for that just pylint
and only in CI and not in actual test suite.
Updated pytest output of the 3.9.5 with python 3.10.14 and pytest 8.1.1 with excluded units checking circular imports
Describe the bug
pytest is failing in 9 units.
To Reproduce
I'm packaging your module as an rpm package so I'm using the typical PEP517 based build, install and test cycle used on building packages from non-root account.
python3 -sBm build -w --no-isolation
build
with--no-isolation
I'm using during all processes only locally installed modulescut off from access to the public network
(pytest is executed with-m "not network"
)Expected behavior
Pytest should not fail.
Logs/tracebacks
Python Version
aiohttp Version
multidict Version
yarl Version
OS
Linux x86/64
Related component
Server
Additional context
Here is list of installed modules in build env
Code of Conduct