Excluding test code with --verifytypes feature

[erictraut]

Tagging @jorenham and @MarcoGorelli

This discussion is a follow-up to this thread.

Joren, you mentioned in that thread that it would be useful if the --verifytypes feature in pyright allowed for exclusions (e.g. for test code). The --verifytypes feature is intended to run on the installed library, and most libraries don't ship test code with the package. It looks like numpy does bundle some of its tests. I see a tests and a testing directory. I presume this is intentional and not just a packaging oversight?

In a few other cases where library authors asked me about packaged test code, I recommended that they rename their test directory to _tests. The underscore is the standard mechanism that tells tools that the directory is considered private — not part of the public interface of the library. If it's named tests (no underscore), then language servers will offer completion suggestions (including auto-import completions) that target this directory. That's probably not ideal for users. If there's good reason to ship tests with numpy, what are your thoughts on renaming the directory to _tests? Or would that be too disruptive?

You mentioned in the other thread that adding support for some exclusion mechanism for --verifytypes would help here. Leveraging the existing exclude probably doesn't make sense because that setting is intended to define which source files are in your project, but --verifytypes doesn't work on project code; it operates on the code that is installed in your Python environment. I guess that I could add a separate exclusion mechanism, but that still wouldn't address the fact that language servers would still think that tests is part of the public interface for the library. Maybe we need some other industry-standard way for a library to indicate which subdirectories or submodules are considered "public". I previously explored expanding the contents of the "py.typed" file for this purpose, but the feedback from the community was pretty negative. They didn't want to complicate "py.typed".

You also mentioned that you're looking for some way to avoid back-sliding on progress toward type completeness. I wonder if you've considered some sort of diffing mechanism here. The --verifytypes command can be combined with --outputjson, which allows the resulting report to be processed easily by scripts. A CI script could do a before-and-after comparison and use the diff to look for regressions. Just a thought.

[jorenham]

It looks like numpy does bundle some of its tests. I see a tests and a testing directory. I presume this is intentional and not just a packaging oversight?

The numpy.testing subpackage is part of the public API, and contains functions such as assert_allclose that can help users write tests involving numpy arrays.

After asking about it, it turns out that the tests directories are bundled is so that users can use np.test() to verify that numpy has been installed correctly (e.g. when it has been manually compiled).

If there's good reason to ship tests with numpy, what are your thoughts on renaming the directory to _tests? Or would that be too disruptive?

Hmm I'm probably not the best person to ask about this, so perhaps @rgommers could help us out here?

I previously explored expanding the contents of the "py.typed" file for this purpose, but the feedback from the community was pretty negative. They didn't want to complicate "py.typed".

Haha, that very same idea popped into my head when doing groceries just now. It's pretty strange that the community objected... I think that a py.typed would be the perfect place for type-checker-agnostic package-specific typing configuration. And I suppose that the format doesn't have to be much more complicated that something like {key}[ = {value}]\n 🤷🏻.
The other idea that popped in my head then, was to instead use a pyright-specific .pyrightignore or a py.right or something like that.
Another option is to (ab)use [tool.pyright] in the pyproject.toml of the target library, by introducing a new option like skip_verify = "**/tests". Alternatively, namespacing it as something like [tool.pyright.verifytypes] could also be worth considering, in case other options that are specific to --verifytypes will be added in the future.

You also mentioned that you're looking for some way to avoid back-sliding on progress toward type completeness. I wonder if you've considered some sort of diffing mechanism here. The --verifytypes command can be combined with --outputjson, which allows the resulting report to be processed easily by scripts. A CI script could do a before-and-after comparison and use the diff to look for regressions. Just a thought.

Yea good idea. Disallowing the type completeness score to go down seems safer than requiring an lower bound. The script would have to install numpy + run pyright twice. But I don't expect that to become the bottleneck of the CI, since mypy_primer also does something like that. Caching the json output using for a particular commit hash could also help.

[jorenham]

In that case, it would help if there was a flag like --ignore-namespace-packages, similar to --ignore-external.

[rgommers]

tests is very widely used in projects to ship test files, not just in numpy.

After reading the pytest docs, I found that it's not only widely used by numerical packages, but under Choosing a test paylout it's actually one of two recommended test suite layouts to have in-tree tests/__init__.py: https://docs.pytest.org/en/stable/explanation/goodpractices.html#tests-as-part-of-application-code.

As I mentioned above, we would want a standardized way to do this

The standard mechanism is __all__. Reading statically populated __all__ should be possible for all tools.

If the tests directory is part of the library's public interface,

Since it's a recommended pytest layout and it's clearly a name for test files, it should be extremely rare for it to be part of the public API. But anyway no one is saying you can't support that, even by default. All it would take to solve this issue is a config file where packages can opt in to ignoring such directories - a single glob like **/tests should be enough.

Absent such a standard, we'll continue to see language servers offer (auto-import) completion suggestions that target test APIs, which is not helpful to users.

They can also read __all__. And even if they don't, this is a single entry for a whole namespace on tab completion. For large projects with hundreds of PRs open, the tradeoff between leaving that one non-public item in place vs. migrating their whole test suite which is lots of work and creates a huge amount of conflicts on open PRs is (a) for them to make and not for you, and (b) very likely easily in favor of keeping the status quo.

@erictraut it's pretty tiring (and sometimes impossible) to deal with conflicting strong views from tool authors, since there are many many dev tools. If pytest and pyright conflict, that isn't helpful for anyone. Please consider adding an exclude config option.

[erictraut]

The standard mechanism is __all__.

The treatment of __all__ is already defined in the typing spec and is part of what allows static analysis tools to determine which symbols are "publicly exported" from a module or submodule. However, this doesn't help us here because __all__ does nothing to determine which submodules are considered part of the public interface.

The standard currently indicates that submodules whose names begin with _ should be considered private and all other submodules should be considered public. That means a submodule named tests will be considered public. If you want to include a submodule named tests in your published package, that will be considered public by today's standards.

The layout of a tests directory in your repo seems orthogonal to whether (or how) you publish your tests directory as part of your package and whether you consider it part of your public interface contract. It's still not clear to me from your answers above whether you consider the tests submodule to be included in the public interface contract. For example, can consumers of the numpy.tests submodule rely on the fact that classes and functions exported from this submodule will be stable across minor releases?

it's pretty tiring (and sometimes impossible) to deal with conflicting strong views from tool authors

I understand and empathize. It's similarly tiring for tool authors to deal with conflicting views and practices among package authors. ;) This is why I push for standardization in areas where it matters. For example, I'm the one who proposed and promoted the specification for __all__ and pushed other tools to adopt it. This is also why I prefer not to build features into pyright that deviate from standards once they are established. This exacerbates the problem.

We could standardize the notion that a submodule with the name of tests should always be excluded from a package's public interface contract for purposes of static tooling. That would require a small update to the Python typing spec, and there's a well-established (and relatively lightweight) mechanism for proposing updates to the typing spec. I'm not sure that hard-coding the name tests is the best approach here, but maybe that's acceptable. A configuration file of some sort would be more flexible but would add complexity, and someone would need to volunteer to write that spec.

[MarcoGorelli]

Thanks all for discussion

Regarding Eric's suggestion

The --verifytypes command can be combined with --outputjson, which allows the resulting report to be processed easily by scripts

I've done that in pyright-cov. Demo:

(without --exclude-like):

$ pyright-cov --verifytypes numpy --ignoreexternal --fail-under 60
Coverage 43.4% is below minimum required 60.0%

(with --exclude-like '*.tests.*'):

$ pyright-cov --verifytypes numpy --ignoreexternal --fail-under 60 --exclude-like '*.tests.*'
Coverage 82.5% is at or above minimum required 60.0%

naming to be determined 😄

I think Pyright gives us everything we need (thanks Eric!) and we can build whatever CI tool we want from the --outputjson output

[jorenham]

A cli flag would suffice here; I don't see what that has to do with the typing spec. So for example something like

pyright --verifytypes numpy --ignore-package numpy.tests --ignore-package numpy.*.tests

or otherwise a glob-style pattern like @rgommers suggested.