Closed kloczek closed 12 months ago
After add --pyargs trio
to pytest params pytest passed collecting units however many units are failing with the same error message pytest.PytestUnhandledCoroutineWarning: async def functions are not natively supported and have been skipped.
Two quick questions! (I'm not on desktop so I can't test anything, therefore the questions may be dumb):
I think the error message is saying something about a different in /BUILDROOT
vs /BUILD
and I'm struggling to realize where that might come from or what might have changed between versions.
Oh, we moved test plugin out which handles async def functions -- check ci.sh
for the specific pytest invocation we use (we run the tests of an installed version of trio!)
I see that few units are failing because missing ruff
in build env (please ignore those unit as I'll try to add this module to build env.
However all other units are failing pytest.PytestUnhandledCoroutineWarning: async def functions are not natively supported and have been skipped.
despite that I have installed pytest-trio
.
Oh no -- we have our own pytest plugin we use ourselves 😰
(Not sure how it differs from pytest-trio, though)
Two quick questions! (I'm not on desktop so I can't test anything, therefore the questions may be dumb):
- Does this happen in 0.22.2?
Last version which was OK Which I have already packaged is 0.22.0.
- Could you provide a specific script that when run in a Docker container (? or non-Docker if this is generic to any Linux system) fails?
Hmm .. I'm not using docker. On building packages I'm using LXC zones because it provides full system with all system components started over systemd inside zone lik it would be bare metal system 🤔
Oh no -- we have our own pytest plugin we use ourselves 😰
(Not sure how it differs from pytest-trio, though)
So I'm assuming that this message shows incorrect advice (?) 🤔
I'm not sure what exactly you mean by that but yeah it's likely pytest is assuming we're the standard user of async def
functions (some async library) rather than an event loop, if that's what you're referring to ^^
In CI, we run this pytest command (in coverage
):
pytest -r a -p trio._tests.pytest_plugin --junitxml=../test-results.xml --run-slow ${INSTALLDIR} --verbose --durations=10 --skip-optional-imports
(we optionally add --skip-optional-imports
but that makes it so the tests shouldn't require e.g. ruff
)
Maybe the problem is just that our invocation of pytest is just really annoying to get right :(
Oh, we moved test plugin out which handles async def functions -- check
ci.sh
for the specific pytest invocation we use (we run the tests of an installed version of trio!)
I see in that script only:
echo "::group:: Run Tests"
if COVERAGE_PROCESS_START=$(pwd)/../.coveragerc coverage run --rcfile=../.coveragerc -m pytest -r a -p trio._tests.pytest_plugin --junitxml=../test-results.xml --run-slow ${INSTALLDIR}
--verbose --durations=10 $flags; then
PASSED=true
else
PASSED=false
fi
First issue which I see here is running pytest as -m pytest
.
AFAIK pytest should not be executed that way because when python interpreter executes module over python -n foo
it always adds current directory to sys.path
.
This is why pytest provides shell wrapper script pytest
.
Yeah we run that command in an empty directory to avoid any silly sys.path
schenanigans.
I think that instead putting tall pytest params in shell scrip all should be in pytest.ini file to allow use pytest without any additional params.
Yeah we run that command in an empty directory to avoid any silly
sys.path
schenanigans.
So instead that it should be used pytest
.
pytest maintainer many times mention in comments that pytest should not be executed by -m pytest
😋
We actually do use the configuration for pytest! However, stuff like --slow
and other flags change runtime things and running a bunch of tests in CI is different than running a single test locally.
... Though TBH the -p
flag should probably go into the pyproject.toml
if possible.
As for running -m pytest
IIRC that was what worked but I would welcome a PR to fix that! I actually didn't know about the sys.path
differences, we do the empty directory thing because it removes that entire class of footguns from the start.
If all necessary pytest params will be placed in pytest.ini I think that ci.sh could be removed and everything else could be moved directly into gh action/workflow 🤔
That was the plan in the past but we have in the past used multiple CI providers in order to test e.g. freebsd. However, those providers stopped working, but the main method to test on e.g. freebsd is this qemu action step that takes in a shell script to run!
We actually do use the configuration for pytest! However, stuff like
--slow
and other flags change runtime things and running a bunch of tests in CI is different than running a single test locally.... Though TBH the
-p
flag should probably go into thepyproject.toml
if possible.
Yeah .. just fount [tool.pytest.ini_options]
in pyproject.toml
😋
OK after add -p trio._tests.pytest_plugin
to pytest params I have now only failing units because missing ruff
😄
--pyargs trio
still is necessary so looks like in [tool.pytest.ini_options]
in addopts
in pyproject.toml
it would be good to add --pyargs trio -p trio._tests.pytest_plugin
.
Nevertheless .. thank you for the hints 👍 😄
BTW I see that trio
is using relative imports which using is in many cases only asking for troubles (and adds additional syscall to obtain current directory name)
Just tested pyest with my patch which replaces all relative imports.
If you want I can submit that as PR.
Hopefully ruff
isn't required if you pass --skip-optional-imports
-- if not, let's fix that!
Also, I'm not sure a patch that changes all imports to be relative (assuming I'm not mistaken about what you mean) would be accepted as that feels too far on the micro-optimization end of things. However, updating pyproject.toml
would be a nice PR to do! (If you don't in a few days and I remember, then I'll make one).
I'm going to close this issue cause it sounds like you figured out the issues, but feel free to reopen if I misunderstood.
Also, I'm not sure a patch that changes all imports to be relative (assuming I'm not mistaken about what you mean) would be accepted as that feels too far on the micro-optimization end of things.
I think they're suggesting the reverse - only using absolute paths and not relative. If the reason for either way is sufficiently solid and we could enable a check for it - ideally an autofix, I wouldn't mind getting rid of the current mess where some things are imported relative and some aren't and there's often no reason why one is used over the other. A stronger case is probably to check that the expected symbols are publicly exported.
I think they're suggesting the reverse - only using absolute paths and not relative.
In case of modules with DSO it causes especially nasty issues on build because it forces to build module in place which is not supported by pep517 build procedure. Please have a look on random core pypi modules like pytest, sphinx, setyptools or any pep517 backend. None of those modules are using relative imports.
Other thing is that in your code is possible to find multiple import of parts of the same module which makes code longer.
As I wrote bu use relative import you are forcing python to obtain current directory and calculate module import path.
With use only full paths all what is done is checking where module is in sitearch or sitelib.
As this module has no DSOs it is installed in sitelib so proper path is done on first hit by load module from sitelib + module_name_py
.
It is yet another issue that test suite is not separated from module code and it is installed with actual module code. Normal practice is placing test suite in tests/ subdirectory in source tree. This directory by default is excluded from install list..
I'm pretty sure that literally nothing you just said is accurate :-(. If anything, relative imports create less work for the import system, because the full path is known immediately, without having to scan sys.metapath
and sys.path
.
Example
As you seee there is no even SINGLE relative import.
So, there is a ruff check for relative imports: https://docs.astral.sh/ruff/rules/relative-imports/
And some looking around seems to suggest that absolute imports are slightly preferred nowadays, any big problems with it in the past have been corrected: https://bugs.python.org/issue10031
I'd personally be in favor of a reformatting PR that enables TID252, it might catch stuff not being exported and clean up imports a bit. This isn't related to this issue though, so feel free to open a separate issue or create that PR though.
It is yet another issue that test suite is not separated from module code and it is installed with actual module code. Normal practice is placing test suite in tests/ subdirectory in source tree. This directory by default is excluded from install list..
See #274
ruff
produces DSO and it causes that it cannot be tested without installing because using relative imports FORCES to have DSO in the the same directory where is rest of the .py files.
trio
and ruff
do not have complicated tree layout and has no DSO so looking on ruff
does not make to much sense.
As I wrote .. jus please try to check few more examples of most frequently used python pypi modules if pytest
is not enough for you.
ruff
additionally is nastier because it sued own fork of libCST.
Looks like maintained is not able to use standalone libCST binaries.
I've already discussed that and did not receive clean justification of that approach.
Looks like complexity is only result of the approach of use libCST.
As I wrote, additional side effect of use relative imports is forcing python to call additional syscalls to obtain absolute path to exact imported module and no other benefits ..
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"
)All fails with
remove __pycache__ / .pyc files and/or use a unique basename for your test file modules
. Here is pytest output:Here is list of installed modules in build env