pylint icon indicating copy to clipboard operation
pylint copied to clipboard

False Positive W0621: Redefining name %r from outer scope when using pytest fixtures

Open orSolocate opened this issue 3 years ago • 11 comments

Bug description

Bug opened from SO issue

Testcase:

# test_wallet.py

@pytest.fixture
def my_wallet():
    '''Returns a Wallet instance with a zero balance'''
    return Wallet()

@pytest.mark.parametrize("earned,spent,expected", [
    (30, 10, 20),
    (20, 2, 18),
])
def test_transactions(my_wallet, earned, spent, expected):
    my_wallet.add_cash(earned)
    my_wallet.spend_cash(spent)
    assert my_wallet.balance == expected

They discuss a workaround to avoid the error and also a method of disabling the checker but this bug issue is about actually fixing it in Pylint, or at least discussing it.

Configuration

No response

Pylint output

`W0621: Redefining name %r from outer scope (line %s)'`

Expected behavior

The Pylint message should recognize this situation of pytest fixture decorator and don't raise a message.

Pylint version

pylint 2.8.4
astroid 2.5.8
Python 3.7.5

OS / Environment

Windows

Additional dependencies

No response

orSolocate avatar May 06 '22 21:05 orSolocate

Well, apparently this code crash on 2.14.0-dev0 and 2.13.8.

Exception on node <Decorators l.2 at 0x7f1be24aaa00> in file '/home/pierre/pylint/b.py'
Traceback (most recent call last):
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 73, in walk
    callback(astroid)
  File "/home/pierre/pylint/pylint/checkers/unsupported_version.py", line 64, in visit_decorators
    self._check_typing_final(node)
  File "/home/pierre/pylint/pylint/checkers/unsupported_version.py", line 79, in _check_typing_final
    for decorator in decorators or uninferable_final_decorators(node):
  File "/home/pierre/pylint/pylint/checkers/utils.py", line 825, in uninferable_final_decorators
    import_node = decorator.expr.lookup(decorator.expr.name)[1][0]
IndexError: tuple index out of range

Pierre-Sassoulas avatar May 06 '22 22:05 Pierre-Sassoulas

We should probably fix the crash, but the issue of handling pytest fixtures should probably be delegated to pytest-pylint.

DanielNoord avatar May 06 '22 22:05 DanielNoord

We should probably fix the crash

I can take a look.

https://github.com/PyCQA/pylint/pull/6532

mbyrnepr2 avatar May 06 '22 22:05 mbyrnepr2

I could be wrong but regarding the "Refined name from outer scope" message. Isn't this correct of Pylint in highlighting what is also mentioned in the SO (pytest docs)?

If a fixture is used in the same module in which it is defined, the function name of the fixture will be shadowed by the function arg that requests the fixture; one way to resolve this is to name the decorated function fixture and then use @pytest.fixture(name='').

mbyrnepr2 avatar May 06 '22 23:05 mbyrnepr2

Related : https://github.com/PyCQA/pylint/issues/1535

Pierre-Sassoulas avatar May 07 '22 12:05 Pierre-Sassoulas

How about my comment here: https://stackoverflow.com/questions/46089480/pytest-fixtures-redefining-name-from-outer-scope-pylint/54045715?noredirect=1#comment122655133_57015304 I.e. you get a similar problem with mocker. from pytest_mock import mocker followed by def test_my_thing(mocker) gives same lint error. Seems to be a recurring theme.

lqueryvg avatar May 09 '22 08:05 lqueryvg

These issues are probably best reported to https://github.com/reverbc/pylint-pytest/. Due to the way pytest creates its fixtures and mocks it's difficult for pylint to infer these as it does not follow the traditional/standard pattern of Pythonic code. Therefore, such issues are better handled by a pylint plugin that is focused on understanding those particular patterns. Similarly, there is https://github.com/PyCQA/pylint-django which does the same but then for common Django patterns. There is simply not enough maintainers-bandwidth to support all these plugins within the main pylint project.

DanielNoord avatar May 09 '22 09:05 DanielNoord

Not sure if there's a specific solution in mind for this, but if the OPs original code

@pytest.fixture
def my_wallet():
    '''Returns a Wallet instance with a zero balance'''

@pytest.mark.parametrize("earned,spent,expected", dataset)
def test_transactions(my_wallet, earned, spent, expected):
    ...
    assert my_wallet.balance == expected

was to emit no errors, I've also had issues where I'll accidentally forget to use the fixture (as an argument), or the correct name, and then I get really confused because the code technically still runs, there's no name redefinition, you just get something strange like an AttributeError: my_wallet has no attribute 'balance', and you'll be like, "yes it does, it's the wallet from the fixture, wtf"

@pytest.fixture
def my_wallet():
    '''Returns a Wallet instance with a zero balance'''

@pytest.mark.parametrize("earned,spent,expected", dataset)
def test_transactions(earned, spent, expected):
    ...
    assert my_wallet.balance == expected  # my_wallet is the "raw" fixture, not the fixture's result

A 'fix' is to do something like

@pytest.fixture(name="my_wallet")
def fixture_my_wallet():
    ...

but it can get a little tedious. My dumb wish is that there was just a prefix you could slap on to functions (e.g. fixture_) that would make them fixtures, but get stripped off when used by the arguments. A prefix like test_ is commonly used for test functions.

# @pytest.fixture(name="my_wallet")  <-- i.e. with or w/o this line, it would have the exact same behavior
def fixture_my_wallet():
    ...

But that's all in the PyTest camp...

nicktimko avatar Aug 05 '22 20:08 nicktimko

We should probably fix the crash

I can take a look.

#6532

@mbyrnepr2 - Hey mate should I make you an assignee for fixing the crash?

orSolocate avatar Aug 06 '22 09:08 orSolocate

Hey @orSolocate that's OK with me. I wonder can we close this one because what @DanielNoord said is an idea I've seen before on other similar issues where Pylint can't anticipate the custom coding patterns used by arbitrary 3rd party tools.

My understanding is that for this issue the solution is to use pylint-pytest; although the status of that project is unclear.

mbyrnepr2 avatar Aug 06 '22 10:08 mbyrnepr2

I agree @mbyrnepr2 (with my own earlier statements 😬). This is code that is too difficult to understand for pylint normally, so it should be handled by a plug-in.

I have started working on my own pytest plugin and this will probably be fixed by it. I'm waiting on confirmation that the current pytest plug-in is unmaintained to start working on it again. I'll come back to this issue when there is any development. For now I think this can stay open as it helps direct users with pytest related issues.

DanielNoord avatar Aug 07 '22 12:08 DanielNoord

although the status of that project is unclear.

Since the last status on this issue, https://github.com/reverbc/pylint-pytest has been archived, and would probably require a new maintainer for that to be the solution.

However, I do see the very valid point that pytest simply isn't done in a pythonic way and a plugin has to fix the issue. I'm hoping someone will pick up the torch, as I'd love to both use pytest as well as pylint.

C0DK avatar Jan 19 '23 05:01 C0DK

Relevant discussion here: https://github.com/PyCQA/meta/issues/56

Pierre-Sassoulas avatar Jan 19 '23 18:01 Pierre-Sassoulas