From e6a5d38562d9c446bd45db1789c7f27f41bc5fea Mon Sep 17 00:00:00 2001 From: Robin Date: Thu, 14 May 2026 17:12:10 -0400 Subject: [PATCH 01/10] add get_recorded_paths() to retrieve TOML shortcut paths --- menuinst/api.py | 7 ++++++ tests/test_metadata.py | 48 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/menuinst/api.py b/menuinst/api.py index 5481a58f..9847783d 100644 --- a/menuinst/api.py +++ b/menuinst/api.py @@ -89,6 +89,13 @@ def remove_shortcut_records(prefix: Path, source: str) -> None: write_menuinst_toml(prefix, data) +def get_recorded_paths(prefix: Path, source: str) -> list[str]: + """Get recorded shortcut paths for a source from menuinst.toml.""" + data = read_menuinst_toml(prefix) + shortcuts = data.get("shortcuts", []) + return [s["path"] for s in shortcuts if s.get("source") == source and "path" in s] + + def _load( metadata_or_path: os.PathLike | dict, target_prefix: str | None = None, diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 1f786e83..e58f74f1 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -6,6 +6,7 @@ from menuinst.api import ( _install_adapter, + get_recorded_paths, record_shortcuts, remove_shortcut_records, write_menuinst_toml, @@ -219,3 +220,50 @@ def test_toml_writes_schemaver_format(self, tmp_path): assert data["schema_version"] == MENUINST_TOML_SCHEMA_VERSION # Verify it's a valid SchemaVer string parse_schemaver(data["schema_version"]) + + +class TestGetRecordedPaths: + """Tests for get_recorded_paths().""" + + def test_returns_paths_for_source(self, tmp_path): + """Should return paths matching the given source.""" + write_menuinst_toml( + tmp_path, + { + "shortcuts": [ + {"source": "foo.json", "path": "/path/to/foo.lnk"}, + {"source": "foo.json", "path": "/path/to/bar.lnk"}, + {"source": "baz.json", "path": "/path/to/baz.lnk"}, + ], + }, + ) + paths = get_recorded_paths(tmp_path, "foo.json") + assert paths == ["/path/to/foo.lnk", "/path/to/bar.lnk"] + + def test_returns_empty_list_when_no_matches(self, tmp_path): + """Should return empty list when no shortcuts match source.""" + write_menuinst_toml( + tmp_path, + {"shortcuts": [{"source": "other.json", "path": "/path/to/other.lnk"}]}, + ) + paths = get_recorded_paths(tmp_path, "foo.json") + assert paths == [] + + def test_returns_empty_list_when_no_toml(self, tmp_path): + """Should return empty list when menuinst.toml doesn't exist.""" + paths = get_recorded_paths(tmp_path, "foo.json") + assert paths == [] + + def test_skips_entries_missing_path_key(self, tmp_path): + """Should skip shortcuts missing the 'path' key.""" + write_menuinst_toml( + tmp_path, + { + "shortcuts": [ + {"source": "foo.json", "path": "/valid/path.lnk"}, + {"source": "foo.json"}, # Missing path key + ], + }, + ) + paths = get_recorded_paths(tmp_path, "foo.json") + assert paths == ["/valid/path.lnk"] From 0320ec0ee6350e77251d4e2159f07838313b6cca Mon Sep 17 00:00:00 2001 From: Robin Date: Thu, 14 May 2026 17:15:08 -0400 Subject: [PATCH 02/10] Add delete_paths() to api.py --- menuinst/api.py | 26 +++++++++++++++++++ tests/test_metadata.py | 58 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/menuinst/api.py b/menuinst/api.py index 9847783d..01aeaa07 100644 --- a/menuinst/api.py +++ b/menuinst/api.py @@ -4,6 +4,7 @@ import json import os +import shutil import sys import warnings from logging import getLogger @@ -17,6 +18,7 @@ _UserOrSystem, elevate_as_needed, read_menuinst_toml, + unlink, user_is_admin, write_menuinst_toml, ) @@ -96,6 +98,30 @@ def get_recorded_paths(prefix: Path, source: str) -> list[str]: return [s["path"] for s in shortcuts if s.get("source") == source and "path" in s] +def delete_paths(paths: list[str]) -> list[Path]: + """Delete files or directories at given paths, warning if not found. + + Handles both files (.lnk, .desktop) and directories (.app bundles on macOS). + """ + deleted = [] + for path_str in paths: + path = Path(path_str) + if path.is_file(): + log.debug("Removing %s", path) + unlink(path) + deleted.append(path) + elif path.is_dir(): + log.debug("Removing %s", path) + shutil.rmtree(path, ignore_errors=True) + deleted.append(path) + else: + log.warning( + "Shortcut not found at expected location: %s - may have been moved or deleted", + path, + ) + return deleted + + def _load( metadata_or_path: os.PathLike | dict, target_prefix: str | None = None, diff --git a/tests/test_metadata.py b/tests/test_metadata.py index e58f74f1..a3d1dcd6 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -6,6 +6,7 @@ from menuinst.api import ( _install_adapter, + delete_paths, get_recorded_paths, record_shortcuts, remove_shortcut_records, @@ -267,3 +268,60 @@ def test_skips_entries_missing_path_key(self, tmp_path): ) paths = get_recorded_paths(tmp_path, "foo.json") assert paths == ["/valid/path.lnk"] + + +class TestDeletePaths: + """Tests for delete_paths().""" + + def test_deletes_existing_files(self, tmp_path): + """Should delete files that exist and return their paths.""" + file1 = tmp_path / "foo.lnk" + file2 = tmp_path / "bar.lnk" + file1.touch() + file2.touch() + + deleted = delete_paths([str(file1), str(file2)]) + + assert not file1.exists() + assert not file2.exists() + assert len(deleted) == 2 + + def test_deletes_directories(self, tmp_path): + """Should delete directories (e.g., .app bundles) using rmtree.""" + app_dir = tmp_path / "MyApp.app" + app_dir.mkdir() + (app_dir / "Contents").mkdir() + (app_dir / "Contents" / "Info.plist").touch() + + deleted = delete_paths([str(app_dir)]) + + assert not app_dir.exists() + assert len(deleted) == 1 + + def test_warns_on_missing_path(self, tmp_path, caplog): + """Should log warning when path doesn't exist.""" + import logging + + missing = tmp_path / "nonexistent.lnk" + + with caplog.at_level(logging.WARNING): + deleted = delete_paths([str(missing)]) + + assert len(deleted) == 0 + assert "Shortcut not found at expected location" in caplog.text + assert str(missing) in caplog.text + + def test_partial_deletion(self, tmp_path, caplog): + """Should delete existing paths and warn about missing ones.""" + import logging + + existing = tmp_path / "exists.lnk" + missing = tmp_path / "missing.lnk" + existing.touch() + + with caplog.at_level(logging.WARNING): + deleted = delete_paths([str(existing), str(missing)]) + + assert not existing.exists() + assert len(deleted) == 1 + assert "missing.lnk" in caplog.text From 9d29114f1b271b82fba93ce51045eb20d2dc2e0a Mon Sep 17 00:00:00 2001 From: Robin Date: Thu, 14 May 2026 17:18:29 -0400 Subject: [PATCH 03/10] Refactor remove functions --- menuinst/platforms/linux.py | 22 +++++++++++++++------- menuinst/platforms/osx.py | 13 +++++++++++-- menuinst/platforms/win.py | 11 +++++++++-- 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/menuinst/platforms/linux.py b/menuinst/platforms/linux.py index 995b897f..9685c071 100644 --- a/menuinst/platforms/linux.py +++ b/menuinst/platforms/linux.py @@ -206,16 +206,24 @@ def create(self) -> Iterable[os.PathLike]: self._update_desktop_database() return self._paths() - def remove(self) -> Iterable[os.PathLike]: - paths = (path for path in self._paths() if Path(path).is_file()) + def cleanup_side_effects(self) -> None: + """Unregister MIME types and update desktop database.""" self._maybe_register_mime_types(register=False) - if paths: - for path in paths: - log.debug("Removing %s", path) - unlink(path) - self._update_desktop_database() + self._update_desktop_database() + + def delete_paths(self) -> tuple[Path, ...]: + """Delete .desktop file and related MIME XML files.""" + paths = tuple(path for path in self._paths() if Path(path).is_file()) + for path in paths: + log.debug("Removing %s", path) + unlink(path) return paths + def remove(self) -> tuple[Path, ...]: + """Remove .desktop file and clean up side effects.""" + self.cleanup_side_effects() + return self.delete_paths() + def _update_desktop_database(self): exe = shutil.which("update-desktop-database") if exe: diff --git a/menuinst/platforms/osx.py b/menuinst/platforms/osx.py index dc85651f..6279de81 100644 --- a/menuinst/platforms/osx.py +++ b/menuinst/platforms/osx.py @@ -96,14 +96,23 @@ def create(self) -> tuple[Path]: self._sign_with_entitlements() return (self.location,) - def remove(self) -> tuple[Path]: - log.debug("Removing %s", self.location) + def cleanup_side_effects(self) -> None: + """Unregister from LaunchServices.""" self._maybe_register_with_launchservices(register=False) + + def delete_paths(self) -> tuple[Path, ...]: + """Delete .app bundle.""" + log.debug("Removing %s", self.location) if self.location.exists(): shutil.rmtree(self.location, ignore_errors=True) return (self.location,) return tuple() + def remove(self) -> tuple[Path, ...]: + """Remove .app bundle and clean up side effects.""" + self.cleanup_side_effects() + return self.delete_paths() + def _create_application_tree(self) -> tuple[Path]: paths = [ self.location / "Contents" / "Resources", diff --git a/menuinst/platforms/win.py b/menuinst/platforms/win.py index 9cb80bbd..847ccdb4 100644 --- a/menuinst/platforms/win.py +++ b/menuinst/platforms/win.py @@ -208,7 +208,8 @@ def create(self) -> tuple[Path, ...]: return paths - def remove(self) -> tuple[Path, ...]: + def cleanup_side_effects(self) -> None: + """Clean up registry entries, terminal profiles, etc.""" changed_extensions = self._unregister_file_extensions() changed_protocols = self._unregister_url_protocols() if changed_extensions or changed_protocols: @@ -217,13 +218,19 @@ def remove(self) -> tuple[Path, ...]: for location in self.menu.terminal_profile_locations: self._add_remove_windows_terminal_profile(location, remove=True) + def delete_paths(self) -> tuple[Path, ...]: + """Delete shortcut files.""" paths = tuple(path for path in self._paths() if Path(path).is_file()) for path in paths: log.debug("Removing %s", path) unlink(path) - return paths + def remove(self) -> tuple[Path, ...]: + """Remove shortcut files and clean up side effects.""" + self.cleanup_side_effects() + return self.delete_paths() + def _paths(self) -> tuple[Path, ...]: paths = [self.location] extra_dirs = [] From 228250dc6c93a12d46296cf9a9b51e14d0757324 Mon Sep 17 00:00:00 2001 From: Robin Date: Thu, 14 May 2026 17:29:42 -0400 Subject: [PATCH 04/10] Update api.remove() to use TOML paths --- menuinst/api.py | 50 +++++++++++++++++------- tests/test_metadata.py | 86 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 14 deletions(-) diff --git a/menuinst/api.py b/menuinst/api.py index 01aeaa07..bf9a3031 100644 --- a/menuinst/api.py +++ b/menuinst/api.py @@ -162,15 +162,20 @@ def install( paths += menu_item.create() # Record shortcuts to menuinst.toml + # Only record MenuItem paths, not Menu paths (.directory files, etc.) + # The Menu's remove() method handles its own cleanup logic if isinstance(metadata_or_path, (str, Path)): source = Path(metadata_or_path).name else: source = f"{menu.name}.json" + item_paths = [] + for menu_item in menu_items: + item_paths += menu_item.create() record_shortcuts( Path(target_prefix), Path(base_prefix), source, - paths, + item_paths, distribution_name=menu.placeholders.get("DISTRIBUTION_NAME"), ) @@ -187,29 +192,46 @@ def remove( ) -> list[os.PathLike]: target_prefix = target_prefix or DEFAULT_PREFIX base_prefix = base_prefix or DEFAULT_BASE_PREFIX + + # When running as admin with .nonadmin marker, force user mode + # This ensures Menu and MenuItem objects use correct paths + if _maybe_try_user(target_prefix, base_prefix): + _mode = "user" + menu, menu_items = _load(metadata_or_path, target_prefix, base_prefix, _mode) menu_items = [item for item in menu_items if item.enabled_for_platform()] if not menu_items: warnings.warn(f"Metadata for {menu.name} is not enabled for {sys.platform}") return [] - paths = [] - for menu_item in menu_items: - paths += menu_item.remove() - paths += menu.remove() - - if not paths and _maybe_try_user(target_prefix, base_prefix): - menu, menu_items = _load(metadata_or_path, target_prefix, base_prefix, "user") - menu_items = [item for item in menu_items if item.enabled_for_platform()] - for menu_item in menu_items: - paths += menu_item.remove() - paths += menu.remove() - - # Remove shortcut records from menuinst.toml + # Determine source name for TOML lookup if isinstance(metadata_or_path, (str, Path)): source = Path(metadata_or_path).name else: source = f"{menu.name}.json" + + # 1. Delete MenuItem shortcut files (.lnk, .desktop, .app) + recorded_paths = get_recorded_paths(Path(target_prefix), source) + if recorded_paths: + paths = delete_paths(recorded_paths) + else: + # Fallback for shortcuts created before menuinst.toml tracking existed. + # This ensures backwards compatibility when upgrading menuinst after + # packages with shortcuts were already installed. + paths = [] + for menu_item in menu_items: + paths.extend(menu_item.delete_paths()) + + # 2. Side-effect cleanup (registry, LaunchServices, MIME types) + for menu_item in menu_items: + menu_item.cleanup_side_effects() + + # 3. Menu directory cleanup (.directory files, etc.) + # The Menu handles its own removal logic (e.g., only removes .directory + # if no other shortcuts from the same menu exist) + paths.extend(menu.remove()) + + # 4. Update TOML records remove_shortcut_records(Path(target_prefix), source) return paths diff --git a/tests/test_metadata.py b/tests/test_metadata.py index a3d1dcd6..762007bf 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -8,7 +8,9 @@ _install_adapter, delete_paths, get_recorded_paths, + install, record_shortcuts, + remove, remove_shortcut_records, write_menuinst_toml, ) @@ -325,3 +327,87 @@ def test_partial_deletion(self, tmp_path, caplog): assert not existing.exists() assert len(deleted) == 1 assert "missing.lnk" in caplog.text + + +class TestRemoveUsesTomlPaths: + """Tests for TOML-based shortcut removal. + + These tests verify the TOML path mechanism works correctly, independent of + platform-specific removal behavior. The tests create generic files (not + actual .lnk/.desktop/.app) to test that recorded paths are used. + """ + + def test_remove_uses_recorded_paths(self, tmp_path, monkeypatch): + """remove() should delete files at recorded TOML paths, not computed paths. + + This tests the TOML lookup mechanism, not full platform removal flow. + """ + monkeypatch.delenv("MENUINST_DISTRIBUTION_NAME", raising=False) + (tmp_path / ".nonadmin").touch() + menu_dir = tmp_path / "Menu" + menu_dir.mkdir() + + # Create a shortcut file at a "recorded" location + recorded_path = tmp_path / "recorded_location" / "MyShortcut.lnk" + recorded_path.parent.mkdir(parents=True) + recorded_path.touch() + + # Pre-populate TOML with the recorded path + write_menuinst_toml( + tmp_path, + {"shortcuts": [{"source": "test.json", "path": str(recorded_path)}]}, + ) + + # Create JSON that would compute a DIFFERENT path + json_file = menu_dir / "test.json" + json_file.write_text( + json.dumps( + { + "$schema": "https://json-schema.org/draft-07/schema", + "menu_name": "Test Menu", + "menu_items": [ + { + "name": "MyShortcut", + "command": ["echo", "test"], + "activate": False, + "platforms": {"linux": {}, "win": {}, "osx": {}}, + } + ], + } + ) + ) + + # remove() should delete the file at the recorded path + remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) + + assert not recorded_path.exists(), "File at recorded TOML path should be deleted" + + def test_remove_falls_back_when_no_toml_entries(self, tmp_path, monkeypatch): + """remove() should fall back to computed paths when no TOML entries exist.""" + monkeypatch.delenv("MENUINST_DISTRIBUTION_NAME", raising=False) + (tmp_path / ".nonadmin").touch() + menu_dir = tmp_path / "Menu" + menu_dir.mkdir() + + # Create JSON (no TOML entries for this source) + json_file = menu_dir / "test.json" + json_file.write_text( + json.dumps( + { + "$schema": "https://json-schema.org/draft-07/schema", + "menu_name": "Test Menu", + "menu_items": [ + { + "name": "MyShortcut", + "command": ["echo", "test"], + "activate": False, + "platforms": {"linux": {}, "win": {}, "osx": {}}, + } + ], + } + ) + ) + + # This should not raise - it should use the fallback path + paths = remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) + # No assertion on paths - just verifying no exception From 138585cc5c9d6d6332ffb4874cdd315ec5f09b25 Mon Sep 17 00:00:00 2001 From: Robin Date: Thu, 14 May 2026 17:34:17 -0400 Subject: [PATCH 05/10] Remove TODO and pre-commit fix --- menuinst/api.py | 7 +------ tests/test_metadata.py | 5 ++--- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/menuinst/api.py b/menuinst/api.py index bf9a3031..df089729 100644 --- a/menuinst/api.py +++ b/menuinst/api.py @@ -68,12 +68,7 @@ def record_shortcuts( def remove_shortcut_records(prefix: Path, source: str) -> None: - """Remove shortcut entries matching source from menuinst.toml. - - TODO: Use the recorded paths as the source of truth for shortcut removal, - instead of recomputing paths from menu JSON metadata. This would handle - cases where shortcuts were moved or the menu JSON changed. - """ + """Remove shortcut entries matching source from menuinst.toml.""" data = read_menuinst_toml(prefix) if not data: return diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 762007bf..4293fe4f 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -8,7 +8,6 @@ _install_adapter, delete_paths, get_recorded_paths, - install, record_shortcuts, remove, remove_shortcut_records, @@ -409,5 +408,5 @@ def test_remove_falls_back_when_no_toml_entries(self, tmp_path, monkeypatch): ) # This should not raise - it should use the fallback path - paths = remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) - # No assertion on paths - just verifying no exception + remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) + # No assertion on return value - just verifying no exception From 2cb3753cc0211da4a9b0707fd7113c90fc9a2078 Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 15 May 2026 09:32:32 -0400 Subject: [PATCH 06/10] fix bug --- menuinst/api.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/menuinst/api.py b/menuinst/api.py index df089729..24f6367a 100644 --- a/menuinst/api.py +++ b/menuinst/api.py @@ -153,8 +153,11 @@ def install( paths = [] paths += menu.create() + item_paths = [] for menu_item in menu_items: - paths += menu_item.create() + created = menu_item.create() + paths += created + item_paths += created # Record shortcuts to menuinst.toml # Only record MenuItem paths, not Menu paths (.directory files, etc.) @@ -163,9 +166,6 @@ def install( source = Path(metadata_or_path).name else: source = f"{menu.name}.json" - item_paths = [] - for menu_item in menu_items: - item_paths += menu_item.create() record_shortcuts( Path(target_prefix), Path(base_prefix), From e685b6a706e1bc90175d593936874e9fc242f25f Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 15 May 2026 09:42:59 -0400 Subject: [PATCH 07/10] temporarily enable test for fork --- .github/workflows/tests.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 00b4f4f9..8a818f0e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -1,14 +1,14 @@ name: Tests +# TODO: REVERT - temporarily allow PR tests on fork branches on: push: branches: - main + - dev-ra-829-2 paths-ignore: - "docs/**" pull_request: - branches: - - main paths-ignore: - "docs/**" From 2368861ca7caa1a88ad9f0521a838a349c35c2e4 Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 15 May 2026 10:10:20 -0400 Subject: [PATCH 08/10] Improve tests --- tests/test_metadata.py | 56 ++++++++++-------------------------------- 1 file changed, 13 insertions(+), 43 deletions(-) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 4293fe4f..ef6e1b4d 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -1,6 +1,7 @@ """Tests for distribution_name resolution and menuinst.toml tracking.""" import json +import logging import pytest @@ -228,7 +229,7 @@ class TestGetRecordedPaths: """Tests for get_recorded_paths().""" def test_returns_paths_for_source(self, tmp_path): - """Should return paths matching the given source.""" + """Test that paths matching the given source are returned.""" write_menuinst_toml( tmp_path, { @@ -243,7 +244,7 @@ def test_returns_paths_for_source(self, tmp_path): assert paths == ["/path/to/foo.lnk", "/path/to/bar.lnk"] def test_returns_empty_list_when_no_matches(self, tmp_path): - """Should return empty list when no shortcuts match source.""" + """Test that empty list is returned when no shortcuts match source.""" write_menuinst_toml( tmp_path, {"shortcuts": [{"source": "other.json", "path": "/path/to/other.lnk"}]}, @@ -252,12 +253,12 @@ def test_returns_empty_list_when_no_matches(self, tmp_path): assert paths == [] def test_returns_empty_list_when_no_toml(self, tmp_path): - """Should return empty list when menuinst.toml doesn't exist.""" + """Test that empty list is returned when menuinst.toml doesn't exist.""" paths = get_recorded_paths(tmp_path, "foo.json") assert paths == [] def test_skips_entries_missing_path_key(self, tmp_path): - """Should skip shortcuts missing the 'path' key.""" + """Test that shortcuts missing the 'path' key are skipped.""" write_menuinst_toml( tmp_path, { @@ -275,7 +276,7 @@ class TestDeletePaths: """Tests for delete_paths().""" def test_deletes_existing_files(self, tmp_path): - """Should delete files that exist and return their paths.""" + """Test that existing files are deleted and their paths returned.""" file1 = tmp_path / "foo.lnk" file2 = tmp_path / "bar.lnk" file1.touch() @@ -288,7 +289,7 @@ def test_deletes_existing_files(self, tmp_path): assert len(deleted) == 2 def test_deletes_directories(self, tmp_path): - """Should delete directories (e.g., .app bundles) using rmtree.""" + """Test that directories (e.g., .app bundles) are deleted using rmtree.""" app_dir = tmp_path / "MyApp.app" app_dir.mkdir() (app_dir / "Contents").mkdir() @@ -300,9 +301,7 @@ def test_deletes_directories(self, tmp_path): assert len(deleted) == 1 def test_warns_on_missing_path(self, tmp_path, caplog): - """Should log warning when path doesn't exist.""" - import logging - + """Test that a warning is logged when path doesn't exist.""" missing = tmp_path / "nonexistent.lnk" with caplog.at_level(logging.WARNING): @@ -312,52 +311,26 @@ def test_warns_on_missing_path(self, tmp_path, caplog): assert "Shortcut not found at expected location" in caplog.text assert str(missing) in caplog.text - def test_partial_deletion(self, tmp_path, caplog): - """Should delete existing paths and warn about missing ones.""" - import logging - - existing = tmp_path / "exists.lnk" - missing = tmp_path / "missing.lnk" - existing.touch() - - with caplog.at_level(logging.WARNING): - deleted = delete_paths([str(existing), str(missing)]) - - assert not existing.exists() - assert len(deleted) == 1 - assert "missing.lnk" in caplog.text - class TestRemoveUsesTomlPaths: - """Tests for TOML-based shortcut removal. - - These tests verify the TOML path mechanism works correctly, independent of - platform-specific removal behavior. The tests create generic files (not - actual .lnk/.desktop/.app) to test that recorded paths are used. - """ + """Tests for TOML-based shortcut removal.""" def test_remove_uses_recorded_paths(self, tmp_path, monkeypatch): - """remove() should delete files at recorded TOML paths, not computed paths. - - This tests the TOML lookup mechanism, not full platform removal flow. - """ + """Test that remove() deletes files at recorded TOML paths, not computed paths.""" monkeypatch.delenv("MENUINST_DISTRIBUTION_NAME", raising=False) (tmp_path / ".nonadmin").touch() menu_dir = tmp_path / "Menu" menu_dir.mkdir() - # Create a shortcut file at a "recorded" location recorded_path = tmp_path / "recorded_location" / "MyShortcut.lnk" recorded_path.parent.mkdir(parents=True) recorded_path.touch() - # Pre-populate TOML with the recorded path write_menuinst_toml( tmp_path, {"shortcuts": [{"source": "test.json", "path": str(recorded_path)}]}, ) - # Create JSON that would compute a DIFFERENT path json_file = menu_dir / "test.json" json_file.write_text( json.dumps( @@ -376,19 +349,17 @@ def test_remove_uses_recorded_paths(self, tmp_path, monkeypatch): ) ) - # remove() should delete the file at the recorded path remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) - assert not recorded_path.exists(), "File at recorded TOML path should be deleted" + assert not recorded_path.exists() def test_remove_falls_back_when_no_toml_entries(self, tmp_path, monkeypatch): - """remove() should fall back to computed paths when no TOML entries exist.""" + """Test that remove() falls back to computed paths when no TOML entries exist.""" monkeypatch.delenv("MENUINST_DISTRIBUTION_NAME", raising=False) (tmp_path / ".nonadmin").touch() menu_dir = tmp_path / "Menu" menu_dir.mkdir() - # Create JSON (no TOML entries for this source) json_file = menu_dir / "test.json" json_file.write_text( json.dumps( @@ -407,6 +378,5 @@ def test_remove_falls_back_when_no_toml_entries(self, tmp_path, monkeypatch): ) ) - # This should not raise - it should use the fallback path + # Verifying no exception is raised remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) - # No assertion on return value - just verifying no exception From 1b871e4b1d8ff32842c912fd61087acee507d0f0 Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 15 May 2026 16:51:16 -0400 Subject: [PATCH 09/10] Update test, change return type --- menuinst/api.py | 9 ++++----- tests/test_metadata.py | 14 +++++++------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/menuinst/api.py b/menuinst/api.py index 24f6367a..3696d75c 100644 --- a/menuinst/api.py +++ b/menuinst/api.py @@ -86,21 +86,20 @@ def remove_shortcut_records(prefix: Path, source: str) -> None: write_menuinst_toml(prefix, data) -def get_recorded_paths(prefix: Path, source: str) -> list[str]: +def get_recorded_paths(prefix: Path, source: str) -> list[Path]: """Get recorded shortcut paths for a source from menuinst.toml.""" data = read_menuinst_toml(prefix) shortcuts = data.get("shortcuts", []) - return [s["path"] for s in shortcuts if s.get("source") == source and "path" in s] + return [Path(s["path"]) for s in shortcuts if s.get("source") == source and "path" in s] -def delete_paths(paths: list[str]) -> list[Path]: +def delete_paths(paths: list[Path]) -> list[Path]: """Delete files or directories at given paths, warning if not found. Handles both files (.lnk, .desktop) and directories (.app bundles on macOS). """ deleted = [] - for path_str in paths: - path = Path(path_str) + for path in paths: if path.is_file(): log.debug("Removing %s", path) unlink(path) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index ef6e1b4d..cbe623cc 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -2,6 +2,7 @@ import json import logging +from pathlib import Path import pytest @@ -241,7 +242,7 @@ def test_returns_paths_for_source(self, tmp_path): }, ) paths = get_recorded_paths(tmp_path, "foo.json") - assert paths == ["/path/to/foo.lnk", "/path/to/bar.lnk"] + assert paths == [Path("/path/to/foo.lnk"), Path("/path/to/bar.lnk")] def test_returns_empty_list_when_no_matches(self, tmp_path): """Test that empty list is returned when no shortcuts match source.""" @@ -269,7 +270,7 @@ def test_skips_entries_missing_path_key(self, tmp_path): }, ) paths = get_recorded_paths(tmp_path, "foo.json") - assert paths == ["/valid/path.lnk"] + assert paths == [Path("/valid/path.lnk")] class TestDeletePaths: @@ -282,7 +283,7 @@ def test_deletes_existing_files(self, tmp_path): file1.touch() file2.touch() - deleted = delete_paths([str(file1), str(file2)]) + deleted = delete_paths([file1, file2]) assert not file1.exists() assert not file2.exists() @@ -295,7 +296,7 @@ def test_deletes_directories(self, tmp_path): (app_dir / "Contents").mkdir() (app_dir / "Contents" / "Info.plist").touch() - deleted = delete_paths([str(app_dir)]) + deleted = delete_paths([app_dir]) assert not app_dir.exists() assert len(deleted) == 1 @@ -305,7 +306,7 @@ def test_warns_on_missing_path(self, tmp_path, caplog): missing = tmp_path / "nonexistent.lnk" with caplog.at_level(logging.WARNING): - deleted = delete_paths([str(missing)]) + deleted = delete_paths([missing]) assert len(deleted) == 0 assert "Shortcut not found at expected location" in caplog.text @@ -315,9 +316,8 @@ def test_warns_on_missing_path(self, tmp_path, caplog): class TestRemoveUsesTomlPaths: """Tests for TOML-based shortcut removal.""" - def test_remove_uses_recorded_paths(self, tmp_path, monkeypatch): + def test_remove_uses_recorded_paths(self, tmp_path): """Test that remove() deletes files at recorded TOML paths, not computed paths.""" - monkeypatch.delenv("MENUINST_DISTRIBUTION_NAME", raising=False) (tmp_path / ".nonadmin").touch() menu_dir = tmp_path / "Menu" menu_dir.mkdir() From b898ac86bbf8f0dd7afa6e13b4244ee08cc79ef4 Mon Sep 17 00:00:00 2001 From: Robin Date: Mon, 18 May 2026 09:36:00 -0400 Subject: [PATCH 10/10] Improve existing test --- tests/test_metadata.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index cbe623cc..b6733313 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -10,6 +10,7 @@ _install_adapter, delete_paths, get_recorded_paths, + install, record_shortcuts, remove, remove_shortcut_records, @@ -378,5 +379,19 @@ def test_remove_falls_back_when_no_toml_entries(self, tmp_path, monkeypatch): ) ) - # Verifying no exception is raised + # Install shortcuts + paths = install(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) + shortcut_paths = [ + p for p in paths if Path(p).suffix in (".lnk", ".desktop") or Path(p).is_dir() + ] + assert shortcut_paths, "Expected at least one shortcut to be created" + + # Clear TOML to simulate legacy install (pre-TOML tracking) + write_menuinst_toml(tmp_path, {}) + + # Remove should use fallback computed paths remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) + + # Verify shortcuts were removed + for p in shortcut_paths: + assert not Path(p).exists(), f"Shortcut should have been removed: {p}"