From f4c0b731369f14325274cea9193133bd19aa69e1 Mon Sep 17 00:00:00 2001 From: Emmanuel Ogbizi Date: Sun, 9 Feb 2020 19:25:09 -0500 Subject: [PATCH] Fix cleanup unused snapshots when targeting files (#127) * test: clean up unused snapshots when targeting file * test: targetting by node ids * test: targeting by marker expresions * test: refactor use testdir fixture * fix: support node id test selection * test: add test for snapshot location warning * refactor: filter before checking if ran all items * chore: update changelog * test: compress collection testcases * fix: support --pyargs syntax * cr: wrap message in gettext for i88n * cr: remove warning message whitespace * cr: warning message formatting --- CHANGELOG.md | 3 + src/syrupy/__init__.py | 10 +- src/syrupy/data.py | 4 +- src/syrupy/extensions/base.py | 19 ++- src/syrupy/extensions/single_file.py | 4 +- src/syrupy/location.py | 10 +- src/syrupy/report.py | 23 ++-- .../test_custom_snapshot_directory.py | 2 +- tests/test_integration_custom.py | 5 + tests/test_integration_default.py | 109 ++++++++++++------ 10 files changed, 122 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10f3ab32..34f218ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ From v1.0.0 onwards, this project adheres to [Semantic Versioning](https://semve ## Master (Unreleased) - Fix bug where untargeted snapshots would be deleted when using pytest in targeted mode (#123) +- Fix bug where snapshot files were not cleaned up when running specific test files (#127) +- Fix bug where targeting specific test nodes in a test file was not supported (#127) +- Fix bug where targeting specific test modules using pyargs was not supported (#127) ## [v0.3.1](https://github.com/tophat/syrupy/compare/v0.3.0...v0.3.1) diff --git a/src/syrupy/__init__.py b/src/syrupy/__init__.py index 500b87d4..9d364143 100644 --- a/src/syrupy/__init__.py +++ b/src/syrupy/__init__.py @@ -54,6 +54,14 @@ def pytest_assertrepr_compare(op: str, left: Any, right: Any) -> Optional[List[s return None +def __is_testpath(arg: str) -> bool: + return not arg.startswith("-") and bool(glob.glob(arg.split("::")[0])) + + +def __is_testmodule(arg: str) -> bool: + return arg == "--pyargs" + + def pytest_sessionstart(session: Any) -> None: """ Initialize snapshot session before tests are collected and ran. @@ -65,7 +73,7 @@ def pytest_sessionstart(session: Any) -> None: update_snapshots=config.option.update_snapshots, base_dir=config.rootdir, is_providing_paths=any( - not arg.startswith("-") and glob.glob(arg) + __is_testpath(arg) or __is_testmodule(arg) for arg in config.invocation_params.args ), ) diff --git a/src/syrupy/data.py b/src/syrupy/data.py index d8c79dd2..f7a1784a 100644 --- a/src/syrupy/data.py +++ b/src/syrupy/data.py @@ -50,6 +50,8 @@ def get(self, snapshot_name: str) -> Optional["Snapshot"]: def add(self, snapshot: "Snapshot") -> None: self._snapshots[snapshot.name] = snapshot + if snapshot.name != SNAPSHOT_EMPTY_FOSSIL_KEY: + self.remove(SNAPSHOT_EMPTY_FOSSIL_KEY) def merge(self, snapshot_fossil: "SnapshotFossil") -> None: for snapshot in snapshot_fossil: @@ -82,7 +84,7 @@ def has_snapshots(self) -> bool: @attr.s(frozen=True) class SnapshotUnknownFossil(SnapshotFossil): - """This is a saved fossil that is unclaimed by any extension currenly in use""" + """This is a saved fossil that is unclaimed by any extension currently in use""" _snapshots: Dict[str, "Snapshot"] = attr.ib(default=SNAPSHOTS_UNKNOWN, init=False) diff --git a/src/syrupy/extensions/base.py b/src/syrupy/extensions/base.py index 17a2e7de..bfe5c8a8 100644 --- a/src/syrupy/extensions/base.py +++ b/src/syrupy/extensions/base.py @@ -5,6 +5,7 @@ abstractmethod, ) from difflib import ndiff +from gettext import gettext from itertools import zip_longest from typing import ( TYPE_CHECKING, @@ -123,12 +124,18 @@ def write_snapshot(self, *, data: "SerializedData", index: int) -> None: """ self._pre_write(data=data, index=index) snapshot_location = self.get_location(index=index) + if not self.test_location.matches_snapshot_location(snapshot_location): + warning_msg = gettext( + "\nCan not relate snapshot location '{}' to the test location." + "\nConsider adding '{}' to the generated location." + ).format(snapshot_location, self.test_location.filename) + warnings.warn(warning_msg) snapshot_name = self.get_snapshot_name(index=index) if not self.test_location.matches_snapshot_name(snapshot_name): - warning_msg = f""" - Can not relate snapshot name '{snapshot_name}' to the test location. - Consider adding '{self.test_location.testname}' to the generated name. - """ + warning_msg = gettext( + "\nCan not relate snapshot name '{}' to the test location." + "\nConsider adding '{}' to the generated name." + ).format(snapshot_name, self.test_location.testname) warnings.warn(warning_msg) snapshot_fossil = SnapshotFossil(location=snapshot_location) snapshot_fossil.add(Snapshot(name=snapshot_name, data=data)) @@ -182,7 +189,7 @@ def _write_snapshot_fossil(self, *, snapshot_fossil: "SnapshotFossil") -> None: @property def _dirname(self) -> str: - test_dirname = os.path.dirname(self.test_location.filename) + test_dirname = os.path.dirname(self.test_location.filepath) return os.path.join(test_dirname, SNAPSHOT_DIRNAME) @property @@ -192,7 +199,7 @@ def _file_extension(self) -> str: def _get_file_basename(self, *, index: int) -> str: """Returns file basename without extension. Used to create full filepath.""" - return f"{os.path.splitext(os.path.basename(self.test_location.filename))[0]}" + return self.test_location.filename def __ensure_snapshot_dir(self, *, index: int) -> None: """ diff --git a/src/syrupy/extensions/single_file.py b/src/syrupy/extensions/single_file.py index 857ad8e9..d25ae14e 100644 --- a/src/syrupy/extensions/single_file.py +++ b/src/syrupy/extensions/single_file.py @@ -43,9 +43,7 @@ def _get_file_basename(self, *, index: int) -> str: @property def _dirname(self) -> str: original_dirname = super(SingleFileSnapshotExtension, self)._dirname - file_basename = os.path.basename(str(self.test_location.filename)) - sub_dirname = os.path.splitext(file_basename)[0] - return os.path.join(original_dirname, sub_dirname) + return os.path.join(original_dirname, self.test_location.filename) def _read_snapshot_fossil(self, *, snapshot_location: str) -> "SnapshotFossil": snapshot_fossil = SnapshotFossil(location=snapshot_location) diff --git a/src/syrupy/location.py b/src/syrupy/location.py index e183a9d4..1a1b5f4d 100644 --- a/src/syrupy/location.py +++ b/src/syrupy/location.py @@ -1,3 +1,4 @@ +import os from typing import ( Any, Optional, @@ -7,7 +8,7 @@ class TestLocation(object): def __init__(self, node: Any): self._node = node - self.filename = self._node.fspath + self.filepath = self._node.fspath self.modulename = self._node.obj.__module__ self.methodname = self._node.obj.__name__ self.nodename = getattr(self._node, "name", None) @@ -18,7 +19,14 @@ def classname(self) -> Optional[str]: classes = self._node.obj.__qualname__.split(".")[:-1] return ".".join(classes) if classes else None + @property + def filename(self) -> str: + return str(os.path.splitext(os.path.basename(self.filepath))[0]) + def matches_snapshot_name(self, snapshot_name: str) -> bool: matches_basemethod = str(self.methodname) in snapshot_name matches_testnode = snapshot_name in str(self.nodename) return matches_basemethod or matches_testnode + + def matches_snapshot_location(self, snapshot_location: str) -> bool: + return self.filename in snapshot_location diff --git a/src/syrupy/report.py b/src/syrupy/report.py index 9c35bba9..d912e196 100644 --- a/src/syrupy/report.py +++ b/src/syrupy/report.py @@ -49,16 +49,9 @@ class SnapshotReport(object): updated: "SnapshotFossils" = attr.ib(factory=SnapshotFossils) used: "SnapshotFossils" = attr.ib(factory=SnapshotFossils) - def filter_fossils(self, item: "SnapshotFossil") -> bool: - if not self.ran_items or not self.is_providing_paths: - return True - - target_snaps = {snap.location for snap in self.used} - - return item.location in target_snaps - def __attrs_post_init__(self) -> None: for assertion in self.assertions: + self.discovered.merge(assertion.extension.discover_snapshots()) for result in assertion.executions.values(): snapshot_fossil = SnapshotFossil(location=result.snapshot_location) snapshot_fossil.add( @@ -74,14 +67,6 @@ def __attrs_post_init__(self) -> None: else: self.failed.update(snapshot_fossil) - discovered_snaps = assertion.extension.discover_snapshots() - filtered_snaps = { - snap.location: snap - for snap in discovered_snaps - if self.filter_fossils(snap) - } - self.discovered.merge(SnapshotFossils(filtered_snaps)) - @property def num_created(self) -> int: return self._count_snapshots(self.created) @@ -113,6 +98,12 @@ def unused(self) -> "SnapshotFossils": self.discovered, self.used ): snapshot_location = unused_snapshot_fossil.location + if self.is_providing_paths and not any( + TestLocation(node).matches_snapshot_location(snapshot_location) + for node in self.ran_items + ): + continue + if self.ran_all_collected_tests: unused_snapshots = {*unused_snapshot_fossil} mark_for_removal = snapshot_location not in self.used diff --git a/tests/examples/test_custom_snapshot_directory.py b/tests/examples/test_custom_snapshot_directory.py index 9c5335d0..3fc77c9f 100644 --- a/tests/examples/test_custom_snapshot_directory.py +++ b/tests/examples/test_custom_snapshot_directory.py @@ -23,7 +23,7 @@ class DifferentDirectoryExtension(AmberSnapshotExtension): @property def _dirname(self) -> str: - test_dirname = os.path.dirname(self.test_location.filename) + test_dirname = os.path.dirname(self.test_location.filepath) return os.path.join(test_dirname, DIFFERENT_DIRECTORY) diff --git a/tests/test_integration_custom.py b/tests/test_integration_custom.py index 0fe0f3e4..4dd310e1 100644 --- a/tests/test_integration_custom.py +++ b/tests/test_integration_custom.py @@ -34,6 +34,9 @@ def _write_snapshot_fossil(self, **kwargs): def delete_snapshots(self, **kwargs): pass + def _get_file_basename(self, *, index = 0): + return self.test_location.filename[::-1] + @pytest.fixture def snapshot_custom(snapshot): @@ -57,5 +60,7 @@ def test_warns_on_snapshot_name(testdir, testcases): result_stdout = clean_output(result.stdout.str()) assert "2 snapshots generated" in result_stdout assert "Warning:" in result_stdout + assert "Can not relate snapshot name" in result_stdout + assert "Can not relate snapshot location" in result_stdout assert "test_passed_custom" in result_stdout assert result.ret == 0 diff --git a/tests/test_integration_default.py b/tests/test_integration_default.py index f918f76b..4566fea3 100644 --- a/tests/test_integration_default.py +++ b/tests/test_integration_default.py @@ -5,14 +5,16 @@ from .utils import clean_output -def test_collection(testdir): +@pytest.fixture +def collection(testdir): tests = { "test_collected": ( """ - def test_collected(snapshot): - assert snapshot == 1 - assert snapshot == "2" - assert snapshot == {1,1,1} + import pytest + + @pytest.mark.parametrize("actual", [1, 2, 3]) + def test_collected(snapshot, actual): + assert snapshot == actual """ ), "test_not_collected": ( @@ -26,49 +28,59 @@ def test_not_collected(snapshot): result = testdir.runpytest("-v", "--snapshot-update") result_stdout = clean_output(result.stdout.str()) assert "4 snapshots generated" in result_stdout + testdir.makefile(".ambr", **{"__snapshots__/other_snapfile": ""}) + return testdir + +def test_unused_snapshots_ignored_if_not_targeted_using_dash_m(collection): updated_tests = { "test_collected": ( """ - def test_collected(snapshot): - assert snapshot == 1 - assert snapshot == "two" + import pytest + + @pytest.mark.parametrize("actual", [1, "2"]) + def test_collected(snapshot, actual): + assert snapshot == actual """ ), } - testdir.makepyfile(**updated_tests) - result = testdir.runpytest("-v", "--snapshot-update", "-k", "test_collected") + collection.makepyfile(**updated_tests) + result = collection.runpytest("-v", "--snapshot-update", "-m", "parametrize") result_stdout = clean_output(result.stdout.str()) assert "1 snapshot passed" in result_stdout assert "1 snapshot updated" in result_stdout assert "1 unused snapshot deleted" in result_stdout + snapshot_path = [collection.tmpdir, "__snapshots__"] + assert os.path.isfile(os.path.join(*snapshot_path, "test_not_collected.ambr")) + assert os.path.isfile(os.path.join(*snapshot_path, "other_snapfile.ambr")) -def test_collection_parametrized(testdir): - tests = { - "test_parametrized": ( +def test_unused_snapshots_ignored_if_not_targeted_using_dash_k(collection): + updated_tests = { + "test_collected": ( """ import pytest - @pytest.mark.parametrize("actual", [1, 2, 3]) + @pytest.mark.parametrize("actual", [1, "2"]) def test_collected(snapshot, actual): assert snapshot == actual """ ), - "test_not_collected": ( - """ - def test_not_collected(snapshot): - assert snapshot == "hello" - """ - ), } - testdir.makepyfile(**tests) - result = testdir.runpytest("-v", "--snapshot-update") + collection.makepyfile(**updated_tests) + result = collection.runpytest("-v", "--snapshot-update", "-k", "test_collected") result_stdout = clean_output(result.stdout.str()) - assert "4 snapshots generated" in result_stdout + assert "1 snapshot passed" in result_stdout + assert "1 snapshot updated" in result_stdout + assert "1 unused snapshot deleted" in result_stdout + snapshot_path = [collection.tmpdir, "__snapshots__"] + assert os.path.isfile(os.path.join(*snapshot_path, "test_not_collected.ambr")) + assert os.path.isfile(os.path.join(*snapshot_path, "other_snapfile.ambr")) + +def test_unused_parameterized_ignored_if_not_targeted_using_dash_k(collection): updated_tests = { - "test_parametrized": ( + "test_collected": ( """ import pytest @@ -78,12 +90,15 @@ def test_collected(snapshot, actual): """ ), } - testdir.makepyfile(**updated_tests) - result = testdir.runpytest("-v", "--snapshot-update", "-k", "test_collected") + collection.makepyfile(**updated_tests) + result = collection.runpytest("-v", "--snapshot-update", "-k", "test_collected") result_stdout = clean_output(result.stdout.str()) assert "2 snapshots passed" in result_stdout assert "snapshot updated" not in result_stdout assert "1 unused snapshot deleted" in result_stdout + snapshot_path = [collection.tmpdir, "__snapshots__"] + assert os.path.isfile(os.path.join(*snapshot_path, "test_not_collected.ambr")) + assert os.path.isfile(os.path.join(*snapshot_path, "other_snapfile.ambr")) @pytest.fixture @@ -271,8 +286,8 @@ def test_unused_snapshots_warning(stubs): assert result.ret == 0 -def test_unused_snapshots_ignored_if_not_targeted_when_targeting_using_dash_k(stubs): - _, testdir, tests, _ = stubs +def test_unused_snapshots_ignored_if_not_targeted_by_testnode_ids(stubs): + _, testdir, tests, snapshot_file = stubs testdir.makepyfile(test_file="\n\n".join(tests[k] for k in tests if k != "unused")) testdir.makefile( ".ambr", **{"__snapshots__/other_snapfile": ""}, @@ -283,18 +298,18 @@ def test_unused_snapshots_ignored_if_not_targeted_when_targeting_using_dash_k(st testdir.makefile( ".py", **{"test_life_always_finds_a_way": test_content}, ) + testfile = os.path.join(testdir.tmpdir, "test_life_always_finds_a_way.py") result = testdir.runpytest( - "-k", "life_always_finds_a_way", "-v", "--snapshot-update" + f"{testfile}::test_life_always_finds_a_way", "-v", "--snapshot-update" ) result_stdout = clean_output(result.stdout.str()) assert "1 snapshot generated" in result_stdout assert result.ret == 0 + assert os.path.isfile(snapshot_file) assert os.path.isfile("__snapshots__/other_snapfile.ambr") -def test_unused_snapshots_ignored_if_not_targeted_when_targeting_specific_testfiles( - stubs, -): +def test_unused_snapshots_ignored_if_not_targeted_by_module_testfiles(stubs): _, testdir, tests, _ = stubs testdir.makepyfile(test_file="\n\n".join(tests[k] for k in tests if k != "unused")) testdir.makefile( @@ -307,6 +322,26 @@ def test_unused_snapshots_ignored_if_not_targeted_when_targeting_specific_testfi assert os.path.isfile("__snapshots__/other_snapfile.ambr") +def test_unused_snapshots_cleaned_up_when_targeting_specific_testfiles(stubs): + _, testdir, tests, _ = stubs + testdir.makepyfile( + test_file=( + """ + def test_used(snapshot): + assert True + """ + ) + ) + testdir.makefile( + ".ambr", **{"__snapshots__/other_snapfile": ""}, + ) + result = testdir.runpytest("-v", "--snapshot-update", "test_file.py") + result_stdout = clean_output(result.stdout.str()) + assert "7 unused snapshots deleted" in result_stdout + assert result.ret == 0 + assert os.path.isfile("__snapshots__/other_snapfile.ambr") + + def test_removed_snapshots(stubs): _, testdir, tests, filepath = stubs assert os.path.isfile(filepath) @@ -333,9 +368,8 @@ def test_removed_snapshot_fossil(stubs): def test_removed_empty_snapshot_fossil_only(stubs): _, testdir, _, filepath = stubs - empty_filepath = os.path.join(os.path.dirname(filepath), "test_empty.ambr") - with open(empty_filepath, "w") as empty_snapfile: - empty_snapfile.write("") + testdir.makefile(".ambr", **{"__snapshots__/test_empty": ""}) + empty_filepath = os.path.join(testdir.tmpdir, "__snapshots__/test_empty.ambr") assert os.path.isfile(empty_filepath) result = testdir.runpytest("-v", "--snapshot-update") result_stdout = clean_output(result.stdout.str()) @@ -350,9 +384,8 @@ def test_removed_empty_snapshot_fossil_only(stubs): def test_removed_hanging_snapshot_fossil(stubs): _, testdir, _, filepath = stubs - hanging_filepath = os.path.join(os.path.dirname(filepath), "test_hanging.abc") - with open(hanging_filepath, "w") as empty_snapfile: - empty_snapfile.write("some dummy content") + testdir.makefile(".abc", **{"__snapshots__/test_hanging": ""}) + hanging_filepath = os.path.join(testdir.tmpdir, "__snapshots__/test_hanging.abc") assert os.path.isfile(hanging_filepath) result = testdir.runpytest("-v", "--snapshot-update") result_stdout = clean_output(result.stdout.str())