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

Add legacypath plugin, move py.path stuff there #9208

Merged
merged 11 commits into from Nov 2, 2021

Conversation

bluetech
Copy link
Member

This is part of issue #7259. The idea is that eventually all remaining py.path dependency will be in this plugin, and it could be phased out from core into an external plugin, and pytest could remove the dependency on py, while providing users a way to restore compatibility.

I managed to move everything to the plugin, except two things, which I don't think can be:

  1. py.path arguments in hooks. This is currently already deprecated so slated for removal in 8.0.
  2. py.path arguments to Node ctors. This is not yet deprecated, but should be so could be removed in 8.0.

Both of these things mostly affect plugins, not really user test suites, so I think deprecation+removal of them is reasonable.

Note: if the plugin is not loaded, all of the tests pass, except test_legacypath.py tests.

This is marked WIP as there as few things I need to do before this can be merged (plugin deps, changelog, docs), but comments are welcome.

For comparison, I rebased my branch which removed py completely, which I used as reference: https://github.com/bluetech/pytest/commits/rm-py

@nicoddemus
Copy link
Member

This is great work so far!

@RonnyPfannschmidt
Copy link
Member

This looks lovely, I'll give it a spin once I get Better from the cold

@bluetech
Copy link
Member Author

Rebased, now uses the public exported pytester types (the goal is to not use any private stuff from the plugin). Still haven't worked on the other issues though.

@bluetech bluetech changed the title WIP: Add legacypath plugin, move py.path stuff there Add legacypath plugin, move py.path stuff there Oct 28, 2021
@bluetech
Copy link
Member Author

I fixed the docs generation and improved the coverage a bit (though not for testdir and a couple other obscure bits).

One thing is that the testdir and tmpdir fixtures will now be loaded even if the pytester and tmpdir plugins are not. The two downsides to this are:

  • Will show in the fixtures list (testdir is not loaded by default)
  • If used and the corresponding plugin is not loaded, will give an unclear warning, e.g. using testdir will give an error pytester fixture not found or such.

I can think of ways to fix this but I don't think they're worth it, so I'm inclined to just leave it as is.

Regarding the changelog and docs, I'd like to add them in a separate PR, as part of making the entire py.path deprecation more cohesive.

So I'm marking the PR as ready.

def tmpdir_factory(request: pytest.FixtureRequest) -> TempdirFactory:
"""Return a :class:`pytest.TempdirFactory` instance for the test session."""
# Set dynamically by pytest_configure().
return request.config._tmpdirhandler # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets make a plugin class that adds those 2 and gets installed in pytest_configure, that way that part is handed more correct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand "those 2" and "that part" -- I'm not sure I get what they refer to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i missplaced my commment - i meant the testdir fixture
if its only added when the pytester plugin is enabled, it should be fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it by moving to a separate plugin in a separate file, otherwise I don't know how to prevent @pytest.fixture from doing its thing, i.e. how to add the fixture dynamically. And I thought that was a bit too much, for something that users are not very likely to hit, and would only result in a slightly less clear error message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluetech a plugin an just be a class - so if that particular fixture jsut goes to a class no new file is needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I thought @pytest.fixture has a more global effect, even though I know it doesn't.

I pushed a commit which does this, PTAL.

… plugins are registered

This preserves the existing behavior and gives a proper error message
in case e.g. `testdir` is requested without the `pytester` plugin being
loaded.
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants