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

[prerelease] pytest.TmpdirFactory is not importable anymore #9432

Closed
The-Compiler opened this issue Dec 22, 2021 · 6 comments · Fixed by #9438
Closed

[prerelease] pytest.TmpdirFactory is not importable anymore #9432

The-Compiler opened this issue Dec 22, 2021 · 6 comments · Fixed by #9438
Labels
plugin: tmpdir related to the tmpdir builtin plugin topic: typing type-annotation issue type: regression indicates a problem that was introduced in a release which was working previously
Milestone

Comments

@The-Compiler
Copy link
Member

The-Compiler commented Dec 22, 2021

Discovered in ClearcodeHQ/pytest-postgresql#533 (comment) via #9415:

Doing e.g. python3 -c "import pytest; pytest.TempdirFactory" fails since 5e883f5. This is a bit similar to #9396, but #9400 won't help with this one.

Test modules often use pytest.TempdirFactory for type annotations - in fact, that's why that was added in 6.2.0.

However, it being set from a plugin means that:

a) Test modules aren't even importable anymore outside of pytest - in case of pytest-postgresql, this breaks an importability check, but I can imagine various other things breaking...
b) Perhaps more importantly, static analysis in IDEs or tools like mypy aren't aware anymore that the attribute exists - however, after all, that's the whole point of it existing, and lots of projects use it that way. (Some import it from _pytest.tmpdir, probably because they did that before 6.2.0 - I'm not too worried about those, they knew what they're in for)

Reproducer:

from pytest import TempdirFactory

def test_nothing(tmpdir: TempdirFactory):
    pass

Fails with mypy:

test_x.py:1: error: Module "pytest" has no attribute "TempdirFactory"; maybe "TempPathFactory"?
Found 1 error in 1 file (checked 1 source file)

What to do? I suppose as a stop-gap until we want to distribute the plugin independently from pytest, we could import the class from the plugin or so?

@The-Compiler The-Compiler added type: regression indicates a problem that was introduced in a release which was working previously plugin: tmpdir related to the tmpdir builtin plugin topic: typing type-annotation issue labels Dec 22, 2021
@The-Compiler The-Compiler added this to the 7.0 milestone Dec 22, 2021
@RonnyPfannschmidt
Copy link
Member

Let's provide the attribute, for python 3.7+ we can provide a deprecation using the module getattr special method

@The-Compiler
Copy link
Member Author

I don't see how we could reasonably deprecate the type annotation without deprecating the tmpdir_factory fixture as a whole.

@bluetech
Copy link
Member

Whoops didn't think of this. The stuff isn't deprecated yet, so for now we should just restore it somehow, i.e. instead of the plugin monkeypatching pytest, jus import it in pytest from _pytest.legacypath. When the time comes to deprecate the entire thing, we'll think how best to do it (hopefully by then we will have dropped python3.6 so module getattr is fully available).

@bluetech
Copy link
Member

(Personal note: in the end I didn't have time for pytest this week, but I do of course intend to work on all of the regressions I caused when I get the chance)

@The-Compiler
Copy link
Member Author

Whoops didn't think of this. The stuff isn't deprecated yet, so for now we should just restore it somehow, i.e. instead of the plugin monkeypatching pytest, jus import it in pytest from _pytest.legacypath.

Sounds good to me!

When the time comes to deprecate the entire thing, we'll think how best to do it (hopefully by then we will have dropped python3.6 so module getattr is fully available).

Note that this probably wouldn't help mypy and friends, which are the main consumers of the type annotation.

(Personal note: in the end I didn't have time for pytest this week, but I do of course intend to work on all of the regressions I caused when I get the chance)

Don't worry about it - we still have quite some "unknown unknowns" in #9415 and topics other people are looking into (like #9404). As said before, I'd rather have a solid 7.0.0 release a bit later than a half-baked one which will cause us trouble down the road.

Also, please don't feel bad for it - you worked on a lot of very tricky topics for this release (which is very appreciated!), so it's somewhat natural that regressions happen to bisect to one of your commits - let us know if we can help somewhere, and thanks for handling the fallout!

@nicoddemus
Copy link
Member

Also, please don't feel bad for it - you worked on a lot of very tricky topics for this release (which is very appreciated!), so it's somewhat natural that regressions happen to bisect to one of your commits - let us know if we can help somewhere, and thanks for handling the fallout!

I second that! 👍

bluetech added a commit to bluetech/pytest that referenced this issue Dec 25, 2021
The monkeypatch approach doesn't work for `import pytest;
pytest.TempdirFactory`.

Fix pytest-dev#9432.
bluetech added a commit to bluetech/pytest that referenced this issue Dec 25, 2021
The monkeypatch approach doesn't work for `import pytest;
pytest.TempdirFactory`.

Fix pytest-dev#9432.
bluetech added a commit to bluetech/pytest that referenced this issue Dec 25, 2021
The monkeypatch approach doesn't work for `import pytest;
pytest.TempdirFactory`.

Fix pytest-dev#9432.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: tmpdir related to the tmpdir builtin plugin topic: typing type-annotation issue type: regression indicates a problem that was introduced in a release which was working previously
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants