Remove dynamic setup of pytest.collect fake module

[blueyed]

Make it a real module instead.

This was initially added in 9b58d6e
(#2315), when removing support
for pytest_namespace.

Not sure if it ever worked "properly", but currently
import pytest.collect and from pytest.collect import File do not
work (but with this patch).

This is only relevant for (backward) compatibility, but the more it
appears to make sense to move it into a lazily loaded, separate file.

[RonnyPfannschmidt]

removing the attribute is a breaking change, we should deprecate it and keep importing it
this was introduced when pytest itself was NOT a package, its just icky and should be taken away

[blueyed]

@RonnyPfannschmidt
What do you mean by "removing the attribute"?
This adds a test for compat that might have even failed before (if being more strict), since now the "attribute" actually behaves like a module.

[RonnyPfannschmidt]

But you either have to do eager import or cut a major release

[nicoddemus]

Perhaps follow the previous approach of using a real module (say _pytest._collect) and use it here?

def __getattr__(name):
    if name == "collect":
        from _pytest import _collect as collect
        return collect
    raise `AttributeError(name)` 

Hmmm unfortunately __getattr__ is Python 3.7 only?

[blueyed]

I'd rather keep the original code as-is, given that it is supposed to not stay anyway.

[RonnyPfannschmidt]

As a follow-up or inline, adding to sys. Modules and adding a deprecation warning subclass of the module type should improve the general situation

[blueyed]

Adding to sys.modules would likely break that test: https://github.com/pytest-dev/pytest/pull/6803/files#diff-dc953e3e35889673e6c7ba78247b25d9R49-R50

[blueyed]

The deprecation warning should be done in a follow-up.

[nicoddemus]

But you either have to do eager import

You mean using a real import on pytest.__init__, like it was done in 30a7a68?

I would be OK with that, currently we are already paying the price of importing those names anyway.

One problem is that I can't think of a way of deprecating that, unfortunately.

(The __getattr__ module level that @blueyed proposes would be ideal for that, but it is Python 3.7+ only).

[blueyed]

The eager import was done in 8a8b1f6.

We can use __getattr__ for py37+ then, and the old / unconditional code otherwise?

[blueyed]

Missing coverage is due to symlink_or_skip - pending investigation by a Windows user (it seems like all of the CI jobs can create symlinks? - #6733).

[nicoddemus]

We can use getattr for py37+ then, and the old / unconditional code otherwise?

Good idea, this way we can upgrade to the py37+ only version when the time comes to drop py36.

Missing coverage is due to symlink_or_skip - pending investigation by a Windows user

I gave this another go, but no luck yet (left a comment on that PR).

[nicoddemus]

Missing coverage is due to symlink_or_skip - pending investigation by a Windows user

Btw I don't think that should block this.

[blueyed]

Missing coverage is due to symlink_or_skip - pending investigation by a Windows user

Btw I don't think that should block this.

Me neither - but would be good that it works / is covered for non-admin users.

[nicoddemus]

but would be good that it works / is covered for non-admin users.

Oh that's what you meant to convey on #6803 (comment)? I thought you meant that this should be blocked until that gets resolved. Clear now. 👍