Make sure we treat tests differently

[sponsfreixes]

When using pytest fixtures, you are supposed to just declare them as input arguments, and they will be automagically loaded. For example:

import pytest
from myapp import meaning_of_life

@pytest.fixture
def foo():
    return 42

def test_meaning_of_life(foo):
    meaning = meaning_of_life()
    assert meaning == foo

This will violate rule 442, which is not desired in this case.

One could argue that fixtures should live on a different file than tests, but you run on the same issue if you have fixtures dependent on other fixtures:

import pytest

@pytest.fixture
def foo():
    return 42

@pytest.fixture
def bar(foo):
    return foo + 1

I think we need to make this feature "pytest aware", or just document that it there is conflict with it and might need to be disabled.

[sobolevn]

Yes, this is true.

I usually disable this check for all conftest.py files. Could you please send a PR with the doc fix?

[sponsfreixes]

Will do!

[sobolevn]

@sponsfreixes are you still interested in fixing docs for this one? 🙂

[skarzi]

I think it would be really useful for new users to have somewhere in docs any example of per-file-ignores prepared specifically for tests and some comments with explanation, why we are suggest to ignore following violations in tests

[sobolevn]

@skarzi completely agree! I think even a whole new documentation entry under Usage might be added.

[sponsfreixes]

@sponsfreixes are you still interested in fixing docs for this one? 🙂

Whoa, it has been already a month since my last comment 😬

Yeah, I was still planning to do it. If somebody wants to steal it before I start, feel free to change assignee.

@skarzi what other violations should be ignored on tests?

[sobolevn]

Here several my projects that have a list of ignored violations for tests:

• https://github.com/sobolevn/django-split-settings/blob/master/setup.cfg#L36-L39
• https://github.com/dry-python/returns/blob/master/setup.cfg#L48
• https://github.com/wemake-services/dotenv-linter/blob/master/setup.cfg#L37-L38

Here's the ignore line for one private project:

  # Enable `assert` keyword and magic numbers for tests:
  tests/*.py: D103, S101, S105, WPS202, WPS210, WPS211, WPS336, WPS432

Basically I ignore:

1. Number of asserts, because I usually violate "one test - one assert" rule
2. I allow magic numbers more than often, because tests might have very strange logic on numbers in examples and test data
3. I allow to have a lot of module members inside a module (functions mostly)
4. Sometimes we have to allow a lot of arguments for test functions, because pytest passes fixtures as arguments
5. Sometimes it makes sense to have a lot of variables when you assemble some complex object from other fixtures / data

I can also ignore new violations from new linter versions in tests, because refactoring them is error-prone and has little value.

[skarzi]

My per-file-ignores for tests includes violations mentioned by @sobolevn and additionally following one:

• WPS118 - imho test functions/methods should have nice, meaningful names, so it's often better to use long but self-explanatory name for them

[sobolevn]

@sponsfreixes friendly ping. Are you still planing to work on this task? Do you need any help?

[sobolevn]

Might be related: #1268

Maybe we can use some kind of --tests-filename-pattern (similar to http://doc.pytest.org/en/latest/reference.html#confval-python_files)? And disable some checks there by default?

I don't see much value in just updating the docs for this one. I suggest to add a feature that will solve this case.

[sobolevn]

WPS202 from #1409

[webknjaz]

Maybe we can use some kind of --tests-filename-pattern (similar to http://doc.pytest.org/en/latest/reference.html#confval-python_files)? And disable some checks there by default?

Could as well just read the pytest config to find out the patterns instead of having a separate flake8 setting...

[sobolevn]

One can use unittest or any other tool. I don't feel that interfering with other tools is a great idea.

[webknjaz]

It's not interfering. You'd just construct the default value of the flake8 setting based on what you see in the project dir.

[sobolevn]

Can you please give a more detailed example of how do you see this process?

[webknjaz]

So we add a setting like wps_tests_filename_pattern that can be set in the config explicitly. But if it's not set, it should have some sort of a reasonable default. We could hardcode it but it'd be a dumb solution. To make it clever, we could read pytest.ini and any other well-known location to find out the pattern and reuse it. And only if this fails, it could fall-back to having a hardcoded value.

[webknjaz]

Here's another case:

@pytest.mark.parametrize(
    ('is_crashing', 'is_strict'),
    (
        pytest.param(True, True, id='strict xfail'),
        pytest.param(False, True, id='strict xpass'),
        pytest.param(True, False, id='non-strict xfail'),
        pytest.param(False, False, id='non-strict xpass'),
    ),
)
def test_xfail(is_crashing, is_strict, testdir):
    """Test xfail/xpass/strict permutations."""
    ...

Boolean literals in pytest.param() calls trigger WPS425: Found boolean non-keyword argument: True which is wrong.