From 3fd6ec9aef27639013d963ba146c26d2a0a9a1be Mon Sep 17 00:00:00 2001
From: Dan LaManna <dan.lamanna@kitware.com>
Date: Thu, 19 Jun 2025 03:39:38 -0400
Subject: [PATCH] Refactor the permissions test to run on mac and linux

---
 isic_cli/cli/types.py      |  8 +++-----
 tests/test_cli_metadata.py | 21 ++++++++-------------
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/isic_cli/cli/types.py b/isic_cli/cli/types.py
index 504e258..38578f2 100644
--- a/isic_cli/cli/types.py
+++ b/isic_cli/cli/types.py
@@ -115,11 +115,9 @@ def convert(self, value, param, ctx):
                 value.parent.mkdir(parents=True, exist_ok=True)
                 with value.open("w", newline="", encoding="utf8"):
                     pass
-            except PermissionError:
-                self.fail(f"Permission denied - cannot write to '{value}'.", param, ctx)
-            except OSError as e:
-                # this is a general catch-all for weirder issues like a read only filesystem,
+            except (PermissionError, OSError):
+                # a user can end up here from lacking permissions, a read only filesystem,
                 # filenames that are too long or have invalid chars, etc.
-                self.fail(f"Cannot write to '{value}'. {e!s}", param, ctx)
+                self.fail(f"Permission denied - cannot write to '{value}'.", param, ctx)
 
         return value
diff --git a/tests/test_cli_metadata.py b/tests/test_cli_metadata.py
index a8e8e27..25b0b9a 100644
--- a/tests/test_cli_metadata.py
+++ b/tests/test_cli_metadata.py
@@ -88,12 +88,15 @@ def test_metadata_download_file(cli_runner):
     assert re.search(r"ISIC_0000000.*Foo.*CC-0.*melanoma.*male", output), output
 
 
-@pytest.mark.skipif(
-    sys.platform in ["win32", "darwin"], reason="Windows and macOS don't support this test"
-)
 @pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem")
-def test_metadata_download_file_no_write(cli_run):
-    result = cli_run(["metadata", "download", "-o", "/metadata.csv"])
+@pytest.mark.parametrize(
+    "output_file", ["/metadata.csv", f"{'1' * 255}.csv"], ids=["no_permissions", "bad_filename"]
+)
+def test_metadata_download_permission_denied(cli_run, output_file):
+    if sys.platform == "win32" and output_file == "/metadata.csv":
+        pytest.skip("Windows doesn't support this test")
+
+    result = cli_run(["metadata", "download", "-o", output_file])
     # it's important that the exit code is 2 and not 1, because the key constraint of this
     # functionality is that the user gets the error message before spending their time
     # downloading the data. exit code 2 is for usage errors with click.
@@ -101,14 +104,6 @@ def test_metadata_download_file_no_write(cli_run):
     assert re.search(r"Permission denied", result.output), result.output
 
 
-@pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem")
-def test_metadata_download_file_bad_filename(cli_run):
-    result = cli_run(["metadata", "download", "-o", f"{'1' * 255}.csv"])
-    # see comment in test_metadata_download_file_no_write for why exit code is 2
-    assert result.exit_code == 2, result.exception
-    assert re.search(r"Cannot write to", result.output), result.output
-
-
 @pytest.mark.usefixtures("_mock_image_metadata")
 @pytest.mark.parametrize(
     "cli_runner",