Not working

[Tobbeman]

Hello, this looks like a great plugin for my usecase! Sadly I have issues, does not look like it is working and I'm not sure why

e2e_test.py:

"""
This module contain end to end tests on a site. End to end in this context
includes activation and basic status check
"""

def test_activation(virtual_site):
    """
    'Dummy' test that will ensure the activation is working
    The actual activation is done in the fixture
    """
    assert True

conftest.py:

@pytest.fixture(scope="session")
def virtual_site():
-- redacted --

result in

$ pylint e2e_test.py 
************* Module e2e_test
e2e_test.py:7:20: W0613: Unused argument 'virtual_site' (unused-argument)

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

pyproject.toml:

[tool.black]
line-length = 79

[tool.pylint.'MASTER']
load-plugins = "pylint_pytest"

[tool.pylint.'FORMAT']
max-line-length = 79

env:

$ pylint --version
pylint 2.7.1
astroid 2.5
Python 3.6.9 (default, Oct  8 2020, 12:12:24) 
[GCC 8.4.0]
$ pytest --version
pytest 6.2.2
$ pipenv --version
pipenv, version 2020.11.15

I realize this might not be much to go on, please let me know if I can provide more info :)

[Tobbeman]

Ok, something really wierd is going on Looks like it was working, before I refactored another component out of conftest.py. The only connection the I can see is that fixture virtual_site is using said component... Otherwise it should have no impact.

No idea how to dig deeper...

[reverbc]

Hi, thanks for trying out this plugin!

Unfortunately I'm not able to reproduce the issue with given code snippets :/

If you can help to provide more code example on how to reproduce the issue e.g. illustrate the pre-refactor architecture and how the fixture in conftest references to that other component that'll be super helpful.

[Tobbeman]

Hello,

Sorry, I cannot seem to remake it in another env..

This is the diff of the affected files:

diff --git a/cloud_api/__init__.py b/cloud_api/__init__.py
new file mode 100644
index 0000000..00347e8
--- /dev/null
+++ b/cloud_api/__init__.py
@@ -0,0 +1,5 @@
+"""
+This module contain all code needed to interact with the cloud api.
+"""
+from .extend import wait_for_installation
+from .cloud_api import CloudApi

--- retracted ---

diff --git a/conftest.py b/conftest.py
index 7d9c678..6d386fb 100644
--- a/conftest.py
+++ b/conftest.py
@@ -10,7 +10,6 @@ import paramiko
 import env
 import cloud_api

-
 LOGGER = logging.getLogger(__name__)

@@ -204,8 +203,8 @@ def virtual_site(virtual_cpe):  # pylint: disable=redefined-outer-name
     LOGGER.info("Waiting for virtual site to be ready")

     time.sleep(env.ACTIVATION_INITIAL_WAIT)
-    success, error = api.wait_for_installation(
-        env.BE_ORG, env.BE_SITE, env.INSTALLATION_TIMEOUT
+    success, error = cloud_api.wait_for_installation(
+        api, env.BE_ORG, env.BE_SITE, env.INSTALLATION_TIMEOUT
     )
     assert success, error

I'm not this is more helpful than before Like I mentioned, I cannot seem to recreate it, so something funky is happening in my env. I have resorted to silencing the warnings manually

[reverbc]

I'm sorry but I still can't reproduce the issue on my end to dig deeper.

The way this plugin works for unused-argument, is first to collect all the fixtures (https://github.com/reverbc/pylint-pytest/blob/pylint-pytest-1.0.0/pylint_pytest/checkers/fixture.py#L73-L78), then a monkey patch (🤮) to pylint's built-in VariableChecker (https://github.com/reverbc/pylint-pytest/blob/pylint-pytest-1.0.0/pylint_pytest/checkers/fixture.py#L38). When we run into warnings like unused-argument, we check 1) if this particular argument is a known fixture to pytest (from the collection mentioned above), and 2) if the function has this argument can use fixture (either a test method, or itself is a fixture).

Since the original e2e_test.py::test_activation has the right naming convention to be a test case, I think it fulfills the 2). If you can run pytest e2e_test --fixtures --collect-only and see if virtual_site fixture is in the list that'll be a good way to start.

[OlgaPaw]

I prepared a non-wroking project exaple. See actions for the errors. https://github.com/OlgaPaw/python_template

pylint --load-plugins pylint_pytest app                      
************* Module app.tests.test_dummy
app/tests/test_dummy.py:5:0: W0612: Unused variable 'setup' (unused-variable)
app/tests/test_dummy.py:9:0: W0612: Unused variable 'test_foo' (unused-variable)

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

I also observed that plugin does not work when I have custom conftest.py (containing imports)

[reverbc]

@OlgaPaw I'm not sure if your provided example is relevant to the original problem.

1. This plugin does not suppress unused-variable warnings. See https://github.com/reverbc/pylint-pytest#suppressed-pylint-warnings for the list of warnings that it's interested in
2. After checking your example, I found the plugin does work:

   
   ((pylint-pytest-Q6jowuRR)) ❱ pylint app --reports=n
   ************* Module app.tests.test_dummy
   app/tests/test_dummy.py:9:13: W0621: Redefining name 'setup' from outer scope (line 5) (redefined-outer-name)
   app/tests/test_dummy.py:9:13: W0613: Unused argument 'setup' (unused-argument)
   app/tests/test_dummy.py:5:0: W0612: Unused variable 'setup' (unused-variable)
   app/tests/test_dummy.py:9:0: W0612: Unused variable 'test_foo' (unused-variable)

---

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

((pylint-pytest-Q6jowuRR)) ❱ pylint app --reports=n --load-plugins pylint_pytest ***** Module app.tests.test_dummy app/tests/test_dummy.py:5:0: W0612: Unused variable 'setup' (unused-variable) app/tests/test_dummy.py:9:0: W0612: Unused variable 'test_foo' (unused-variable)

---

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

See the `redefined-outer-name` and `unused-argument` false positives are suppressed after enabling the plugin.

The `unused-variable` warnings actually came from option `allow-global-unused-variables` in your project's pylintrc, which by default should be `yes` (not to raise warnings) (see [pylint 1.7 release note](http://pylint.pycqa.org/en/latest/whatsnew/1.7.html#bug-fixes)). Since pytest is doing lots of magic to automatically collect and execute test cases, it makes sense not to turn it on when linting test folder. If you have use cases to suppress that warning, please create a separate a feature request ticket.

[OlgaPaw]

My bad, thanks @reverbc for marking it

[webknjaz]

e2e_test.py:7:20: W0613: Unused argument 'virtual_site' (unused-argument)

This sounds reasonable. If you're not going to use the fixture's return value, just apply it via @pytest.mark.usefixtures('virtual_site').

Another pytest linting plugin (but for flake8) even enforces this approach and I think this would be the right thing to do: https://github.com/m-burst/flake8-pytest-style/blob/master/docs/rules/PT019.md.

[reverbc]

e2e_test.py:7:20: W0613: Unused argument 'virtual_site' (unused-argument)

This sounds reasonable. If you're not going to use the fixture's return value, just apply it via @pytest.mark.usefixtures('virtual_site').

Another pytest linting plugin (but for flake8) even enforces this approach and I think this would be the right thing to do: https://github.com/m-burst/flake8-pytest-style/blob/master/docs/rules/PT019.md.

I do like this pattern for explicitly using a fixture without accessing its return/yield value. However, due to pytest limitation that @pytest.mark.usefixtures has no effect in fixture functions, we'll need to have separate handling for test methods and fixture functions. That's an implementation detail tho.

I think we can make a new convention rule to suggest this change since it's still a valid usage.