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

Improve 'module not found' message #10569

Open
nicoddemus opened this issue Dec 8, 2022 · 10 comments
Open

Improve 'module not found' message #10569

nicoddemus opened this issue Dec 8, 2022 · 10 comments
Labels
type: docs documentation improvement, missing or needing clarification

Comments

@nicoddemus
Copy link
Member

nicoddemus commented Dec 8, 2022

In #1567 it was men, user has spent a long time trying to figure out why their namespace package was not being found, reason being that pytest does not support namespace packages via --pyargs (#478).

One easy suggestion is to improve the error message, from:

ERROR: module or package not found: module.misc (missing __init__.py?)

To something like this:

ERROR: pytest cannot import module or package 'module.misc'
Possible reasons:
- Missing an __init__.py file
- Namespace package (not supported)
- Not reachable via PYTHONPATH
See <link to docs> for more information.

Bonus points if we link to a documentation where we have an explanation for each bullet point above.

Originally posted by @nicoddemus in #1567 (comment)

@nicoddemus nicoddemus added the type: docs documentation improvement, missing or needing clarification label Dec 8, 2022
@dimpase
Copy link

dimpase commented Dec 8, 2022

Are there solid technical reasons for not supporting PEP 420, or it's just lack of labour?

@nicoddemus
Copy link
Member Author

Are there solid technical reasons for not supporting PEP 420, or it's just lack of labour?

TBH the details are murky, I recall a very long discussion involving pytest maintainers and other members of the community (like @jaraco), and IIRC we could not arrive to a solution on how to support this in pytest.

Others who remember more details and/or have links to share please do so.

Some refs:
#478
#3396

@dimpase
Copy link

dimpase commented Dec 8, 2022

Our (for SageMath) main use of pytest would be to run doctests (in Python and in Cython). This can be done on per-file basis, in parallel (otherwise it takes many hours of wallclock time). But pytest makes it unnecessary hard to do things like pytest foo/*/* -- all these missing __init__.py errors. Currently we have a homemade tool which uses a bit of Python doctest module for this, but it's old and tired, and we really want an off the shelf solution instead.

@dimpase
Copy link

dimpase commented Dec 9, 2022

We'd like to ask for an option to provide a package discovery hook for namespace modules/packages (e.g. in our case (our homegrown doctest system also does Python module discovery, after all). That is to say, a pytest-approved way to monkey-patch https://github.com/pytest-dev/pytest/blob/main/src/_pytest/_py/path.py @mkoeppe

@RonnyPfannschmidt
Copy link
Member

there will never be a way to monkeypatch this legacy path thing that we will remove as soon as we can as its only vendor-ed due to a need to drop pylib asap to begin with

as for what to do about the discovery of homegrown doctest, thats something else

pep440 awareness should help to fix most of the issues, for anything else we need to see the use-case to properly figure what api is necessary to provide

@RonnyPfannschmidt
Copy link
Member

@nicoddemus i believe the key issue is that python collection currently works in terms of files, to properly collect in namespace packages, python collection needs to be module/package/import aware

as this would in general be a prerequisite for package shippable testsuites it may be a good idea to focus on "pythonic" python collection in then ext year

@dimpase
Copy link

dimpase commented Dec 9, 2022

or anything else we need to see the use-case to properly figure what api is necessary to provide

in SageMath directories containing namespace packages are recognised by presence of all.py or an all_*.py file (for some value of *). E.g. this contains a namespace package, as it has all.py (all.py may also contain import and lazy_import statements). Whereas that does not contain a Python package.
(there are also "classical" packages, with __init__.py, present).

@dimpase
Copy link

dimpase commented Dec 9, 2022

as for what to do about the discovery of homegrown doctest, thats something else

Our doctests are more or less standard, and in fact pytest is already mostly able to run them, on the level of individual files. That's not what we are worried about.
It's the framework to test them that's homegrown, and to be replaced by pytest.

@dimpase
Copy link

dimpase commented Dec 9, 2022

there will never be a way to monkeypatch this legacy path thing

OK, I see. Is this the place responsible for collecting tests pythonically?

def collect(self) -> Iterable[Union[nodes.Item, nodes.Collector]]:

Is it stable - so that we can monkey-patch it without a fear that it'll break soon?

@RonnyPfannschmidt
Copy link
Member

No promises

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

3 participants