Primer tests "à la mypy" (#5173)

* Add changelog and warning about unstable API in testutil

* Add primer tests, (running pylint on external libs during tests)

In order to anticipate crash/fatal messages, false positives are harder
to anticipate. Follow-up will be #5359 and later on #5364 

Add '__tracebackhide__ = True' so the traceback is manageable

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

.github/workflows/ci.yaml

@@ -509,3 +509,37 @@ jobs:
           . venv/bin/activate
           pip install -e .
           pytest -m primer_stdlib -n auto
+
+  pytest-primer-external:
+    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_external -n auto

.gitignore

@@ -20,3 +20,4 @@ build-stamp
 .pytest_cache/
 .mypy_cache/
 .benchmarks/
+.pylint_primer_tests/

ChangeLog

@@ -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
+
 * 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

doc/whatsnew/2.12.rst

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

pylint/constants.py

@@ -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)
 

pylint/testutils/primer.py

@@ -0,0 +1,107 @@
+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]
+    """Commit hash to pin the repository on"""
+
+    pylint_additional_args: List[str]
+    """Arguments to give to pylint"""
+
+    pylintrc_relpath: Optional[str]
+    """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:  # pragma: no cover
+        """Concatenates the target directory and clones the file
+
+        Not expected to be tested as the primer won't work if it doesn't.
+        It's tested in the continuous integration primers, only the coverage
+        is not calculated on everything. If lazy clone breaks for local use
+        we'll probably notice because we'll have a fatal when launching the
+        primer locally.
+        """
+        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 new commits",
+                remote_sha1_commit,
+                local_sha1_commit,
+            )
+            repo = git.Repo(self.clone_directory)
+            origin = repo.remotes.origin
+            origin.pull()
+        else:
+            logging.info("Repository already up to date.")

requirements_test_min.txt

@@ -3,3 +3,4 @@
 astroid==2.9.0  # Pinned to a specific version for tests
 pytest~=6.2
 pytest-benchmark~=3.4
+gitpython>3

setup.cfg

@@ -69,9 +69,10 @@ test = pytest
 [tool:pytest]
 testpaths = tests
 python_files = *test_*.py
-addopts = -m "not primer_stdlib"
+addopts = --strict-markers -m "not primer_stdlib and not primer_external"
 markers =
     primer_stdlib: Checks for crashes and errors when running pylint on stdlib
+    primer_external: Checks for crashes and errors when running pylint on external libs
     benchmark: Baseline of pylint performance, if this regress something serious happened
 
 [isort]
@@ -118,3 +119,6 @@ ignore_missing_imports = True
 
 [mypy-toml.decoder]
 ignore_missing_imports = True
+
+[mypy-git.*]
+ignore_missing_imports = True

tests/primer/packages_to_lint.json

@@ -0,0 +1,59 @@
+{
+  "flask": {
+    "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"]
+  }
+}

tests/primer/test_primer_external.py

@@ -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_external
+    @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"]
+            # Duplicate code takes too long and is relatively safe
+            disables = ["--disable=duplicate-code"]
+            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'")

tests/primer/test_primer_stdlib.py

@@ -43,10 +43,11 @@ def _patch_stdout(out):
 @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: