Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ppiasek opened this issue Jun 9, 2024 · 14 comments
Open
Labels
bug Something isn't working

Comments

@ppiasek
Copy link

ppiasek commented Jun 9, 2024

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.

@ppiasek ppiasek added the bug Something isn't working label Jun 9, 2024
@stdedos
Copy link
Collaborator

stdedos commented Jun 10, 2024

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

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

(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:

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

@stdedos
Copy link
Collaborator

stdedos commented Jun 10, 2024

... 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
Copy link
Author

ppiasek commented Jun 13, 2024

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

@stdedos
Copy link
Collaborator

stdedos commented Jun 13, 2024

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

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

@ppiasek
Copy link
Author

ppiasek commented Jun 26, 2024

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
Copy link
Collaborator

stdedos commented Jun 26, 2024

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
Copy link
Author

ppiasek commented Jun 27, 2024

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
Copy link
Collaborator

stdedos commented Jun 28, 2024

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
Copy link
Collaborator

stdedos commented Jul 1, 2024

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

"Somehow", when using pylint tests --disable=all --enable=C0114 --enable=W0621, the check

and node.name in FixtureChecker._pytest_fixtures
returns false.

Which means that, the seeding function

FixtureChecker._pytest_fixtures = fixture_collector.fixtures
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
Copy link
Collaborator

stdedos commented Jul 2, 2024

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

@stdedos
Copy link
Collaborator

stdedos commented Jul 2, 2024

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:

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

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 #29)

@ppiasek
Copy link
Author

ppiasek commented Jul 3, 2024

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
Copy link
Collaborator

stdedos commented Jul 3, 2024

Shameless advertizing:

What would really help me, is if you know a Python Powerhouse Dev that would be interested in figuring out this Heisenbug: #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?

class FixtureChecker(BasePytestChecker):
msgs = {
"W6401": (
"Using a deprecated @pytest.yield_fixture decorator",
"deprecated-pytest-yield-fixture",
"Used when using a deprecated pytest decorator that has been deprecated in pytest-3.0",
),
"W6402": (
"Using useless `@pytest.mark.*` decorator for fixtures",
"useless-pytest-mark-decorator",
(
"@pytest.mark.* decorators can't by applied to fixtures. "
"Take a look at: https://docs.pytest.org/en/stable/reference.html#marks"
),
),
"W6403": (
"Using a deprecated positional arguments for fixture",
"deprecated-positional-argument-for-pytest-fixture",
(
"Pass scope as a kwarg, not positional arg, which is deprecated in future pytest. "
f"Take a look at: {ARGUMENT_ARE_KEYWORD_ONLY}"
),
),
"F6401": (
(
"pylint-pytest plugin cannot enumerate and collect pytest fixtures. "
"Please run `pytest --fixtures --collect-only %s` and resolve "
"any potential syntax error or package dependency issues. stdout: %s. stderr: %s."
),
"cannot-enumerate-pytest-fixtures",
"Used when pylint-pytest has been unable to enumerate and collect pytest fixtures.",
),
}

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
Copy link
Author

ppiasek commented Jul 3, 2024

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.

stdedos added a commit that referenced this issue Jul 13, 2024
Write the positive test of `redefined-outer-name`
(#70)

Signed-off-by: Stavros Ntentos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants