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

Avoiding source-tree imports during CIBW_TEST_COMMAND #1534

Open
joerick opened this issue Jun 23, 2023 · 7 comments
Open

Avoiding source-tree imports during CIBW_TEST_COMMAND #1534

joerick opened this issue Jun 23, 2023 · 7 comments

Comments

@joerick
Copy link
Contributor

joerick commented Jun 23, 2023

Continuing from #1530 (reply in thread)

It seems that pytest defaults to 'prepend' import mode, meaning that for a source tree without a /src folder, like this

  • mymodule
    • __init__.py
    • main.py
    • extension.c
  • tests
    • test_main.py

Even when invoked as cibuildwheel does, with the wheel installed in a venv, with cwd as a random temp dir, the mymodule folder in the source tree is higher in the import order than the installed wheel.

This is a pity, the intention of changing the cwd during the test-command was to prevent this problem. I'm trying to think if there's a way that cibuildwheel can work around this...? Obviously the /src structure is best, but we can't mandate that for all cibuildwheel projects.

Another thing that I do wonder is if it's even worth changing the cwd for the test-command given this pytest default - it's a common gotcha within cibuildwheel, and if the above is universally true it's not even helping...!

@joerick
Copy link
Contributor Author

joerick commented Jun 23, 2023

off the top of my head, could we

  • manipulate PYTEST_ADDOPTS to add --import-mode=. I think importlib this won't work for many projects' tests, where they have imports like from tests.utils import foo. Perhaps we could use append - pytest docs even recommend this for testing against installed versions of packages
  • Somehow isolate test dirs from code dirs before running test-command? e.g. copy them out-of-tree before running
  • A pytest plugin built into cibuildwheel that further manipulates sys.path to ensure the wheel install is at the top
  • Somehow detecting when tests [might] import source-tree files and presenting a warning at runtime. I'm not sure how we could detect this though... (I suppose we could inspect the wheel and look for modules that have the same name as folders in the project dir. But that assumes the test path is on the first level of the project, I think?)

I'll keep pondering this. Ideas welcome.

@golmschenk
Copy link

As this in part spawned from the discussion I started, I wanted to note that the solution of using import mode importlib did work for my minimal test case, but as noted above, it broke for my real world case where the tests directory had more complex internal imports. I now chose to switch to the src structure for my real world case.

@henryiii
Copy link
Contributor

That's why they haven't activated importlib by default - it's too different from the old hacks. In general, you should not assume your test directory is importable, and use pytest 7's new pythonpath= setting to add things you want to import inside your test directory.

IMO, support for non-src directory layout is a Python and pytest bug, and not ours. Even if cibuildwheel "fixes" this, it's still broken outside of cibuildwheel, so there still will be hidden landmines for users. I'm not really even a big fan of changing the path we do, and once Python <3.11 support is dropped, I could see recommending we set PYTHONSAFEPATH and remove the directory switch.

Here's a report today about a similar issue that has nothing to do with cibuildwheel: scikit-build/scikit-build-core#406

I don't think we should try to dig ourselves even deeper in trying to fix buggy package design by assuming people are using pytest and muddling with pytest settings. I think we could document that adding __init__.py to tests is a bad idea unless importmode=importlib is set, and pytest 7 has a new pythonpath= setting that can be used to import helpers inside tests.

@joerick
Copy link
Contributor Author

joerick commented Jun 25, 2023

Yeah... on reflection these seem too hacky to be useful.

I'm now thinking that instead we add a note to the test-command docs, to the effect:


  • If your project keeps python code within a /src folder, good news, the simple config will work great.

  • if your project keeps python module code at the top-level, and you use pytest, there are some gotchas. During this step, we want to run your tests against the installed wheel. But Pytest's default import mode prepend adds the parent of your test folder to the python import path, which means that when your tests do import mymodule, they import from the source-tree files, not the installed wheel.

    There are a few ways around this:

    • (preferred) Change your project to store module code in /src. This prevents a lot of import-based errors, and is now recommended by the pypa for new projects.
    • When invoking pytest, use the option --import-mode=append - this mode means that the wheel will appear in the import path above the source-tree version, so will

At this point i'm a little stuck - actually --import-mode=append doesn't work at all for me, because the test module is shadowed by python built-in modules (test and unittest look like pretty problematic names in this respect). So I suppose --import-mode=importlib would be the other recommendation, but then I don't understand how one could import test utils in this scenario.

I think we could document that adding __init__.py to tests is a bad idea unless importmode=importlib is set, and pytest 7 has a new pythonpath= setting that can be used to import helpers inside tests.

What did you mean by the 'adding __init__.py' here @henryiii ? Why is that a bad idea for imports? My understanding is that it's required to import utils within tests. I'm also not sure how the new pythonpath option would help here...

@henryiii
Copy link
Contributor

henryiii commented Jun 25, 2023

I'd add as a recommendation you don't use "test" as the name for your test folder, but always "tests". Solves that problem. :)

This would be the structure for test utils:

tests/utils/importme.py

No test/__init__.py. Then you have:

pythonpath = ["tests/utils"]

Then inside your test code you import importme. (I think this would also work by adding test to the pythonpath=, and not having it nested, but nesting is nicer for organization anyway.)

The downside to this is it's not great for IDE's, but they have a setting (VSCode does, anyway) to add extra folders to a project's path, they just don't pick up on this one. Adding tests directly might work, though, since they will assume same-directory imports work.

@henryiii
Copy link
Contributor

(Since I always use src layout and tests folders, I haven't seen this enough to have a good feel for how well it works, I've so far only used pythonnpath= to add a pytest local plugin).

@webknjaz
Copy link
Member

Have you tried python -Im pytest? That -I unsets anything that might've gotten into PYTHONPATH (including counteracting the effect of runpy / -m that adds CWD there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants