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

Make “good practices” match the future default import mode #8110

Closed
1 task done
flying-sheep opened this issue Dec 9, 2020 · 13 comments
Closed
1 task done

Make “good practices” match the future default import mode #8110

flying-sheep opened this issue Dec 9, 2020 · 13 comments
Labels
type: docs documentation improvement, missing or needing clarification

Comments

@flying-sheep
Copy link
Contributor

flying-sheep commented Dec 9, 2020

  • a detailed description of the bug or problem you are having

The import mode docs say that the importlib mode will be the future default, and test modules will be unable to import each other.

That’s great, and promotes a distinction between my_package.test_utils and test_*.py test collections!

I think in the good practices docs, this should be outlined more clearly, and the drawbacks of the prepend import mode should be deemphasized while promoting the importlib mode.

Good practices should include not adding __init__.py to test directories:

  • If you want to ship the tests in your package, they should be included similarly to data.
  • If you separate tests and package, __init__.py is just a workaround for the prepend import mode, which should be phased out.
@nicoddemus nicoddemus added the type: docs documentation improvement, missing or needing clarification label Dec 9, 2020
@jaraco
Copy link
Contributor

jaraco commented Dec 22, 2022

I have a similar situation that I'm not sure is addressed by the proposed recommendation. Consider the jaraco.abode project, which has a test suite at tests/. When applying import-mode=importlib, the modules in tests/* are unable to access tests/mock/*. I'd like for the modules in tests/mock/* to be accessible to the tests, but I don't want to distribute them with the package (they're only relevant when running tests, and feedback I've gotten from downstream repackagers is that the test suite should not be present in the installed package). What's the recommended solution for such a use-case under import-mode=importlib?

@RonnyPfannschmidt
Copy link
Member

Currently importlib won't be the default due to these issues

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Dec 22, 2022

I wouldn’t frame this as issues, Ronny. It’s just behavior that differs from how people have done things. Behavior that enforces better practices, but isn’t widespread enough for better patterns to be widely used. Which is enough to be careful when changing things but isn’t an issue.

@jaraco you just have to get those modules imported, either by getting them into PYTHONPATH or by manually importing them. E.g. by doing one of the following:

  1. move them into your module, e.g. as jaraco.abode.testing.mock or jaraco.abode.test_mock_utils or so, or

  2. move them under another top level module such as testing.jaraco.abode (testing is a global namespace package containing test utils for some systems or packages), or

  3. import them via path, e.g. like this:

    tests/conftest.py

    # all fixtures defined in tests/conftest.py are available in all tests.
    here = Path(__file__).parent
    
    @pytest.fixture(scope='session')
    def mock_module():
        spec = importlib.util.spec_from_file_location('jaraco.abode.__mock', here / 'mock')
        module = importlib.util.module_from_spec(spec)
        spec.loader.exec_module(module)
        return module

    tests/some_tests.py

    def test_something(mock_module):
        assert mock_module.FOO == 'foo'

Think about how things are now: Many people basically define a separate global tests package, that python will know as sys.modules['tests'] with pytest’s default import mode. Some of them are accidentally included in the distribution and installed in your venv, where they cause havoc when you try to run your own tests. With --import-mode=importlib and the above solutions, that’s impossible: sys.modules won’t contain a tests module, which makes things much easier.

@jaraco
Copy link
Contributor

jaraco commented Dec 22, 2022

Thanks flying-sheep for the detailed response.

Option 1 is a non starter. As I stated above, downstream packagers have specifically requested that tests not be included with the package. Over time, this has driven me from storing tests within the package to storing them in ./tests and excluding them as a matter of course.

I'm a little unsure what option 2 is. Are you suggesting that ./tests is not viable (because others accidentally package that module) but ./testing is? I can explore this approach, but I'm skeptical that it's not achieving my expected goals.

Option 3 is dreadful. Not only does it require adding another boilerplate module to each project, but it requires a custom fixture for every possible functionality that needs to be available to the tests, specific to each test suite. This approach is far from sustainable, especially compared to the status quo (where a test suite can have importable, shared functionality).

Currently importlib won't be the default due to these issues

That's understandable, but currently the only solution to #3396 (closed) is to use import-mode=importlib, so it doesn't matter if it's default or not, I (and all packages with native namespace packages) are forced to use it.

jaraco added a commit to jaraco/jaraco.abode that referenced this issue Dec 22, 2022
jaraco added a commit to jaraco/jaraco.abode that referenced this issue Dec 22, 2022
@jaraco
Copy link
Contributor

jaraco commented Dec 22, 2022

I'm a little unsure what option 2 is. Are you suggesting that ./tests is not viable (because others accidentally package that module) but ./testing is? I can explore this approach, but I'm skeptical that it's not achieving my expected goals.

I tried this recommendation out in jaraco.abode, and sure enough, it seems to be mostly working. Two tests are failing due to some obscure edge case(s), but for the most part, moving tests to testing seems to allow the use of import-mode=importlib. It's weird and annoying, but maybe the solution here is to rename tests to testing across all projects.

The edge cases are rather obscure, and may be pointing to a misconception on my part about the approach.

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Dec 22, 2022

Option 1 is a non starter. As I stated above, downstream packagers have specifically requested that tests not be included with the package.

Well, you’d include test utils with the package. The actual tests aren‘t included. You’d have

  • <pkgroot>/jaraco/abode/{automation,cli,testing,...}, (which will be all distributed) and
  • ./tests/{conftest.py,test_foo.py,...} (which won’t be distributed)

I'm a little unsure what option 2 is. Are you suggesting that ./tests is not viable (because others accidentally package that module) but ./testing is?

Yes, testing is a well known namespace package containing submodules that provide test utils. Nobody is supposed to package a <pkgroot>/testing/__init__.py, but people do package <pkgroot>/testing/<mymodule>. In your case <pkgroot>/testing/jaraco or <pkgroot>/testing/jaraco/abode.

@jaraco
Copy link
Contributor

jaraco commented Dec 22, 2022

Oh. When I added exclude=testing, the tests fail the same way as they did when tests were under ./tests, so it seems the issue is that pytest is requiring the packages to be installed.

@flying-sheep
Copy link
Contributor Author

You replied at the same time as my above comment, please read what I wrote carefully.

If you don’t want to use the import-via-path solution, you need to either install your test utils alongside the package before running the tests or add your test-utils-module’s containing directory to sys.path in another way.

@RonnyPfannschmidt
Copy link
Member

@flying-sheep those are issues as they break existing testsuites and depending in the setup it imposes hours of work and introducing own custom cargo culted sys.path hacks to hopefully get testing working, while still breaking a number of ways one would generally expect to import code

as long as relative imports between test modules and relatively located test helper modules are not working its not something that can be used as a default as it just wrecks loads of things

@flying-sheep
Copy link
Contributor Author

If you use --import-mode=importlib for a new project from the start, you’ll automatically have to organize things in a way that works, e.g. by using a mypackage.test_utils/testing.mypackage module, or by using fixtures instead of test utility modules. You’ll stop seeing the contents of tests folders as Python modules, and automatically avoid trying to do relative imports.

Nothing breaks unless you deliberately migrate to a different import mode for an existing project. And each migration is effort if you choose to do it, yes.

@flying-sheep
Copy link
Contributor Author

@jaraco you misinterpreted my suggestion when doing jaraco/jaraco.abode@446a488

  • /testing/mock/ would be the location for test utils for https://pypi.org/project/mock/, you should put your test utils into /testing/jaraco/abode/ or /jaraco/abode/testing/ or /jaraco/abode/test_utils/ or so.
  • /testing/ is not where you put any tests, they still go into /tests/

@jaraco
Copy link
Contributor

jaraco commented Dec 25, 2022

I think I understand now - with import-mode=importlib, the test suite is not allowed to have any importable behavior that's not installed with the package. I guess that's my gripe. I'd like for there to be a way to import behavior for tests but not require that behavior to be installed with the package. All three options above require moving the mock behavior into the installed package, behavior that's currently only distributed with the sources.

There was a time when the prepend import mode did not support importable behavior not in the package. Because of this, I developed a habit of always including the tests in the main distribution package (installed), so they were importable and able to be modularized. Later, pytest started supporting importable behavior from test packages, making it possible to have a test suite with shared modules not installed as part of the package. Later, users started requesting that I move this behavior out of the package so that it's not installed (now considered a best practice). I'd like not to have to return to that prior expectation unless pytest is willing to defend that position (that all importable behavior must be installed).

@flying-sheep
Copy link
Contributor Author

Fixed by #10206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: docs documentation improvement, missing or needing clarification
Projects
None yet
Development

No branches or pull requests

4 participants