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.

Sign up for GitHub

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

Clean up beyond_top_level tests #7279

Merged
merged 2 commits into from
Aug 12, 2022
Merged

Clean up beyond_top_level tests #7279

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3223ef9
Clean up beyond_top_level tests
DanielNoord Aug 9, 2022
cad40d6
Merge branch 'main' into test
jacobtylerwalls Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 41 additions & 26 deletions tests/checkers/unittest_imports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
import os

import astroid
import pytest
from pytest import CaptureFixture

from pylint import epylint as lint
from pylint.checkers import imports
from pylint.interfaces import UNDEFINED
from pylint.lint import Run
from pylint.testutils import CheckerTestCase, MessageTest

REGR_DATA = os.path.join(os.path.dirname(__file__), "..", "regrtest_data", "")
Expand Down Expand Up @@ -41,42 +41,57 @@ def test_relative_beyond_top_level(self) -> None:
self.checker.visit_importfrom(module.body[2].body[0])

@staticmethod
@pytest.mark.xfail(
reason="epylint manipulates cwd; these tests should not be using epylint"
)
def test_relative_beyond_top_level_two() -> None:
output, errors = lint.py_run(
f"{os.path.join(REGR_DATA, 'beyond_top_two')} -d all -e relative-beyond-top-level",
return_std=True,
def test_relative_beyond_top_level_two(capsys: CaptureFixture[str]) -> None:
Run(
[
f"{os.path.join(REGR_DATA, 'beyond_top_two')}",
"-d all",
"-e relative-beyond-top-level",
],
exit=False,
)
output, errors = capsys.readouterr()

top_level_function = os.path.join(
REGR_DATA, "beyond_top_two/namespace_package/top_level_function.py"
)
output2, errors2 = lint.py_run(
f"{top_level_function} -d all -e relative-beyond-top-level",
return_std=True,
Run(
[top_level_function, "-d all", "-e relative-beyond-top-level"],
exit=False,
)
output2, errors2 = capsys.readouterr()

assert len(output.readlines()) == len(output2.readlines())
assert errors.readlines() == errors2.readlines()
# The first package fails to lint
assert len(output.split("\n")) == 1
Comment on lines +64 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we leave this as 5 and xfail it? Tests are documentation, and I don't want to document the wrong result.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, maybe the result is correct if the expectation is that you have to cd to a namespace package you want to lint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was meaning to ask this. Is the new behavior how we want to do this? Or is this a temporary result because of improved but not perfect namespace package handling?

If this is desired (ie., you can't lint a package like this) I think it makes sense to just have this as a test itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for giving me time to double-check. Yes, you need to cd or otherwise do something fancy (with a hook?) if you want to find namespace packages. This matches the behavior of python's import statement, so far as I can tell. So the result is correct.

To go back to the original issue linked in these tests, we're just trying to test for the absence of no-name-in-module or relative-beyond-top-level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong about this after all. I guess the beyond_top_two is on sys.path somewhere, so it should be linted. (I guess because tests is on sys.path?) See #7304.

Ultimately new tests should create their own isolated packages with code, to make this easier to reason about!

assert len(output2.split("\n")) == 5
assert errors == errors2

@staticmethod
def test_relative_beyond_top_level_three() -> None:
output, errors = lint.py_run(
f"{os.path.join(REGR_DATA, 'beyond_top_three/a.py')} -d all -e relative-beyond-top-level",
return_std=True,
def test_relative_beyond_top_level_three(capsys: CaptureFixture[str]) -> None:
Run(
[
f"{os.path.join(REGR_DATA, 'beyond_top_three/a.py')}",
"-d all",
"-e relative-beyond-top-level",
],
exit=False,
)
assert len(output.readlines()) == 5
assert errors.readlines() == []
output, errors = capsys.readouterr()
assert len(output.split("\n")) == 5
assert errors == ""

@staticmethod
def test_relative_beyond_top_level_four() -> None:
output, errors = lint.py_run(
f"{os.path.join(REGR_DATA, 'beyond_top_four/module')} -d missing-docstring,unused-import",
return_std=True,
def test_relative_beyond_top_level_four(capsys: CaptureFixture[str]) -> None:
Run(
[
f"{os.path.join(REGR_DATA, 'beyond_top_four/module')}",
"-d missing-docstring,unused-import",
],
exit=False,
)
assert len(output.readlines()) == 5
assert errors.readlines() == []
output, errors = capsys.readouterr()
assert len(output.split("\n")) == 5
assert errors == ""

def test_wildcard_import_init(self) -> None:
module = astroid.MANAGER.ast_from_module_name("init_wildcard", REGR_DATA)
Expand Down