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

Load plugins from paths in 'pythonpath' option #12536

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

millerdev
Copy link
Contributor

Real fix for #11118

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jun 26, 2024
@millerdev millerdev force-pushed the early-local-plugin branch 7 times, most recently from 204e433 to 7843f28 Compare June 26, 2024 23:51
@millerdev
Copy link
Contributor Author

Can't figure out how to resolve the codecov failure, which seems to be illegitimate anyway since it removed the incorrect "not covered by tests" annotations. Otherwise, this is ready for review.

@bluetech
Copy link
Member

@millerdev Since the issue is closed, could you explain your use case for this change? I'm asking because we generally prefer to put functionality in plugins, so we'd like to understand the reasoning before moving to the core.

@millerdev
Copy link
Contributor Author

@bluetech I am trying to load a plugin that implements early hooks like pytest_load_initial_conftests from within the package under test. I could find no other way to do it without this change. My requirements are:

  • tests must be run against packages in the CWD (pythonpath contains . to enable this)
  • tests must be run with the pytest command (not python -h pytest).
  • a custom pytest plugin required to run the tests resides within one of the packages under test
  • the packages under test are not pip install-able

The python_path plugin where this functionality was implemented was using the earliest available pytest hook (pytest_load_initial_conftests) to update sys.path with values from the pythonpath option. Unfortunately that hook is not early enough in the plugin lifecycle to allow plugins to be loaded from pythonpath paths.

@millerdev
Copy link
Contributor Author

@bluetech were you able to consider my use case requirements? Is there anything else blocking for this PR that I should address before it can be merged?

@millerdev millerdev force-pushed the early-local-plugin branch from 2b319c1 to dfa49d0 Compare July 29, 2024 10:32
@nicoddemus
Copy link
Member

@millerdev in summary your requirement is that you need to load plugins which are implemented in files in ., however for them to be able to be imported, their path needs to be in PYTHONPATH already?

@millerdev
Copy link
Contributor Author

@nicoddemus That is correct. Thank you for taking a look!

@nicoddemus
Copy link
Member

I see, thanks for clarifying.

I understand (from a glance, don't have much time now to look deeply into it) you basically removed the python_path.py plugin and made the pythonpath configuration value a feature of Config, right?

Hard to tell if this might cause regressions or not, my gut feeling is that this is OK because it only affects those already using pythonpath and is only configuring $PYTHONPATH a little earlier than before.

In general I think it makes sense to set $PYTHONPATH via the pythonpath confval as early as possible. I also believe it was implemented as a separate plugin because it was cleaner that way, but it seems it is not enough to cover all cases.

What do others think? @RonnyPfannschmidt @Pierre-Sassoulas @bluetech

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
@millerdev millerdev force-pushed the early-local-plugin branch from 6a07028 to 2163ca1 Compare August 1, 2024 17:18
@millerdev millerdev force-pushed the early-local-plugin branch from 2163ca1 to 6c04963 Compare August 1, 2024 17:20
changelog/11118.improvement.rst Outdated Show resolved Hide resolved
testing/test_python_path.py Outdated Show resolved Hide resolved
testing/test_python_path.py Show resolved Hide resolved
millerdev and others added 2 commits August 3, 2024 10:22
Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
@RonnyPfannschmidt
Copy link
Member

We need to make note to turn this into a early plugin late (similar to what coverage needs)

@bluetech
Copy link
Member

bluetech commented Aug 4, 2024

Seems there is a use case for moving this functionality to core, but that means it can't be a plugin. We prefer functionality to be in plugins because it keeps the core cleaner and leaner. So the question is, is it worth complicating the core in order to accommodate the use case?

IMO the answer is no. The use case and constraints (not being able to set PYTHONPATH or sys.path) seem too niche to me. But I don't feel strongly about it, i.e. perfectly fine if one of the other maintainers wants to merge.

BTW: Apologies for being slow to follow up @millerdev.

@nicoddemus
Copy link
Member

I'm OK with this, given the implementation is really simple, and it solves one pain point that is avoiding have to configure PYTHONPATH externally.

So if it is OK with @bluetech I would like to get this in.

@nicoddemus nicoddemus merged commit 85760bf into pytest-dev:main Aug 6, 2024
28 of 29 checks passed
@nicoddemus
Copy link
Member

Thanks @millerdev for the contribution!

@millerdev
Copy link
Contributor Author

@nicoddemus Thank you for getting this in!

@millerdev
Copy link
Contributor Author

@nicoddemus in what version of pytest is this expected to be released?

millerdev added a commit to dimagi/pytest-unmagic that referenced this pull request Oct 17, 2024
@nicoddemus
Copy link
Member

Version 8.4, but we do not have an estimated release date yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants