Package should not be a Module/File

[bluetech]

Note: this issue is extracted verbatim from #7777 (comment) so I can refer to it separately

Package is currently a subclass of a Module, with package.path being the __init__.py file.

While technically in terms of the Python data model this is correct, I'm pretty convinced this was the wrong decision in pytest:

Package is a Module with path __init__.py, but it doesn't actually act as a Module behaviorally, it overrides everything that Module does and doesn't call super() on anything.

Module is a nodes.File, which makes Package a nodes.File, but it doesn't act as a file.

The previous points can be rephrased as: Package breaks Liskov substitution -- any code dealing with File or Module generically probably doesn't want Package.

Package is a Module with path __init__.py, but it actually collects a "real" (non-Package) Module for the __init__.py file (if permitted by python_files glob). This is very confusing.

Package and __init__.py necessitates several special-casings because of these points:

• pytest/src/_pytest/cacheprovider.py

  Lines 275 to 278 in 24534cd

  	# Packages are Modules, but _last_failed_paths only contains
  	# test-bearing paths and doesn't try to include the paths of their
  	# packages, so don't filter them.
  	if isinstance(collector, Module) and not isinstance(collector, Package):

• pytest/src/_pytest/main.py

  Lines 805 to 818 in 24534cd

  	# If __init__.py was the only file requested, then the matched
  	# node will be the corresponding Package (by default), and the
  	# first yielded item will be the __init__ Module itself, so
  	# just use that. If this special case isn't taken, then all the
  	# files in the package will be yielded.
  	if argpath.name == "__init__.py" and isinstance(matching[0], Package):
  	try:
  	yield next(iter(matching[0].collect()))
  	except StopIteration:
  	# The package collects nothing with only an __init__.py
  	# file in it, which gets ignored by the default
  	# "python_files" option.
  	pass
  	continue

• pytest/src/_pytest/fixtures.py

  Line 123 in 24534cd

  fixture_package_name = "{}/{}".format(fixturedef.baseid, "__init__.py")

• pytest/src/_pytest/python.py

  Lines 229 to 231 in 24534cd

  	if module_path.name == "__init__.py":
  	pkg: Package = Package.from_parent(parent, path=module_path)
  	return pkg

• pytest/src/_pytest/python.py

  Lines 750 to 761 in 24534cd

  	# Always collect the __init__ first.
  	if path_matches_patterns(self.path, self.config.getini("python_files")):
  	yield Module.from_parent(self, path=self.path)
  	pkg_prefixes: Set[Path] = set()
  	for direntry in visit(str(this_path), recurse=self._recurse):
  	path = Path(direntry.path)
  	# We will visit our own __init__.py file, in which case we skip it.
  	if direntry.is_file():
  	if direntry.name == "__init__.py" and path.parent == this_path:
  	continue

Proposed solution

As part of the breaking Package changes discussed previously in this issue, also make these changes

• Package no longer inherits from Module (or File by extension), just from FSCollector.
• Package.path is the package directory, not the __init__.py file.
• Collecting pkg/__init__.py collects the __init__.py file as a module (file), doesn't collect the entire package.

This also matches the new Directory node, which is the non-Package directory collector. Directory will inherit just from FSCollector and its path will be the directory. It will be much better if they are as similar to each other as possible.

Complication

Currently Package has a setup() method which imports the __init__.py' and runs its setup_module function and registers teardown_module finalizer (in effectively the package scope). If we want to stop having Package as a Module it becomes a bit less natural to implement.

This functionality has some issues:

• It is undocumented.
• It doesn't match unittest which doesn't have package functionality
• The names setup_module and teardown_module conflict with the "real" __init__.py Module setup/teardown; i.e., these methods are executed twice, if the __init__.py is included in the glob. It would have been better to call them setup_package/teardown_package (this is how nose calls them).

It is tempting to just remove it, but it will probably cause some breakage (particularly the __init__.py importing part), so for now I plan to keep it in an ad-hoc manner.

POC

I have an initial implementation of this change here, with all tests passing:
https://github.com/bluetech/pytest/commits/pkg-mod