From 16f54d256ea184db34d724b4ce6705a6ee080718 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Wed, 20 Jul 2022 11:22:19 +0200 Subject: [PATCH] Deduplicate requested test cases for `package new` and `package update` We previously didn't deduplicate the set of test cases requested when creating or updating a package. This can lead to noisy updates when users specify a new test case which already exists in the package. This commit adds logic to remove duplicates in the getter for the list of test cases. We deduplicate the list in the getter rather than the setter, to avoid introducing invisible duplicates if users use `append()` on the returned list. --- commodore/cli.py | 11 +++++---- commodore/package/template.py | 20 +++++++++++++++- docs/modules/ROOT/pages/reference/cli.adoc | 2 ++ tests/test_package_template.py | 28 +++++++++++++++++++++- 4 files changed, 55 insertions(+), 6 deletions(-) diff --git a/commodore/cli.py b/commodore/cli.py index c9b7c56c..95de8323 100644 --- a/commodore/cli.py +++ b/commodore/cli.py @@ -483,7 +483,8 @@ def package(config: Config, verbose: int): show_default=True, multiple=True, help="Additional test cases to generate in the new package. Can be repeated. " - + "Test case `defaults` will always be generated.", + + "Test case `defaults` will always be generated." + + "Commodore will deduplicate test cases by name.", ) @verbosity @pass_config @@ -550,7 +551,8 @@ def package_new( show_default=True, multiple=True, help="Additional test cases to generate in the new package. Can be repeated. " - + "Already existing test cases will always be kept.", + + "Already existing test cases will always be kept. " + + "Commodore will deduplicate test cases by name.", ) @click.option( "--remove-test-case", @@ -583,8 +585,9 @@ def package_update( t.golden_tests = golden_tests if update_copyright_year: t.copyright_year = None - t.test_cases.extend(additional_test_case) - t.test_cases = [tc for tc in t.test_cases if tc not in remove_test_case] + test_cases = t.test_cases + test_cases.extend(additional_test_case) + t.test_cases = [tc for tc in test_cases if tc not in remove_test_case] t.update() diff --git a/commodore/package/template.py b/commodore/package/template.py index c28f2062..0c613643 100644 --- a/commodore/package/template.py +++ b/commodore/package/template.py @@ -23,7 +23,7 @@ class PackageTemplater(Templater): template_url: str template_version: str template_commit: str - test_cases: list[str] = ["defaults"] + _test_cases: list[str] = ["defaults"] copyright_year: Optional[str] = None _target_dir: Optional[Path] = None @@ -54,6 +54,24 @@ def from_existing(cls, config: Config, package_path: Path): t.copyright_year = cookiecutter_args["copyright_year"] return t + @property + def test_cases(self) -> list[str]: + """Return list of test cases. + + The getter deduplicates the stored list before returning it. + + Don't use `append()` on the returned list to add test cases to the package, as + the getter returns a copy of the list stored in the object.""" + cases = [] + for t in self._test_cases: + if t not in cases: + cases.append(t) + return cases + + @test_cases.setter + def test_cases(self, test_cases: list[str]): + self._test_cases = test_cases + def _cruft_renderer( self, template_location: str, diff --git a/docs/modules/ROOT/pages/reference/cli.adoc b/docs/modules/ROOT/pages/reference/cli.adoc index 4f1b5a2b..f223601f 100644 --- a/docs/modules/ROOT/pages/reference/cli.adoc +++ b/docs/modules/ROOT/pages/reference/cli.adoc @@ -234,6 +234,7 @@ This command doesn't have any command line options. Additional test cases to generate in the new package. Can be repeated. Test case `defaults` will always be generated. + Commodore will deduplicate the provided test cases. == Package Update @@ -248,6 +249,7 @@ This command doesn't have any command line options. *--additional-test-case, -t* CASE:: Additional test cases to add to the package. Can be repeated. + Commodore will deduplicate the provided test cases. *--remove-test-case* CASE:: Test cases to remove from the package. diff --git a/tests/test_package_template.py b/tests/test_package_template.py index d28bb49b..39182be9 100644 --- a/tests/test_package_template.py +++ b/tests/test_package_template.py @@ -57,6 +57,7 @@ def call_package_new( [ [], ["foo"], + ["foo", "foo"], ["foo", "bar"], ], ) @@ -103,10 +104,15 @@ def test_run_package_new_command( for f in expected_files: assert (pkg_dir / f).is_file() + expected_cases = ["defaults"] + for t in additional_test_cases: + if t not in expected_cases: + expected_cases.append(t) + with open(pkg_dir / ".github" / "workflows" / "test.yaml") as gh_test: workflows = yaml.safe_load(gh_test) instances = workflows["jobs"]["test"]["strategy"]["matrix"]["instance"] - assert instances == list(["defaults"] + additional_test_cases) + assert instances == expected_cases @pytest.mark.parametrize( @@ -293,3 +299,23 @@ def test_package_templater_from_existing_nonexistent(tmp_path: Path, config: Con _ = PackageTemplater.from_existing(config, tmp_path / "test-package") assert str(e.value) == "Provided package path isn't a directory" + + +@pytest.mark.parametrize( + "test_cases,expected", + [ + ([], []), + (["defaults"], ["defaults"]), + (["defaults", "foo"], ["defaults", "foo"]), + (["defaults", "foo", "foo"], ["defaults", "foo"]), + (["foo", "bar"], ["foo", "bar"]), + (["foo", "bar", "foo"], ["foo", "bar"]), + ], +) +def test_package_templater_test_cases( + tmp_path: Path, config: Config, test_cases: list[str], expected: list[str] +): + p = PackageTemplater(config, "test-package") + p.test_cases = test_cases + + assert p.test_cases == expected