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

Primer tests "à la mypy" #5173

Merged
merged 10 commits into from
Nov 24, 2021
36 changes: 35 additions & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -508,4 +508,38 @@ jobs:
run: |
. venv/bin/activate
pip install -e .
pytest -m primer_stdlib -n auto
pytest -m primer -n auto -k test_primer_stdlib_no_crash

pytest-primer-external:
Copy link
Member

Choose a reason for hiding this comment

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

IMO it would make sense to create a new workflow file for it. That will make it easier to distinguish for all the other tests in the Github UI.

Only small downside, you need to compy the prepare-tests-linux job.

Copy link
Member Author

Choose a reason for hiding this comment

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

name: Run primer tests on external libs Python ${{ matrix.python-version }} (Linux)
runs-on: ubuntu-latest
needs: prepare-tests-linux
strategy:
matrix:
python-version: [3.8, 3.9, "3.10"]
steps:
- name: Check out code from GitHub
uses: actions/checkout@v2.3.5
- name: Set up Python ${{ matrix.python-version }}
id: python
uses: actions/setup-python@v2.2.2
with:
python-version: ${{ matrix.python-version }}
- name: Restore Python virtual environment
id: cache-venv
uses: actions/cache@v2.1.6
with:
path: venv
key:
${{ runner.os }}-${{ steps.python.outputs.python-version }}-${{
needs.prepare-tests-linux.outputs.python-key }}
- name: Fail job if Python cache restore failed
if: steps.cache-venv.outputs.cache-hit != 'true'
run: |
echo "Failed to restore Python venv from cache"
exit 1
- name: Run pytest
run: |
. venv/bin/activate
pip install -e .
pytest -m primer -n auto -k test_primer_external_packages_no_crash
Copy link
Member

Choose a reason for hiding this comment

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

pytest isn't quite ideal as it only displays the test output if it fails. Could be difficult to detect regressions, at lot a new false-positives, that way.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We're going to handle new false positives later on, right now we're just checking for fatal messages and crashes.

Copy link
Member Author

Choose a reason for hiding this comment

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

1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ build-stamp
.pytest_cache/
.mypy_cache/
.benchmarks/
.pylint_primer_tests/
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ Release date: TBA

Closes #4982

* Introduced primer tests and a configuration tests framework. The helper classes available in
``pylint/testutil/`` are still unstable and might be modified in the near future.

Closes #4412 #5287
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved

* Fix ``install graphiz`` message which isn't needed for puml output format.

* Fix a crash in the ``check_elif`` extensions where an undetected if in a comprehension
Expand Down
7 changes: 6 additions & 1 deletion doc/whatsnew/2.12.rst
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ New checkers

* Fix ``useless-super-delegation`` false positive when default keyword argument is a variable.

* Added new checker `use-implicit-booleanness``: Emitted when using collection
* Added new checker ``use-implicit-booleanness``: Emitted when using collection
literals for boolean comparisons

* Added new checker ``use-implicit-booleaness-not-comparison``: Emitted when
Expand Down Expand Up @@ -204,3 +204,8 @@ Other Changes
* Fix crash on ``open()`` calls when the ``mode`` argument is not a simple string.

Partially closes #5321

* Introduced primer tests and a configuration tests framework. The helper classes available in
``pylint/testutil/`` are still unstable and might be modified in the near future.

Closes #4412 #5287
1 change: 1 addition & 0 deletions pylint/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from pylint.__pkginfo__ import __version__

PY37_PLUS = sys.version_info[:2] >= (3, 7)
PY38_PLUS = sys.version_info[:2] >= (3, 8)
PY39_PLUS = sys.version_info[:2] >= (3, 9)

Expand Down
100 changes: 100 additions & 0 deletions pylint/testutils/primer.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import logging
from pathlib import Path
from typing import Dict, List, Optional, Union

import git

PRIMER_DIRECTORY_PATH = Path(".pylint_primer_tests")


class PackageToLint:
"""Represents data about a package to be tested during primer tests"""

url: str
"""URL of the repository to clone"""

branch: str
"""Branch of the repository to clone"""

directories: List[str]
"""Directories within the repository to run pylint over"""

commit: Optional[str] = None
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
"""Commit hash to pin the repository on"""

pylint_additional_args: List[str] = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default values are only supported in 3.6.1, but I don't think that matters here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, #5171 makes it ok :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pylint_additional_args: List[str] = []
pylint_additional_args: List[str]

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Pierre-Sassoulas I think you missed this comment (the previous conversation was resolved).

Copy link
Member Author

Choose a reason for hiding this comment

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

Strange, sorry !

"""Arguments to give to pylint"""

pylintrc_relpath: Optional[str] = None
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
"""Path relative to project's main directory to the pylintrc if it exists"""

def __init__(
self,
url: str,
branch: str,
directories: List[str],
commit: Optional[str] = None,
pylint_additional_args: Optional[List[str]] = None,
pylintrc_relpath: Optional[str] = None,
) -> None:
self.url = url
self.branch = branch
self.directories = directories
self.commit = commit
self.pylint_additional_args = pylint_additional_args or []
self.pylintrc_relpath = pylintrc_relpath

@property
def pylintrc(self) -> Optional[Path]:
if self.pylintrc_relpath is None:
return None
return self.clone_directory / self.pylintrc_relpath

@property
def clone_directory(self) -> Path:
"""Directory to clone repository into"""
clone_name = "/".join(self.url.split("/")[-2:]).replace(".git", "")
return PRIMER_DIRECTORY_PATH / clone_name

@property
def paths_to_lint(self) -> List[str]:
"""The paths we need to lint"""
return [str(self.clone_directory / path) for path in self.directories]

@property
def pylint_args(self) -> List[str]:
options: List[str] = []
if self.pylintrc is not None:
# There is an error if rcfile is given but does not exists
options += [f"--rcfile={self.pylintrc}"]
return self.paths_to_lint + options + self.pylint_additional_args

def lazy_clone(self) -> None:
"""Concatenates the target directory and clones the file"""
logging.info("Lazy cloning %s", self.url)
if not self.clone_directory.exists():
options: Dict[str, Union[str, int]] = {
"url": self.url,
"to_path": str(self.clone_directory),
"branch": self.branch,
"depth": 1,
}
logging.info("Directory does not exists, cloning: %s", options)
git.Repo.clone_from(**options)
return

remote_sha1_commit = (
git.cmd.Git().ls_remote(self.url, self.branch).split("\t")[0]
)
local_sha1_commit = git.Repo(self.clone_directory).head.object.hexsha
if remote_sha1_commit != local_sha1_commit:
logging.info(
"Remote sha is %s while local sha is %s : pulling",
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
remote_sha1_commit,
local_sha1_commit,
)
repo = git.Repo(self.clone_directory)
origin = repo.remotes.origin
origin.pull()
else:
logging.info("Already up to date.")
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions requirements_test_min.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
astroid==2.9.0 # Pinned to a specific version for tests
pytest~=6.2
pytest-benchmark~=3.4
gitpython>3
7 changes: 5 additions & 2 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,9 @@ test = pytest
[tool:pytest]
testpaths = tests
python_files = *test_*.py
addopts = -m "not primer_stdlib"
addopts = --strict-markers -m "not primer"
markers =
primer_stdlib: Checks for crashes and errors when running pylint on stdlib
primer: Checks for crashes and errors when running pylint on stdlib or on external libs
benchmark: Baseline of pylint performance, if this regress something serious happened

[isort]
Expand Down Expand Up @@ -118,3 +118,6 @@ ignore_missing_imports = True

[mypy-toml.decoder]
ignore_missing_imports = True

[mypy-git.*]
ignore_missing_imports = True
59 changes: 59 additions & 0 deletions tests/primer/packages_to_lint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
{
"flask": {
Comment on lines +1 to +2
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered adding astroid here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense. Especially since astroid is very pylint ready so we could check for false positive right away.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes more sense in #5364

"url": "https://github.com/pallets/flask.git",
"branch": "main",
"directories": ["src/flask"]
},
"pytest": {
"url": "https://github.com/pytest-dev/pytest.git",
"branch": "main",
"directories": ["src/_pytest"]
},
"psycopg": {
"url": "https://github.com/psycopg/psycopg.git",
"branch": "master",
"directories": ["psycopg/psycopg"]
},
"keras": {
"url": "https://github.com/keras-team/keras.git",
"branch": "master",
"directories": ["keras"]
},
"sentry": {
"url": "https://github.com/getsentry/sentry.git",
"branch": "master",
"directories": ["src/sentry"]
},
"django": {
"url": "https://github.com/django/django.git",
"branch": "main",
"directories": ["django"]
},
"pandas": {
"url": "https://github.com/pandas-dev/pandas.git",
"branch": "master",
"directories": ["pandas"],
"pylint_additional_args": ["--ignore-patterns=\"test_"]
},
"black": {
"url": "https://github.com/psf/black.git",
"branch": "main",
"directories": ["src/black/", "src/blackd/", "src/black_primer/", "src/blib2to3/"]
},
"home-assistant": {
"url": "https://github.com/home-assistant/core.git",
"branch": "dev",
"directories": ["homeassistant"]
},
"graph-explorer": {
"url": "https://github.com/vimeo/graph-explorer.git",
"branch": "master",
"directories": ["graph_explorer"],
"pylintrc_relpath": ".pylintrc"
},
"pygame": {
"url": "https://github.com/pygame/pygame.git",
"branch": "main",
"directories": ["src_py"]
}
}
66 changes: 66 additions & 0 deletions tests/primer/test_primer_external.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE
import json
import logging
import subprocess
from pathlib import Path
from typing import Dict, Union

import pytest
from pytest import LogCaptureFixture

from pylint.testutils.primer import PackageToLint

PRIMER_DIRECTORY = Path(".pylint_primer_tests/").resolve()


def get_packages_to_lint_from_json(
json_path: Union[Path, str]
) -> Dict[str, PackageToLint]:
result: Dict[str, PackageToLint] = {}
with open(json_path, encoding="utf8") as f:
for name, package_data in json.load(f).items():
result[name] = PackageToLint(**package_data)
return result


PACKAGE_TO_LINT_JSON = Path(__file__).parent / "packages_to_lint.json"
PACKAGES_TO_LINT = get_packages_to_lint_from_json(PACKAGE_TO_LINT_JSON)
"""Dictionary of external packages used during the primer test"""


class TestPrimer:
@staticmethod
@pytest.mark.primer
@pytest.mark.parametrize("package", PACKAGES_TO_LINT.values(), ids=PACKAGES_TO_LINT)
def test_primer_external_packages_no_crash(
package: PackageToLint,
caplog: LogCaptureFixture,
) -> None:
__tracebackhide__ = True # pylint: disable=unused-variable
TestPrimer._primer_test(package, caplog)

@staticmethod
def _primer_test(package: PackageToLint, caplog: LogCaptureFixture) -> None:
"""Runs pylint over external packages to check for crashes and fatal messages

We only check for crashes (bit-encoded exit code 32) and fatal messages
(bit-encoded exit code 1). We assume that these external repositories do not
have any fatal errors in their code so that any fatal errors are pylint false
positives
"""
caplog.set_level(logging.INFO)
package.lazy_clone()

try:
# We want to test all the code we can
enables = ["--enable-all-extensions", "--enable=all"]
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if I would use --enable-all-extensions. For catching errors it might work, but I expect this will litter the log with other warnings. Usually when projects choose to disable a message, they don't want to be bothered by it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're going to have to separate "finding fatal and crashes" and finding new false positives.

Copy link
Member Author

Choose a reason for hiding this comment

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

# Duplicate code takes too long and is relatively safe
disables = ["--disable=duplicate-code"]
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
command = ["pylint"] + enables + disables + package.pylint_args
logging.info("Launching primer:\n%s", " ".join(command))
subprocess.run(command, check=True)
except subprocess.CalledProcessError as ex:
msg = f"Encountered {{}} during primer test for {package}"
assert ex.returncode != 32, msg.format("a crash")
assert ex.returncode % 2 == 0, msg.format("a message of category 'fatal'")
5 changes: 3 additions & 2 deletions tests/primer/test_primer_stdlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,15 @@ def _patch_stdout(out):
MODULES_NAMES = [m[1] for m in MODULES_TO_CHECK]


@pytest.mark.primer_stdlib
@pytest.mark.primer
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
@pytest.mark.parametrize(
("test_module_location", "test_module_name"), MODULES_TO_CHECK, ids=MODULES_NAMES
)
def test_lib_module_no_crash(
def test_primer_stdlib_no_crash(
test_module_location: str, test_module_name: str, capsys: CaptureFixture
) -> None:
"""Test that pylint does not produces any crashes or fatal errors on stdlib modules"""
__tracebackhide__ = True # pylint: disable=unused-variable
os.chdir(test_module_location)
with _patch_stdout(io.StringIO()):
try:
Expand Down