Docs: clarify conftest usage and fixture import pitfalls #14128
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
for more information, see https://pre-commit.ci
bluetech
left a comment
bluetech
left a comment
There was a problem hiding this comment.
Thanks for looking at this. Please see my comments.
Also pinging @The-Compiler who might want to review this (the issue discusses a bit larger reworking of the docs around this IIUC).
doc/en/how-to/fixtures.rst
Outdated
| Possible values for ``scope`` are: ``function``, ``class``, ``module``, ``package`` or ``session``. | ||
|
|
||
| The next example puts the fixture function into a separate ``conftest.py`` file | ||
| .. note:: |
There was a problem hiding this comment.
Can you try integrating this into the text itself rather than in a "note"? The "note" structure somewhat interrupts the flow and it's tempting to have too many of them but I think it's better to have a natural flow.
doc/en/how-to/fixtures.rst
Outdated
| The next example puts the fixture function into a separate ``conftest.py`` file | ||
| .. note:: | ||
|
|
||
| Fixture *scope* controls lifetime and caching, not visibility. |
There was a problem hiding this comment.
This is the first usage of the word "visibility", it will be hard to understand the meaning of the sentence without already knowing what it means in this context.
doc/en/how-to/fixtures.rst
Outdated
|
|
||
| Fixture *scope* controls lifetime and caching, not visibility. | ||
| Visibility is determined by where the fixture is defined (test module, | ||
| ``conftest.py``, or plugin). |
There was a problem hiding this comment.
conftests are first mentioned in the next sentence, so in terms of flow it will be better to discuss this after.
doc/en/how-to/fixtures.rst
Outdated
|
|
||
| .. note:: | ||
|
|
||
| This is the recommended way to share fixtures across multiple test modules. |
There was a problem hiding this comment.
It's a bit better to avoid "This" and the thing itself instead, i.e. "Conftests are the recommended way ...". This (hehe) way the paragraph stands on its own.
doc/en/how-to/fixtures.rst
Outdated
| This is the recommended way to share fixtures across multiple test modules. | ||
| Avoid importing fixtures from other test files or from ``conftest.py``, | ||
| as importing fixtures can cause them to be registered in multiple modules and | ||
| lead to confusing behavior (such as fixtures running more than once). |
There was a problem hiding this comment.
| lead to confusing behavior (such as fixtures running more than once). | |
| lead to confusing behavior such as fixtures running more than once. |
doc/en/how-to/fixtures.rst
Outdated
| lead to confusing behavior (such as fixtures running more than once). | ||
|
|
||
| As a rule of thumb: **never import fixtures from test files or conftest.py, | ||
| unless it is only for type annotations**. |
There was a problem hiding this comment.
Can you explain the type annotations part?
changelog/13148.doc.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Clarified fixture visibility, conftest usage, and why importing fixtures is discouraged. | |||
There was a problem hiding this comment.
| Clarified fixture visibility, conftest usage, and why importing fixtures is discouraged. | |
| :ref:`Clarified <smtpshared>` fixture visibility, conftest usage, and why importing fixtures is discouraged. |
|
Thanks a lot for the detailed review! This makes sense — I’ll rework the text |
|
Hello @bluetech :) I’ve updated the text to integrate the notes into the main flow, clarified the wording, and addressed the type-annotation explanation. Please let me know if this looks better now. |
2 participants
Clarifies that fixture scope controls lifetime and caching, not visibility
Explicitly explains that fixture visibility depends on where it is defined (test module, conftest.py, or plugin)
Recommends conftest.py as the correct and supported way to share fixtures across multiple test modules
Adds a clear warning against importing fixtures from test files or conftest.py, which can cause duplicate registration and confusing behavior
Adds a simple rule of thumb: never import fixtures except for type annotations
Links readers to the reference documentation for deeper technical details
closes #13148