[BLOCKED] --import-mode=importlib: use sys.path relative path if present (wait for #7870)

[davidhewitt]

This PR is inspired by #5147 (comment) to use sys.path to discover package paths when attempting to import test modules in importlib mode.

This helps weird edge-case papercuts such as:

• relative imports in test files, which currently doesn't work with importlib mode (see Feedback about --import-mode=importlib in pytest 6.0 #7245 (comment))
• implicit namespace packages where the child package name clashes with a builtin module, which I have issues with on all modes (not just importlib).

This patch only takes effect if the file being tested is in the user's sys.path. I think that usually this is only the case if the containing package was installed with setup.py develop.

I'm happy to write docs, changelog entry etc., but first wanted to collect feedback on this approach before doing so.

---

Here is a quick checklist that should be present in PRs.

• Include documentation when adding new features.
• Include new tests or update existing tests when applicable.
• Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

• Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

  Write sentences in the past or present tense, examples:

  • Improved verbose diff output with sequences.
  • Terminal summary statistics now use multiple colors.

  Also make sure to end the sentence with a ..

• Add yourself to AUTHORS in alphabetical order.

[bluetech]

Could you add a comment above here, explaining with this code block is needed? IIUC, what it does is "If the path is importable as a name from the search path, then import it as such.". But the comment should say why it's done.

Also, AFAIK (not 100% sure) the previous importlib code doesn't insert the imported module into sys.modules, but importlib.import_module does. I think the idea was to not insert it, as mentioned in the --import-mode=importlib changelog entry:

Traditionally pytest used __import__ while changing sys.path to import test modules (which also changes sys.modules as a side-effect), which works but has a number of drawbacks, like requiring test modules that don't live in packages to have unique names (as they need to reside under a unique name in sys.modules).

--import-mode=importlib uses more fine grained import mechanisms from importlib which don't require pytest to change sys.path or sys.modules at all, eliminating much of the drawbacks of the previous mode.

[davidhewitt]

Sure thing, very happy to add a comment. I'll push it as a separate commit.

Also, AFAIK (not 100% sure) the previous importlib code doesn't insert the imported module into sys.modules, but importlib.import_module does.

This is true. I noticed that #7870 was actually proposing to change that though so that the imported module is added to sys.modules (to fix the issues noted there).

An interesting thing to note about this approach is that the entry that ends up in sys.modules will be the same as if import foo.bar.test_baz was executed - e.g. sys.modules["foo.bar.test_baz"].

With the approach in #7870 the concern is that test modules which have conflicting names will again have issues, as described in the changelog entry you quote. (The code in #7870 would add just test_baz to sys.modules, for example.)

This approach would add the fully qualified name foo.bar.test_baz to sys.modules, which may be interesting as an alternative approach to the fix in #7870.

[bluetech]

Thanks for the explanation.

So the approach does sounds appealing, but it does bring us again to have a dependence on sys.path and how it is configured (e.g. if it contains the test directory, at what depth, etc.). Basically it becomes pretty similar to --import-mode=append, no? Just without keeping the sys.path dirty.

[davidhewitt]

Yes, that's a very fair assessment of this approach.

Given that it's perhaps markedly different from the importlib mode, perhaps it makes more sense to make this a new mode, maybe called path?

That way it'll have reliable behaviour - if the test file is not underneath a path entry, pytest will not be able to import it. So users who want to test using this mode will have to add the test files to their path (usually by setup.py develop).

The advantages of it are primarily for test files inside packages, so I don't think that's an unreasonable requirement.

[nicoddemus]

Thanks @davidhewitt for the patch! This is complicated topic so we are very grateful for the attempt.

This is an interesting approach, but doesn't handle one of the main uses of --importmode=importlib, which is using it to collect test files which are not in sys.path at all. Consider this common layout:

src/
   analysis/

tests/
   io/
      test_measurement.py
   model/
      test_measurment.py 

In this case src is added to sys.path due to python setup.py develop (or adding to $PYTHONPATH manually), however we don't have any parent of tests/io/test_measurement.py and tests/model/test_measurement.py in sys.path, which wouldn't trigger your patch and would still cause the issues with pickle/dataclasses mentioned in the original issue.

Your patch would work fine though when the tests are siblings of an entry in sys.path, which is common for "in-source" test layouts:

src/
   analysis/
     /io
       /tests
         test_measurement.py
     /model
       /tests    
         test_measurment.py 

But again this is not one of the use cases that importlib mode attempts to solve.

Given that it's perhaps markedly different from the importlib mode, perhaps it makes more sense to make this a new mode, maybe called path?

That's sensible, but I'm not sure it is the best approach. Supposing we can fix #7870, what would be the advantage of path over importlib?

[davidhewitt]

As well as #7870 I have a separate issue with importlib mode, which is that relative imports no longer work in importlib mode. This patch originated as a attempt to figure out how to get __package__ correctly set as suggested at #7245 (comment).

I'm happy to close this PR if it doesn't look like the right solution.

[nicoddemus]

@davidhewitt again thanks for the patch!

Let's keep this open until we get #7870 working (even if only as a reference): we can then see if it also solves the relative import problem.

[davidhewitt]

👍

Note that the test case I've written here is particularly awkward but is amazingly also a real-world use case I'm suffering with which motivated me to write the PR.

I'm attempting a relative import from a test file that is inside a pep 420 namespace package, where the first non-implicit package is called csv, and so collides with the builtin package of the same name.

Interestingly the append mode also cannot handle this case correctly, so I was wondering if this patch could also be relevant for the append mode.

[nicoddemus]

#7870 has been merged, do we still need to revisit this?

[Zac-HD]

Since this hasn't been updated in years, I'm going to assume that it's stale and close it - but if the issue persists, it would be fantastic to rebase and open a new PR 🙏