From 345504da258c8ff3f298f51ebb860ca32b347706 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Tue, 17 Mar 2026 20:20:07 +0100 Subject: [PATCH 1/2] Extend duplicate-code disable to cover tempdir helper Move the ``# pylint: enable=duplicate-code`` comment after the ``tempdir`` context manager so it is covered by the existing suppression scope. --- tests/config/test_find_default_config_files.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/config/test_find_default_config_files.py b/tests/config/test_find_default_config_files.py index 2c50a55682..52f8c53595 100644 --- a/tests/config/test_find_default_config_files.py +++ b/tests/config/test_find_default_config_files.py @@ -60,9 +60,6 @@ def fake_home() -> Iterator[None]: shutil.rmtree(folder, ignore_errors=True) -# pylint: enable=duplicate-code - - @contextlib.contextmanager def tempdir() -> Iterator[str]: """Create a temp directory and change the current location to it. @@ -83,6 +80,9 @@ def tempdir() -> Iterator[str]: shutil.rmtree(abs_tmp) +# pylint: enable=duplicate-code + + @pytest.mark.usefixtures("pop_pylintrc") def test_pylintrc() -> None: """Test that the environment variable is checked for existence.""" From fe2d79de02838ff958a1ea4574af54c36df761ff Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Fri, 4 Nov 2022 23:55:50 +0100 Subject: [PATCH 2/2] Extract Comparator class from CompareCommand Move JSON loading, batching, and cross-referencing logic into a dedicated Comparator class. CompareCommand.run() now delegates to the Comparator and iterates over it to build the comment. --- pylint/testutils/_primer/comparator.py | 69 +++++++++++++++++++ .../_primer/primer_compare_command.py | 67 +++--------------- .../batched => batched_cases}/expected.txt | 0 .../batched_cases/expected_comparator.json | 1 + .../main_batch0.json | 0 .../main_batch1.json | 0 .../batched => batched_cases}/pr_batch0.json | 0 .../batched => batched_cases}/pr_batch1.json | 0 .../both_empty/expected.txt | 0 .../cases/both_empty/expected_comparator.json | 1 + .../{fixtures => cases}/both_empty/main.json | 0 .../{fixtures => cases}/both_empty/pr.json | 0 .../message_changed/expected.txt | 0 .../message_changed/expected_comparator.json | 1 + .../message_changed/expected_truncated.txt | 0 .../message_changed/main.json | 0 .../message_changed/pr.json | 0 .../new_message/expected.txt | 0 .../new_message/expected_comparator.json | 1 + .../{fixtures => cases}/new_message/main.json | 0 .../{fixtures => cases}/new_message/pr.json | 0 .../no_change/expected.txt | 0 .../cases/no_change/expected_comparator.json | 1 + .../{fixtures => cases}/no_change/main.json | 0 .../{fixtures => cases}/no_change/pr.json | 0 .../removed_message/expected.txt | 0 .../removed_message/expected_comparator.json | 1 + .../removed_message/main.json | 0 .../removed_message/pr.json | 0 tests/testutils/_primer/test_comparator.py | 44 ++++++++++++ tests/testutils/_primer/test_primer.py | 14 ++-- 31 files changed, 134 insertions(+), 66 deletions(-) create mode 100644 pylint/testutils/_primer/comparator.py rename tests/testutils/_primer/{fixtures/batched => batched_cases}/expected.txt (100%) create mode 100644 tests/testutils/_primer/batched_cases/expected_comparator.json rename tests/testutils/_primer/{fixtures/batched => batched_cases}/main_batch0.json (100%) rename tests/testutils/_primer/{fixtures/batched => batched_cases}/main_batch1.json (100%) rename tests/testutils/_primer/{fixtures/batched => batched_cases}/pr_batch0.json (100%) rename tests/testutils/_primer/{fixtures/batched => batched_cases}/pr_batch1.json (100%) rename tests/testutils/_primer/{fixtures => cases}/both_empty/expected.txt (100%) create mode 100644 tests/testutils/_primer/cases/both_empty/expected_comparator.json rename tests/testutils/_primer/{fixtures => cases}/both_empty/main.json (100%) rename tests/testutils/_primer/{fixtures => cases}/both_empty/pr.json (100%) rename tests/testutils/_primer/{fixtures => cases}/message_changed/expected.txt (100%) create mode 100644 tests/testutils/_primer/cases/message_changed/expected_comparator.json rename tests/testutils/_primer/{fixtures => cases}/message_changed/expected_truncated.txt (100%) rename tests/testutils/_primer/{fixtures => cases}/message_changed/main.json (100%) rename tests/testutils/_primer/{fixtures => cases}/message_changed/pr.json (100%) rename tests/testutils/_primer/{fixtures => cases}/new_message/expected.txt (100%) create mode 100644 tests/testutils/_primer/cases/new_message/expected_comparator.json rename tests/testutils/_primer/{fixtures => cases}/new_message/main.json (100%) rename tests/testutils/_primer/{fixtures => cases}/new_message/pr.json (100%) rename tests/testutils/_primer/{fixtures => cases}/no_change/expected.txt (100%) create mode 100644 tests/testutils/_primer/cases/no_change/expected_comparator.json rename tests/testutils/_primer/{fixtures => cases}/no_change/main.json (100%) rename tests/testutils/_primer/{fixtures => cases}/no_change/pr.json (100%) rename tests/testutils/_primer/{fixtures => cases}/removed_message/expected.txt (100%) create mode 100644 tests/testutils/_primer/cases/removed_message/expected_comparator.json rename tests/testutils/_primer/{fixtures => cases}/removed_message/main.json (100%) rename tests/testutils/_primer/{fixtures => cases}/removed_message/pr.json (100%) create mode 100644 tests/testutils/_primer/test_comparator.py diff --git a/pylint/testutils/_primer/comparator.py b/pylint/testutils/_primer/comparator.py new file mode 100644 index 0000000000..1a4d66fa6b --- /dev/null +++ b/pylint/testutils/_primer/comparator.py @@ -0,0 +1,69 @@ +# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html +# For details: https://github.com/pylint-dev/pylint/blob/main/LICENSE +# Copyright (c) https://github.com/pylint-dev/pylint/blob/main/CONTRIBUTORS.txt + +from __future__ import annotations + +import json +from collections.abc import Generator +from pathlib import Path + +from pylint.reporters.json_reporter import OldJsonExport +from pylint.testutils._primer.primer_command import ( + PackageData, + PackageMessages, +) + + +class Comparator: + def __init__( + self, base_file: str, new_file: str, batches: int | None = None + ) -> None: + self._base_file = base_file + self._new_file = new_file + self._batches = batches + + def __iter__(self) -> Generator[tuple[str, PackageData, PackageData]]: + main_data: PackageMessages + if self._batches is None: + main_data = self._load_json(self._base_file) + pr_data = self._load_json(self._new_file) + else: + main_data = {} + pr_data = {} + for idx in range(self._batches): + main_data.update( + self._load_json( + self._base_file.replace("BATCHIDX", "batch" + str(idx)) + ) + ) + pr_data.update( + self._load_json( + self._new_file.replace("BATCHIDX", "batch" + str(idx)) + ) + ) + + missing_messages: PackageMessages = {} + for package, data in main_data.items(): + package_missing_messages: list[OldJsonExport] = [] + for message in data["messages"]: + try: + pr_data[package]["messages"].remove(message) + except ValueError: + package_missing_messages.append(message) + missing_messages[package] = PackageData( + commit=pr_data[package]["commit"], + messages=package_missing_messages, + ) + + for package, pkg_missing in missing_messages.items(): + new_messages = pr_data[package] + if not pkg_missing["messages"] and not new_messages["messages"]: + continue + yield package, pkg_missing, new_messages + + @staticmethod + def _load_json(file_path: Path | str) -> PackageMessages: + with open(file_path, encoding="utf-8") as f: + result: PackageMessages = json.load(f) + return result diff --git a/pylint/testutils/_primer/primer_compare_command.py b/pylint/testutils/_primer/primer_compare_command.py index 7b245b9e19..784574a7ee 100644 --- a/pylint/testutils/_primer/primer_compare_command.py +++ b/pylint/testutils/_primer/primer_compare_command.py @@ -3,79 +3,28 @@ # Copyright (c) https://github.com/pylint-dev/pylint/blob/main/CONTRIBUTORS.txt from __future__ import annotations -import json -from pathlib import Path, PurePosixPath +from pathlib import PurePosixPath -from pylint.reporters.json_reporter import OldJsonExport -from pylint.testutils._primer.primer_command import ( - PackageData, - PackageMessages, - PrimerCommand, -) +from pylint.testutils._primer.comparator import Comparator +from pylint.testutils._primer.primer_command import PackageData, PrimerCommand MAX_GITHUB_COMMENT_LENGTH = 65536 class CompareCommand(PrimerCommand): def run(self) -> None: - if self.config.batches is None: - main_data = self._load_json(self.config.base_file) - pr_data = self._load_json(self.config.new_file) - else: - main_data = {} - pr_data = {} - for idx in range(self.config.batches): - main_data.update( - self._load_json( - self.config.base_file.replace("BATCHIDX", "batch" + str(idx)) - ) - ) - pr_data.update( - self._load_json( - self.config.new_file.replace("BATCHIDX", "batch" + str(idx)) - ) - ) - - missing_messages_data, new_messages_data = self._cross_reference( - main_data, pr_data + comparator = Comparator( + self.config.base_file, self.config.new_file, self.config.batches ) - comment = self._create_comment(missing_messages_data, new_messages_data) + comment = self._create_comment(comparator) with open(self.primer_directory / "comment.txt", "w", encoding="utf-8") as f: f.write(comment) - @staticmethod - def _cross_reference( - main_data: PackageMessages, pr_data: PackageMessages - ) -> tuple[PackageMessages, PackageMessages]: - missing_messages_data: PackageMessages = {} - for package, data in main_data.items(): - package_missing_messages: list[OldJsonExport] = [] - for message in data["messages"]: - try: - pr_data[package]["messages"].remove(message) - except ValueError: - package_missing_messages.append(message) - missing_messages_data[package] = PackageData( - commit=pr_data[package]["commit"], messages=package_missing_messages - ) - return missing_messages_data, pr_data - - @staticmethod - def _load_json(file_path: Path | str) -> PackageMessages: - with open(file_path, encoding="utf-8") as f: - result: PackageMessages = json.load(f) - return result - - def _create_comment( - self, all_missing_messages: PackageMessages, all_new_messages: PackageMessages - ) -> str: + def _create_comment(self, comparator: Comparator) -> str: comment = "" - for package, missing_messages in all_missing_messages.items(): + for package, missing_messages, new_messages in comparator: if len(comment) >= MAX_GITHUB_COMMENT_LENGTH: break - new_messages = all_new_messages[package] - if not missing_messages["messages"] and not new_messages["messages"]: - continue comment += self._create_comment_for_package( package, new_messages, missing_messages ) diff --git a/tests/testutils/_primer/fixtures/batched/expected.txt b/tests/testutils/_primer/batched_cases/expected.txt similarity index 100% rename from tests/testutils/_primer/fixtures/batched/expected.txt rename to tests/testutils/_primer/batched_cases/expected.txt diff --git a/tests/testutils/_primer/batched_cases/expected_comparator.json b/tests/testutils/_primer/batched_cases/expected_comparator.json new file mode 100644 index 0000000000..e3a42bfd31 --- /dev/null +++ b/tests/testutils/_primer/batched_cases/expected_comparator.json @@ -0,0 +1 @@ +[{ "package": "astroid", "missing": 2, "new": 0 }] diff --git a/tests/testutils/_primer/fixtures/batched/main_batch0.json b/tests/testutils/_primer/batched_cases/main_batch0.json similarity index 100% rename from tests/testutils/_primer/fixtures/batched/main_batch0.json rename to tests/testutils/_primer/batched_cases/main_batch0.json diff --git a/tests/testutils/_primer/fixtures/batched/main_batch1.json b/tests/testutils/_primer/batched_cases/main_batch1.json similarity index 100% rename from tests/testutils/_primer/fixtures/batched/main_batch1.json rename to tests/testutils/_primer/batched_cases/main_batch1.json diff --git a/tests/testutils/_primer/fixtures/batched/pr_batch0.json b/tests/testutils/_primer/batched_cases/pr_batch0.json similarity index 100% rename from tests/testutils/_primer/fixtures/batched/pr_batch0.json rename to tests/testutils/_primer/batched_cases/pr_batch0.json diff --git a/tests/testutils/_primer/fixtures/batched/pr_batch1.json b/tests/testutils/_primer/batched_cases/pr_batch1.json similarity index 100% rename from tests/testutils/_primer/fixtures/batched/pr_batch1.json rename to tests/testutils/_primer/batched_cases/pr_batch1.json diff --git a/tests/testutils/_primer/fixtures/both_empty/expected.txt b/tests/testutils/_primer/cases/both_empty/expected.txt similarity index 100% rename from tests/testutils/_primer/fixtures/both_empty/expected.txt rename to tests/testutils/_primer/cases/both_empty/expected.txt diff --git a/tests/testutils/_primer/cases/both_empty/expected_comparator.json b/tests/testutils/_primer/cases/both_empty/expected_comparator.json new file mode 100644 index 0000000000..fe51488c70 --- /dev/null +++ b/tests/testutils/_primer/cases/both_empty/expected_comparator.json @@ -0,0 +1 @@ +[] diff --git a/tests/testutils/_primer/fixtures/both_empty/main.json b/tests/testutils/_primer/cases/both_empty/main.json similarity index 100% rename from tests/testutils/_primer/fixtures/both_empty/main.json rename to tests/testutils/_primer/cases/both_empty/main.json diff --git a/tests/testutils/_primer/fixtures/both_empty/pr.json b/tests/testutils/_primer/cases/both_empty/pr.json similarity index 100% rename from tests/testutils/_primer/fixtures/both_empty/pr.json rename to tests/testutils/_primer/cases/both_empty/pr.json diff --git a/tests/testutils/_primer/fixtures/message_changed/expected.txt b/tests/testutils/_primer/cases/message_changed/expected.txt similarity index 100% rename from tests/testutils/_primer/fixtures/message_changed/expected.txt rename to tests/testutils/_primer/cases/message_changed/expected.txt diff --git a/tests/testutils/_primer/cases/message_changed/expected_comparator.json b/tests/testutils/_primer/cases/message_changed/expected_comparator.json new file mode 100644 index 0000000000..d826f8390a --- /dev/null +++ b/tests/testutils/_primer/cases/message_changed/expected_comparator.json @@ -0,0 +1 @@ +[{ "package": "astroid", "missing": 1, "new": 1 }] diff --git a/tests/testutils/_primer/fixtures/message_changed/expected_truncated.txt b/tests/testutils/_primer/cases/message_changed/expected_truncated.txt similarity index 100% rename from tests/testutils/_primer/fixtures/message_changed/expected_truncated.txt rename to tests/testutils/_primer/cases/message_changed/expected_truncated.txt diff --git a/tests/testutils/_primer/fixtures/message_changed/main.json b/tests/testutils/_primer/cases/message_changed/main.json similarity index 100% rename from tests/testutils/_primer/fixtures/message_changed/main.json rename to tests/testutils/_primer/cases/message_changed/main.json diff --git a/tests/testutils/_primer/fixtures/message_changed/pr.json b/tests/testutils/_primer/cases/message_changed/pr.json similarity index 100% rename from tests/testutils/_primer/fixtures/message_changed/pr.json rename to tests/testutils/_primer/cases/message_changed/pr.json diff --git a/tests/testutils/_primer/fixtures/new_message/expected.txt b/tests/testutils/_primer/cases/new_message/expected.txt similarity index 100% rename from tests/testutils/_primer/fixtures/new_message/expected.txt rename to tests/testutils/_primer/cases/new_message/expected.txt diff --git a/tests/testutils/_primer/cases/new_message/expected_comparator.json b/tests/testutils/_primer/cases/new_message/expected_comparator.json new file mode 100644 index 0000000000..e436268fa3 --- /dev/null +++ b/tests/testutils/_primer/cases/new_message/expected_comparator.json @@ -0,0 +1 @@ +[{ "package": "astroid", "missing": 0, "new": 1 }] diff --git a/tests/testutils/_primer/fixtures/new_message/main.json b/tests/testutils/_primer/cases/new_message/main.json similarity index 100% rename from tests/testutils/_primer/fixtures/new_message/main.json rename to tests/testutils/_primer/cases/new_message/main.json diff --git a/tests/testutils/_primer/fixtures/new_message/pr.json b/tests/testutils/_primer/cases/new_message/pr.json similarity index 100% rename from tests/testutils/_primer/fixtures/new_message/pr.json rename to tests/testutils/_primer/cases/new_message/pr.json diff --git a/tests/testutils/_primer/fixtures/no_change/expected.txt b/tests/testutils/_primer/cases/no_change/expected.txt similarity index 100% rename from tests/testutils/_primer/fixtures/no_change/expected.txt rename to tests/testutils/_primer/cases/no_change/expected.txt diff --git a/tests/testutils/_primer/cases/no_change/expected_comparator.json b/tests/testutils/_primer/cases/no_change/expected_comparator.json new file mode 100644 index 0000000000..fe51488c70 --- /dev/null +++ b/tests/testutils/_primer/cases/no_change/expected_comparator.json @@ -0,0 +1 @@ +[] diff --git a/tests/testutils/_primer/fixtures/no_change/main.json b/tests/testutils/_primer/cases/no_change/main.json similarity index 100% rename from tests/testutils/_primer/fixtures/no_change/main.json rename to tests/testutils/_primer/cases/no_change/main.json diff --git a/tests/testutils/_primer/fixtures/no_change/pr.json b/tests/testutils/_primer/cases/no_change/pr.json similarity index 100% rename from tests/testutils/_primer/fixtures/no_change/pr.json rename to tests/testutils/_primer/cases/no_change/pr.json diff --git a/tests/testutils/_primer/fixtures/removed_message/expected.txt b/tests/testutils/_primer/cases/removed_message/expected.txt similarity index 100% rename from tests/testutils/_primer/fixtures/removed_message/expected.txt rename to tests/testutils/_primer/cases/removed_message/expected.txt diff --git a/tests/testutils/_primer/cases/removed_message/expected_comparator.json b/tests/testutils/_primer/cases/removed_message/expected_comparator.json new file mode 100644 index 0000000000..cd879eeea8 --- /dev/null +++ b/tests/testutils/_primer/cases/removed_message/expected_comparator.json @@ -0,0 +1 @@ +[{ "package": "astroid", "missing": 1, "new": 0 }] diff --git a/tests/testutils/_primer/fixtures/removed_message/main.json b/tests/testutils/_primer/cases/removed_message/main.json similarity index 100% rename from tests/testutils/_primer/fixtures/removed_message/main.json rename to tests/testutils/_primer/cases/removed_message/main.json diff --git a/tests/testutils/_primer/fixtures/removed_message/pr.json b/tests/testutils/_primer/cases/removed_message/pr.json similarity index 100% rename from tests/testutils/_primer/fixtures/removed_message/pr.json rename to tests/testutils/_primer/cases/removed_message/pr.json diff --git a/tests/testutils/_primer/test_comparator.py b/tests/testutils/_primer/test_comparator.py new file mode 100644 index 0000000000..e91852ed25 --- /dev/null +++ b/tests/testutils/_primer/test_comparator.py @@ -0,0 +1,44 @@ +# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html +# For details: https://github.com/pylint-dev/pylint/blob/main/LICENSE +# Copyright (c) https://github.com/pylint-dev/pylint/blob/main/CONTRIBUTORS.txt + +import json +from pathlib import Path + +import pytest + +from pylint.testutils._primer.comparator import Comparator + +CASES_PATH = Path(__file__).parent / "cases" + + +@pytest.mark.parametrize( + "directory", + [pytest.param(p, id=p.name) for p in CASES_PATH.iterdir() if p.is_dir()], +) +def test_comparator(directory: Path) -> None: + """Test Comparator with each fixture directory.""" + comparator = Comparator(str(directory / "main.json"), str(directory / "pr.json")) + expected = json.loads((directory / "expected_comparator.json").read_text("utf-8")) + results = list(comparator) + assert len(results) == len(expected) + for (package, missing, new), exp in zip(results, expected): + assert package == exp["package"] + assert len(missing["messages"]) == exp["missing"] + assert len(new["messages"]) == exp["new"] + + +def test_comparator_batched() -> None: + fixture = Path(__file__).parent / "batched_cases" + comparator = Comparator( + str(fixture / "main_BATCHIDX.json"), + str(fixture / "pr_BATCHIDX.json"), + batches=2, + ) + expected = json.loads((fixture / "expected_comparator.json").read_text("utf-8")) + results = list(comparator) + assert len(results) == len(expected) + for (package, missing, new), exp in zip(results, expected): + assert package == exp["package"] + assert len(missing["messages"]) == exp["missing"] + assert len(new["messages"]) == exp["new"] diff --git a/tests/testutils/_primer/test_primer.py b/tests/testutils/_primer/test_primer.py index df555a7f81..6017d297b4 100644 --- a/tests/testutils/_primer/test_primer.py +++ b/tests/testutils/_primer/test_primer.py @@ -20,7 +20,7 @@ TEST_DIR_ROOT = HERE.parent.parent PRIMER_DIRECTORY = TEST_DIR_ROOT / ".pylint_primer_tests/" PACKAGES_TO_PRIME_PATH = TEST_DIR_ROOT / "primer/packages_to_prime.json" -FIXTURES_PATH = HERE / "fixtures" +CASES_PATH = HERE / "cases" # If you change this, also change DEFAULT_PYTHON in # ``.github/workflows/primer_comment.yaml`` @@ -52,20 +52,20 @@ class TestPrimer: @pytest.mark.parametrize( "directory", [ - pytest.param(p, id=str(p.relative_to(FIXTURES_PATH))) - for p in FIXTURES_PATH.iterdir() - if p.is_dir() and p.name != "batched" # tested separately + pytest.param(p, id=str(p.relative_to(CASES_PATH))) + for p in CASES_PATH.iterdir() + if p.is_dir() ], ) def test_compare(self, directory: Path) -> None: """Test for the standard case. - Directory in 'fixtures/' with 'main.json', 'pr.json' and 'expected.txt'. + Directory in 'cases/' with 'main.json', 'pr.json' and 'expected.txt'. """ self.__assert_expected(directory) def test_compare_batched(self) -> None: - fixture = FIXTURES_PATH / "batched" + fixture = HERE / "batched_cases" self.__assert_expected( fixture, fixture / "main_BATCHIDX.json", @@ -76,7 +76,7 @@ def test_compare_batched(self) -> None: def test_truncated_compare(self) -> None: """Test for the truncation of comments that are too long.""" max_comment_length = 525 - directory = FIXTURES_PATH / "message_changed" + directory = CASES_PATH / "message_changed" with patch( "pylint.testutils._primer.primer_compare_command.MAX_GITHUB_COMMENT_LENGTH", max_comment_length,