Seeking improved pytest rules, as flake8-pytest-style doesn't align with best practices

[RonnyPfannschmidt]

hi everyone,

before i evaluated ruff i wasn't even aware of flake8-pytest-styles,
as one of the pytest maintainers i'm particularly annoyed as the rules don't reflect documented and communicated best practices and the upstream doesn't seem likely to change those details

• PT001 should be reversed (drop brackets)
• PT004 is simply incorrect - public no return value fixtures should have public names for both usage in the usefixtures maker and as dependencies in other fixtures
• PT005 is equally incorrect - internal fixtures return values even if underscores are used - sabotage of that is not ok
• PT006 undoes the intentionally established pattern of being able to comma separate multiple names in a single string
• PT009 sabotages intentional usage of unittest (which is sometimes necessary)
• PT021 sabotages well established patterns for complex fixtures that setup in phases where on needs to ensure teardown but ought not to split them into multiple smaller fixtures
• PT023 should drop parens on marks by default
• PT024/25 should warn on any mark on a fixture as no marks are supported on fixtures
• PT027also sabotages legitimate usage of unittest patterns

[ThiefMaster]

As a user who just started adding ruff to his (somewhat big) codebase I also added config to change the weird defaults WRT parentheses because the defaults were quite weird!

Just some thoughts on your points:

• PT006: Maybe parametrize-names-type (and possibly the other settings related to parametrize as well?) should allow a list of valid types (e.g. ['csv', 'tuple'] if you don't want lists), and when applying a fix pick the first one in the list (to avoid an extra setting to set the preferred type)?
• PT009: I guess this is a matter of old existing vs new/modern codebase? On the latter I absolutely want this enabled because that way someone not familiar with the nice pytest features will immediately get a linter warning if they try to use unittest-style assertions. Personally I'd simply disable it in codebases that make use of it, but I think "supporting legacy stuff" should not be a default... I'm curious, when is using the legacy assertions actually necessary? (My first thought there was "imagine if this was simply autofixable")
• PT021: Would it make sense if this rule only triggered when there's just a single request.addfinalizer() call in a fixture? I guess for most applications it's not common to have truly complex fixtures, so it nicely covers the common cases of someone simply not being aware that they can use yield in their fixtures.

[ofek]

Agreed, except:

• PT006 - please keep this as-is, I think it is perfect and wherever I have worked people got confused about the types and for multiple prefer the tuple just until now there was no way to enforce it
• PT009/PT021/PT027 - please keep these as-is, there should be a desire to encourage the "best way" to do things as that is the entire purpose of tools like this

[RonnyPfannschmidt]

in that case there should also be a suggestion to drop TestCase base class and xunit setup methods
imho PT009/PT021/PT027should group under a unittest2pytest style thing - and perhaps take some more code off https://github.com/pytest-dev/unittest2pytest

[RonnyPfannschmidt]

perhaps it makes sense to trigger the pytest migration rules based on whether a pytest import is in a test file,

a very neat but expensive way would be to have "migraton" rules/fixes only apply when the line is currently being changed anyway

[ThiefMaster]

a very neat but expensive way would be to have "migraton" rules/fixes only apply when the line is currently being changed anyway

I would not like that in my own project; It leaves an inconsistent mess and makes PRs harder to review because now they suddenly contain no longer just the main change they are about but also 'infrastructure changes'. I prefer a separate PR for those (which can be more easily skimmed over during review)

perhaps it makes sense to trigger the pytest migration rules based on whether a pytest import is in a test file

but many test files don't contain any pytest imports if they don't use parametrization...

[eli-schwartz]

before i evaluated ruff i wasn't even aware of flake8-pytest-styles,
as one of the pytest maintainers i'm particularly annoyed as the rules don't reflect documented and communicated best practices and the upstream doesn't seem likely to change those details

I do not use flake8-pytest-style anyway, but. I did notice that it is recommended at https://docs.pytest.org/en/stable/explanation/goodpractices.html#checking-with-flake8-pytest-style

Should it be removed?

[RonnyPfannschmidt]

Good point

It should

[charliermarsh]

FWIW, I have no problem with shipping better pytest rules (though I haven't used pytest extensively so don't feel qualified to have good opinions on the rules themselves). In practice, it may be easiest to ship them under an entirely new pytest prefix / rule set, and mark the flake8-pytest-style rules as deprecated, but we can make that decision later.

[RonnyPfannschmidt]

I'll coordinate with pytest-core to get together a official set

[ofek]

FYI until such time that there is an official set, for Hatch's new fmt command I'm going to by default ignore rules PT001, PT004, PT005, and PT023, while enabling the rest.

[Mogost]

PT001, PT023 styles are configurable by https://docs.astral.sh/ruff/settings/#flake8-pytest-style-fixture-parentheses and https://docs.astral.sh/ruff/settings/#flake8-pytest-style-mark-parentheses

[karolinepauls]

Would it be possible to disable the most offending rules by default for the time being?

[ofek]

If you would prefer to have a large default selection of "good" rules, you can use Hatch https://hatch.pypa.io/latest/config/static-analysis/

The undesirable ones that you speak of here are excluded by default.

[jhbuhrman]

as one of the pytest maintainers i'm particularly annoyed as the rules don't reflect documented and communicated best practices and the upstream doesn't seem likely to change those details

I understand to a large extent what you mean. But the way I see it is that there is a difference between the possibilities of a programming language, a (testing) framework, or a library (API), and the agreements a team wants to make about a uniform usage of those assets.

As an example, I can understand that a team agrees to always use a decorator with a set of parentheses, even when there are no additional parameters to provide in a particular case, or that a team always uses the list (or tuple) form in specifying the parameter names in @pytest.mark.parametrize instead of specifying them separated by commas in a single string. So I don't see it as a Bad Thing that these rules exist, ready to be used by any team that wants to do so.

OTOH, I didn't dive in all the particular rules, but advocating PT004 / PT005 as a by pytest advocated practice is incorrect IMHO.

BTW, I think a know where these two rules come from: my line of reasoning is as follows:

• if you need to use a fixture having a side effect but without a useful return value, the fixture will probably return None.
• The developer needs the side effect, so specifies the fixture name as a parameter in their test function, or in another fixture, so a usefixture decorator is not possible.
• Other linters will start complaining that the parameter value is not used in the function.
• There is a "convention" that parameters starting with an underscore are a hint to some linters that it is OK that the value is not being used.

I assume that this might be the line of reasoning behind PT004 / PT005.

Unfortunately the rule is implemented in a wrong way. Instead of checking the name of the decorated function, the rule should check the name of the fixture. This could be the name of the function, but can also be different if the fixture decorator has a name= parameter, or when pytest_bdd.given for example has a target_fixture= parameter.

[bjtho08]

@charliermarsh FYI the latest version of flake8-pytest-style (v2.0.0), which was shipped on april 1st, inverted the defaults for PT001 and PT023 so that they now conform with pytest recommendations. Maybe ruff should push out a similar change soon?

[PhorstenkampFuzzy]

If you do establish a new set of rules around pytest. A rule that warns users on the use of the @pytest.mark.usefixtur and demands the @pytest.mark.usefixturs. I know of a few instances where debugging this caused A LOT of work.

[avilaton]

If you are using pants (pantsbuild.org) then rule PT004 and PT005 will really hurt. We had a session scoped fixture which for changed from django_db_modify_db_settings into _django_db_modify_db_settings which then caused it to be ignored and was VERY HARD to figure out later. Rule PT004 is a menace IMO.

[RonnyPfannschmidt]

@charliermarsh based on this detail i think a release that declares them unsafe or removes them ougth to be expedited

[charliermarsh]

@RonnyPfannschmidt -- Neither of these rules have a fix though -- those changes must've been applied manually by the author.

[charliermarsh]

I'd be fine to deprecate PT004 and PT005 but it will have to wait until the next minor release (we don't deprecate rules in patch releases IIRC).

[PhorstenkampFuzzy]

Could also a rule be added. I have sometimes seen a problem with pytest.mark.usefixutres vs. pytest.mark.usefixture.
Could a roule be added that forbides the later?

[charliermarsh]

Current plan is to deprecate PT004 and PT005 in the upcoming v0.6.

[MichaReiser]

The upcoming 0.6 release deprecates PT004 and PT005 and changes the defaults for PT001 and PT023.