Checks pass when run under 3.8 or later but fails on 3.6 and 3.7

[gaborbernat]

Bug report

When using this plugin under python3.8 or later it behaves differently than running under python3.6/python3.7.

What's wrong

Take the test project https://github.com/tox-dev/tox/tree/rewrite, using flake8-pytest-style==1.3. When running the flake8 test suite under python3.8 or later it passes with no errors. When running it under 3.7 or 3.6:

src/tox/pytest.py:66:1: PT004 fixture 'ensure_logging_framework_not_altered' does not return anything, add leading underscore
src/tox/pytest.py:111:1: PT004 fixture 'check_os_environ_stable' does not return anything, add leading underscore
src/tox/pytest.py:118:1: PT004 fixture 'no_color' does not return anything, add leading underscore
src/tox/pytest.py:274:1: PT004 fixture 'enable_pep517_backend_coverage' does not return anything, add leading underscore
src/tox/pytest.py:565:1: PT005 fixture '_invalid_index_fake_port' returns a value, remove leading underscore
tests/config/test_sets.py:13:1: PT005 fixture '_conf_builder' returns a value, remove leading underscore

How it should work

Consistently under all python versions.

System information

• Operating system: Fedora/Ubuntu
• Python version: Python3.8/3.9/3.7/3.6
• flake8 version: flake8-bugbear==20.11.1 flake8-comprehensions==3.3.1 flake8-pytest-style==1.3 flake8-spellcheck==0.23 flake8-unused-arguments==0.0.6 flake8==3.8.4

As discovered under https://github.com/tox-dev/tox/pull/1970 by @jugmac00

[m-burst]

Hi @gaborbernat, Thank you for the report. I have reproduced the error locally, will investigate further.

[m-burst]

I have figured out the reason why this inconsistency occurs:

1. tox has noqa for PT004/PT005 for a few fixtures
2. We use the lineno of the AST node to check for noqa and to ultimately decide whether to report the error (flake8 does not handle this automatically, so each plugin has to do it by itself)
3. In 3.8 the reported lineno for function definitions with decorators was changed: before 3.8 it was the line with the first decorator, and since 3.8 it is the line with the def

As a result, under 3.7- we try to find the noqa comment for PT004 on the wrong line, fail to find it and report the error.

I am not sure how to proceed right now, will try to figure out a solution.

In the meantime, the (ugly) workaround is to add a duplicate noqa comment on the fixtures where the PT004/PT005 are reported, e.g.:

@pytest.fixture(autouse=True)  # noqa: PT004 <-- add this for 3.7-
def ensure_logging_framework_not_altered() -> Iterator[None]:  # noqa: PT004  <-- this was already added, works for 3.8+
    ...

[gaborbernat]

• We use the lineno of the AST node to check for noqa and to ultimately decide whether to report the error (flake8 does not handle this automatically, so each plugin has to do it by itself)

Do we have an issue with this in flake8? Sounds like something that should be shared across plugins (but ideally provided by flake8).

I am not sure how to proceed right now, will try to figure out a solution.

Sounds to me like you want to make the where to look for noqa dynamic based on the python version.

[m-burst]

Do we have an issue with this in flake8? Sounds like something that should be shared across plugins (but ideally provided by flake8).

I took a closer look, and it seems that flake8 did implement this at some point, so my knowledge on this is not up-to-date.

Regardless of this, flake8 does suffer from the same inconsistency between Python version with some of their checks, see an example issue in their tracker.