Suppressed Pylint Warnings not working when none of new warnings enabled

ppiasek commented 3 weeks ago

Describe the bug I installed pylint-pytest plugin only to suppress pylint warnings related to pytest package usage. All introduced new warnings are disabled because other linters are checking them.

For some reason, the W0621 warning was constantly displayed on my test file, when the pytest fixture was defined and later used in the tests. After adding any newly introduced warnings to the config file, the W0621 warning stopped displaying.

To Reproduce Package versions

pylint==3.1.0
pylint-pytest==1.1.8
pytest==8.1.1
pytest-cov==5.0.0
pytest-mock==3.14.0

Folder structure Doesn't matter, just create a file with content below.

File content

import pytest

@pytest.fixture()
def my_fixture():
    return

def test_fixture(my_fixture):
    pass

pylint output with the plugin

$ python -m pylint tests/example.py 
************* Module tests.example
tests\example.py:9:17: W0621: Redefining name 'my_fixture' from outer scope (line 5) (redefined-outer-name)

-----------------------------------
Your code has been rated at 8.00/10

Expected behavior W0621 warning is suppressed by pylint-pytest plugin when fixture and tests using it are defined in the same file.

stdedos commented 3 weeks ago

It seems that we do have a test case exactly like the one you are describing:

https://github.com/pylint-dev/pylint-pytest/blob/057510452686eedbbced44154c526ffd6e09d2ad/tests/input/redefined-outer-name/caller_not_a_test_func.py#L1-L11

(idk why it writes there "invalid test case" - probably it means "this is not a valid test case, since it does not start with test (https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#conventions-for-python-test-discovery))

Extending our test case

$ bat -pp tests/test_caller_not_a_test_func.py
import pytest

@pytest.fixture
def this_is_a_fixture():
    return True

def not_a_test_function(this_is_a_fixture):
    # invalid test case...
    assert True

def test_function_valid(this_is_a_fixture):
    assert True

and running pylint against it gives:

$ pylint tests --disable=C
************* Module test_caller_not_a_test_func
tests/test_caller_not_a_test_func.py:9:24: W0621: Redefining name 'this_is_a_fixture' from outer scope (line 5) (redefined-outer-name)
tests/test_caller_not_a_test_func.py:9:24: W0613: Unused argument 'this_is_a_fixture' (unused-argument)
tests/test_caller_not_a_test_func.py:14:24: W0621: Redefining name 'this_is_a_fixture' from outer scope (line 5) (redefined-outer-name)
tests/test_caller_not_a_test_func.py:14:24: W0613: Unused argument 'this_is_a_fixture' (unused-argument)
************* Module test_example
tests/test_example.py:7:4: W0101: Unreachable code (unreachable)
tests/test_example.py:10:17: W0621: Redefining name 'my_fixture' from outer scope (line 5) (redefined-outer-name)

-------------------------------------------------------------------
Your code has been rated at 5.38/10 (previous run: 10.00/10, -4.62)

It does certainly intrigue me why:

Our test works correctly
Our test fails to work, when moved to the (bare-bones)

stdedos commented 3 weeks ago

... scratch all of the above: is pylint-pytest enabled?

$ pylint --generate-rcfile | grep pylint_pytest
load-plugins=pylint_pytest

I had the same issue without it; loading it, it gives:

$ pylint tests --disable=C
************* Module test_caller_not_a_test_func
tests/test_caller_not_a_test_func.py:9:24: W0621: Redefining name 'this_is_a_fixture' from outer scope (line 5) (redefined-outer-name)
tests/test_caller_not_a_test_func.py:9:24: W0613: Unused argument 'this_is_a_fixture' (unused-argument)
************* Module test_example
tests/test_example.py:7:4: W0101: Unreachable code (unreachable)

------------------------------------------------------------------
Your code has been rated at 7.69/10 (previous run: 5.38/10, +2.31)

i.e. what's expected

PS: Worry not about tests/test_example.py:7:4: W0101: Unreachable code (unreachable):

$ bat -pp tests/test_example.py
import pytest

@pytest.fixture()
def my_fixture():
    return True
    return False

def test_fixture(my_fixture):
    assert my_fixture

I was making sure that the function was picked up as a test function

ppiasek commented 3 weeks ago

Yeah I had that plugin enabled I even debugged the pylint run to make sure it was loaded and it was.

stdedos commented 3 weeks ago

Okay ... but what about now? Is it still happening?

Remember that technically tests/example.py is not a valid test file name

ppiasek commented 1 week ago

Yes, it is.

$ cat tests/test_fixture_issue.py 
import pytest

@pytest.fixture()
def my_fixture():
    return True

def test_fixture(my_fixture):
    assert my_fixture

$ pylint tests --disable=C
************* Module test_fixture.tests.test_fixture_issue
tests\test_fixture_issue.py:9:17: W0621: Redefining name 'my_fixture' from outer scope (line 5) (redefined-outer-name)

-----------------------------------
Your code has been rated at 8.00/10

PS. Sorry for late response, I totally missed your question.

stdedos commented 1 week ago

No worries. But "try to keep the loop", if it is possible 🙃

I have tried again reproducing your issue, but I fail to do so:

ua@h /tmp/pylint_pytest_testbench$ pylint tests --disable=C

-------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 7.69/10, +2.31)

ua@h /tmp/pylint_pytest_testbench$ pylint tests
************* Module test_example
tests/test_example.py:10:0: C0304: Final newline missing (missing-final-newline)
tests/test_example.py:1:0: C0114: Missing module docstring (missing-module-docstring)
tests/test_example.py:5:0: C0116: Missing function or method docstring (missing-function-docstring)
tests/test_example.py:9:0: C0116: Missing function or method docstring (missing-function-docstring)

-------------------------------------------------------------------
Your code has been rated at 2.00/10 (previous run: 10.00/10, -8.00)

ua@h /tmp/pylint_pytest_testbench$ ls -lah
total 12K
drwxrwxr-x   5 abcdefg abcdefg  160 Ιουν 27 00:11 .
drwxrwxrwt 118 root    root    3,4K Ιουν 27 00:11 ..
-rw-rw-r--   1 abcdefg abcdefg   37 Ιουν 27 00:07 .pylintrc
drwxrwxr-x   3 abcdefg abcdefg  120 Ιουν 27 00:11 .pytest_cache
-rw-rw-r--   1 abcdefg abcdefg   79 Ιουν 27 00:08 README.md
-rw-rw-r--   1 abcdefg abcdefg   87 Ιουν 27 00:07 requirements.txt
drwxrwxr-x   3 abcdefg abcdefg   80 Ιουν 27 00:11 tests
drwxrwxr-x   5 abcdefg abcdefg  140 Ιουν 27 00:07 .venv
ua@h /tmp/pylint_pytest_testbench$ tree
.
├── README.md
├── requirements.txt
└── tests
    ├── __pycache__
    │   └── test_example.cpython-38-pytest-8.1.1.pyc
    └── test_example.py

2 directories, 4 files

pylint_pytest_testbench.tar.gz

Am I missing something? 😕

ppiasek commented 1 week ago

Alright, I found the issue. Shorter version of my pyproject.toml config:

[tool.pylint.main]
disable = "all"
enable=["C0114", "W0621"]
jobs = 1
py-version = "3.11"
load-plugins = [
  "pylint.extensions.bad_builtin",
  "pylint.extensions.broad_try_clause",
  "pylint.extensions.code_style",
  "pylint.extensions.confusing_elif",
  "pylint.extensions.consider_refactoring_into_while_condition",
  "pylint.extensions.consider_ternary_expression",
  "pylint.extensions.dict_init_mutate",
  "pylint.extensions.docparams",
  "pylint.extensions.dunder",
  "pylint.extensions.empty_comment",
  "pylint.extensions.eq_without_hash",
  "pylint.extensions.for_any_all",
  "pylint.extensions.mccabe",
  "pylint.extensions.no_self_use",
  "pylint.extensions.overlapping_exceptions",
  "pylint.extensions.private_import",
  "pylint.extensions.redefined_variable_type",
  "pylint.extensions.set_membership",
  "pylint.extensions.typing",
  "pylint.extensions.while_used",
  "pylint_pytest",
]

That disable="all" part causes the issue. But I don't know why. I only needed checks in my "enable" part, so I am disabling all before, as explained in pylint's help.

EDIT: Sorry, I should add a config example in my issue description.

stdedos commented 1 week ago

Okay - now I replicated it

$ pylint tests --disable=all --enable=C0114 --enable=W0621
************* Module test_example
tests/test_example.py:1:0: C0114: Missing module docstring (missing-module-docstring)
tests/test_example.py:9:17: W0621: Redefining name 'my_fixture' from outer scope (line 5) (redefined-outer-name)

------------------------------------------------------------------
Your code has been rated at 6.00/10 (previous run: 6.00/10, +0.00)

I'll see what I can do

stdedos commented 6 days ago

I know at least from where the issue is coming from:

"Somehow", when using pylint tests --disable=all --enable=C0114 --enable=W0621, the check https://github.com/pylint-dev/pylint-pytest/blob/8310a88916e4b0b7462fc525c56d60b9af4dc126/pylint_pytest/checkers/variables.py#L103 returns false.

Which means that, the seeding function https://github.com/pylint-dev/pylint-pytest/blob/46cf4500a51d3e25cabaa9c651ec1d8c2b8754ac/pylint_pytest/checkers/fixture.py#L131 is not executed either.

We depend on pytest telling us which function names are fixtures.

I'll have to ask pylint for help on that one.

stdedos commented 5 days ago

... there might be a simpler explanation for that issue (and totally/trivially our fault), but I'd like to verify it first

stdedos commented 5 days ago

I have debugged the issue on the https://github.com/pylint-dev/pylint-pytest/releases/tag/v2.0.0a0, and it seems to be easier to figure out there:

Given the way that pylint is invoked i.e., catch-all --disable, it seems that our checker FixtureChecker is deemed as useless, and its hooks not visited (pertinent to your issue, specifically visit_module). (_And verified: https://github.com/pylint-dev/pylint/blob/d281f2fbf9672706ac07ee45d7cff3c4e2ff83bb/pylint/utils/ast_walker.py#L54-L57_)

There is a way to make it work, specifically for your case - there be dragons, though

It has a very simple workaround **in v2.0.0a0**: ```patch diff --git a/pylint_pytest/checkers/variables.py b/pylint_pytest/checkers/variables.py index 7dde882..df2bdb5 100644 --- a/pylint_pytest/checkers/variables.py +++ b/pylint_pytest/checkers/variables.py @@ -4,7 +4,6 @@ from astroid import Arguments, Module from astroid.nodes.node_ng import NodeNG from pylint.checkers.variables import VariablesChecker from pylint.interfaces import Confidence from pylint_pytest.utils import _can_use_fixture, _is_same_module from .fixture import FixtureChecker @@ -13,6 +12,15 @@ from .fixture import FixtureChecker class CustomVariablesChecker(VariablesChecker): """Overrides the default VariablesChecker of pylint to discard unwanted warning messages""" + def visit_module(self, node: NodeNG) -> None: + FixtureChecker.visit_module(self, node) + + # def visit_decorators(self, node: NodeNG) -> None: + # FixtureChecker.visit_decorators(self, node) + + # def visit_functiondef(self, node: NodeNG) -> None: + # FixtureChecker.visit_functiondef(self, node) + # pylint: disable=protected-access # this class needs to access the fixture checker registries ``` However it will fail when activating `visit_functiondef` - or fail to fact-check ``` tests/caller_not_a_test_func.py:9:1: W6403: Using a deprecated positional arguments for fixture (deprecated-positional-argument-for-pytest-fixture) ``` in ```patch diff --git a/tests/input/redefined-outer-name/caller_not_a_test_func.py b/tests/input/redefined-outer-name/caller_not_a_test_func.py index 985a519..67d59ea 100644 --- a/tests/input/redefined-outer-name/caller_not_a_test_func.py +++ b/tests/input/redefined-outer-name/caller_not_a_test_func.py @@ -6,6 +6,7 @@ def this_is_a_fixture(): return True +@pytest.fixture("function") def not_a_test_function(this_is_a_fixture): # invalid test case... assert True ``` .. and it stands to reason that we should actually do that 😕

For the "soon coming"™️ https://github.com/pylint-dev/pylint-pytest/releases/tag/v2.0.0, I guess I should try to figure out a PassiveChecker of sorts that could be delegated to, to visit and build the fixture information that we need - which then both CustomVariablesChecker and FixtureChecker could ask for that information.

I am not sure what should I do for the v1.x series. v2.x is not "backwards incompatible" code-wise per se, but I want to carve it out for semantic reasons (also big refactorings, like https://github.com/pylint-dev/pylint-pytest/pull/29)

ppiasek commented 4 days ago

For me, I can wait till v2.x if that helps you. Apparently, I'm the only one who was irritated enough to raise an issue. 😄 For now, we are just disabling that message, but would like to enable it in the future.

stdedos commented 4 days ago

Shameless advertizing:

What would really help me, is if you know a Python Powerhouse Dev that would be interested in figuring out this Heisenbug: https://github.com/pylint-dev/pylint-pytest/issues/68

I haven't spent "countless hours" on it, but I have spent enough to drive me a little crazy 😅

I mean ... idk. Maybe I should say to pylint to actually evaluate our rule, as it affects the W0621 message?

https://github.com/pylint-dev/pylint-pytest/blob/b493935542e199148670245617be5c08ddeefee0/pylint_pytest/checkers/fixture.py#L42-L74

We don't issue it of course, but we do "omit" it 😅

More things to ask from pylint, as I am in the process of learning the pylint ropes

ppiasek commented 4 days ago

I can try looking at it, maybe with some of my friends. Would be fun, I just dunno about my time availability. Will let you know.

pylint-dev / pylint-pytest

Suppressed Pylint Warnings not working when none of new warnings enabled #70