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

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

Open
orSolocate opened this issue May 6, 2022 · 13 comments
Labels
Lib specific 💅 This affect the code from a particular library Regression

Comments

@orSolocate
Copy link
Contributor

orSolocate commented May 6, 2022

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 orSolocate added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label May 6, 2022
@orSolocate orSolocate changed the title False Positive F0401: Unable to import when importing parent directory False Positive W0621: Redefining name %r from outer scope when using pytest fixtures May 6, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Crash 💥 A bug that makes pylint crash and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 6, 2022
@Pierre-Sassoulas
Copy link
Member

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

@DanielNoord
Copy link
Member

DanielNoord commented May 6, 2022

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

@mbyrnepr2
Copy link
Collaborator

mbyrnepr2 commented May 6, 2022

We should probably fix the crash

I can take a look.

#6532

@mbyrnepr2
Copy link
Collaborator

mbyrnepr2 commented May 6, 2022

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='').

@Pierre-Sassoulas
Copy link
Member

Related : #1535

@lqueryvg
Copy link

lqueryvg commented May 9, 2022

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.

@DanielNoord
Copy link
Member

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.

@nicktimko
Copy link

nicktimko commented Aug 5, 2022

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...

@orSolocate
Copy link
Contributor Author

orSolocate commented Aug 6, 2022

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?

@mbyrnepr2
Copy link
Collaborator

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.

@DanielNoord
Copy link
Member

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 DanielNoord added the Lib specific 💅 This affect the code from a particular library label Aug 23, 2022
@DanielNoord DanielNoord removed this from the 2.15.0 milestone Aug 23, 2022
@jacobtylerwalls jacobtylerwalls removed the Crash 💥 A bug that makes pylint crash label Sep 16, 2022
@C0DK
Copy link

C0DK commented Jan 19, 2023

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.

@Pierre-Sassoulas
Copy link
Member

Relevant discussion here: PyCQA/meta#56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Lib specific 💅 This affect the code from a particular library Regression
Projects
None yet
Development

No branches or pull requests

8 participants