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

Recognize pytest fixtures used as arguments that are implicitly accessed #1059

Closed
engelfranziska opened this issue Mar 17, 2021 · 5 comments
Closed
Labels
waiting for user response Requires more information from user

Comments

@engelfranziska
Copy link

This issue is derived from #684 and concerns using fixtures across files as described in the documentation on sharing fixtures across multiple where the function test_order seems to describe such a case.

The MWE below reproduces the issue:

information on system/versions:

  • Win10.0.19042 Build 19042
  • VSCode1.54.3
  • Python 3.9.2
  • Pylance 2021.3.1
  • pytest 6.2.2

the mymain.py file:

"""Main python file."""


def main(text: str = 'default text'):
    """Return the text."""
    return text

the conftest.py file

"""Test configuration file."""

import pytest


@pytest.fixture
def some_fixture():
    """A fixture to be used in test_main."""
    return 'fixture provided text'


@pytest.fixture
def another_fixture():
    """A fixture to be used in test_main."""
    print('printing from another fixture')

and the test_main.py file

"""Testing main."""

import pytest
import mymain


def test_main(some_fixture: str):
    """Testing main with the fixture."""
    returned = mymain.main(some_fixture)
    assert returned == 'fixture provided text'


def test_main2(another_fixture: None):  # this is where pylance complains
    """Testing fixture as argument."""


@pytest.mark.usefixtures('another_fixture')  # this is accepted just fine (valid alternative)
def test_main3():
    """Testing fixture with usefixtures."""

What works fine?

  • Everything in this setup works fine with respect two running pytest and getting the expected results (every test passes and everything is printed that should be).
  • test_main and test_main3 are completely fine according to pylance, since in case one the argument is used within the function and in test_main3 the fixture used is hidden in a string. I guess in both cases pylance does not care or know about the fixtures being used.

What is (kind of) not 'expected'?

As described in #684, the implicitly used fixture another_fixture in test_main2 is detected as not accessed.
The tests show that the fixture is indeed used, since the expected text is printed, but pylance does not know/recognize that.
(pylint (2.7.2) throws the unused-argument warning)

What would be expected (or nice)?

Since pytest obviously works just fine and "accesses" the fixtures in each case, pylance should reflect that by not indicating the fixture as not accessed.

Running pytest -v -s gives something like this (test order is reversed)

========================== test session starts ====================================
platform win32 -- Python 3.9.2, pytest-6.2.2, py-1.10.0, pluggy-0.13.1 -- <line continued>
... C:\...\WindowsApps\PythonSoftwareFoundation.Python.3.9_qbz5n2kfra8p0\python.exe
cachedir: .pytest_cache
rootdir: C:\...\pylance_fixture_conftest
plugins: cov-2.11.1, depends-1.0.1
collected 3 items


test_main.py::test_main3 printing from another fixture
PASSED
test_main.py::test_main2 printing from another fixture
PASSED
test_main.py::test_main PASSED

============================= 3 passed in 0.04s ===================================
@jakebailey
Copy link
Member

Are you seeing something more strong than just the dimming and italicization of the parameter in the UI? We do not emit a "real" diagnostic for unaccessed variables, only a hint to the UI to display it as unused. The thread you linked talks about this.

If you're referring to "pylint (2.7.2) throws the unused-argument warning", we aren't pylint and aren't in control of its linting rules. The way pylint works in VS Code would likely show a real squiggle. Normally pylint is disabled by the Python extension if Pylance is running and must be explicitly opted into.

@jakebailey jakebailey added the waiting for user response Requires more information from user label Mar 17, 2021
@github-actions github-actions bot removed the triage label Mar 17, 2021
@engelfranziska
Copy link
Author

You are spot on. It is pylint that is doing the "more drastic hinting". If disabled (again) the variable is only dimmed in my case. Nevertheless the pylint comment was just a side note anyway. I am sorry if I made the impression you were one and the same.

So am I correct to assume that this "false-positive", which I admit is not that much of a problem, will be tolerated since it is more or less founded in pytests "unique way" of handling this stuff?

Or is it not considered a false-positive?

I was curious since one of the comments on the thread mentioned above was about whether or not pylance will support or be able to resolve those pytest specific features in the future.

@erictraut
Copy link
Contributor

This falls into the gray area between "false positive" and "expected behavior". The problem is that pytest is doing some "magical" things at runtime that are not discoverable through static analysis of the code. Pylance is using the grayed text to give you a hint that it cannot find any usage of these symbols. Think of it as a subtle cue to you, the developer. If it's expected (as it is in this case) that they these symbols are not referenced, you can ignore it. But sometimes you might see the dimmed text and think, "boy, that's unexpected, maybe I have a bug here".

@jakebailey
Copy link
Member

jakebailey commented Mar 17, 2021

So am I correct to assume that this "false-positive", which I admit is not that much of a problem, will be tolerated since it is more or less founded in pytests "unique way" of handling this stuff?

Or is it not considered a false-positive?

It's not a "false positive" from the analysis' point of view. The function has an unused parameter, and we aren't aware of this special pytest behavior which (ab)uses function parameters to perform other operations external to the function. That's going outside the bounds of what we're doing as a type checker, at least from the "we implement PEP-defined behaviors" point of view. I'm not entirely certain we'd want to change this behavior without a lot of feedback; it's only a visual indicator for the UI and doesn't show up in the problems window as a "true" diagnostic (though does show on hover). Maybe in the future there could be a plugin (if we ever have plugins) to hide this, but that seems like overkill for a visual indicator.

I was curious since one of the comments on the thread mentioned above was about whether or not pylance will support or be able to resolve those pytest specific features in the future.

There was a comment in the thread about supporting pytest completions for fixtures, which is a different enhancement and involves different work. As far as I'm aware, nobody ever opened up that feature request or defined how they wanted it to work.

@engelfranziska
Copy link
Author

Okay, that pretty much answers my questions and clarifies things.
Thanks a lot for the insight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for user response Requires more information from user
Projects
None yet
Development

No branches or pull requests

3 participants