feat: add warning when pytest.ini and pyproject.toml are present #13797
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
sgaist
force-pushed
the
add-warning-on-pytest-ini-and-pyproject-presence
branch
from
caf5ef7 to
fce5184
Compare
The behaviour does not change however users will be notified that pytest.ini is selected over pyproject.toml and any configuration related to pytest it may contain.
sgaist
force-pushed
the
add-warning-on-pytest-ini-and-pyproject-presence
branch
from
fce5184 to
5ff1af4
Compare
|
I am unsure what to do to satisfy codecov. Any hint would be appreciated. |
There was a problem hiding this comment.
Thanks for working on this.
I haven't thought yet whether we really want this, but in any case a couple of comments:
We should not emit a warning if pyproject.toml exists but without a pytest section. That's very common and expected and not ambiguous (please also add a test for this case).
Also, it will be better not to have the "should warn" logic in terminal.py, it shouldn't know about config stuff itself. Ideally (I think) the core config file search logic should indicate whether need to warn.
Doing so, Terminal does not need to know anything about the configuration files.
|
@bluetech I should have kept my original implementation which was following your suggestion minus the verification of the content of the I refactored the code following your suggestion. |
Do it the other way around, check if the file suffix is ini rather than listing the possible names.
src/_pytest/config/__init__.py
Outdated
| return self._inipath | ||
|
|
||
| @property | ||
| def should_warn(self) -> bool: |
There was a problem hiding this comment.
Thanks for the update.
-
We shouldn't expose this in the public API, let's keep it private unless there's a need.
-
If it's private, no need for a property
-
The name
should_warnis too generic, we should say warn about what. Or better, something like_ignored_config_files = ["pyproject.toml", ".pytest.ini']containing the ignored files we detected. Semantically whether or not to warn is up to the place which warns.
There was a problem hiding this comment.
Works for me, I'll update the code.
This will let users know exactly which files have been ignored in case multiple of them contain pytest configuration.
There was a problem hiding this comment.
Thanks! Now the approach looks good to me, though I haven't reviewed the code in detail.
@nicoddemus I'll leave it to you to decide whether we want this :)
src/_pytest/config/__init__.py
Outdated
| args, namespace=copy.copy(self.option) | ||
| ) | ||
| rootpath, inipath, inicfg = determine_setup( | ||
| rootpath, inipath, inicfg, ignored_files = determine_setup( |
There was a problem hiding this comment.
I'd use ignored_config_files because ignored_files as again a bit too generic.
I think so, because having both configurations is confusing and error prone -- a warning would be welcome. I will try to review this in the next few days, thanks folks! |
Add a warning when pytest.ini and pyproject.toml are present
The behaviour does not change however users will be notified that pytest.ini is selected over pyproject.toml and any configuration related to pytest it may contain.
Fixes #13330