From f88c2bfe59f1e098b10e12f80ec817bca06ff241 Mon Sep 17 00:00:00 2001
From: Bart Feenstra <bart@mynameisbart.com>
Date: Wed, 27 Nov 2024 13:45:49 +0000
Subject: [PATCH] Improve the npm API's error handling and test coverage
 (#2227)

---
 betty/_npm.py            | 23 +++++++++++++++--------
 betty/tests/test__npm.py | 36 +++++++++++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/betty/_npm.py b/betty/_npm.py
index 151922148..a463406f3 100644
--- a/betty/_npm.py
+++ b/betty/_npm.py
@@ -8,27 +8,29 @@
 
 import logging
 import sys
+from subprocess import CalledProcessError
 from typing import Sequence, TYPE_CHECKING
 
+from betty import subprocess
 from betty.asyncio import wait_to_thread
 from betty.error import UserFacingError
 from betty.locale import Str, DEFAULT_LOCALIZER
 from betty.requirement import Requirement
-from betty.subprocess import run_process
 
 if TYPE_CHECKING:
     from pathlib import Path
     from asyncio import subprocess as aiosubprocess
 
-
-_NPM_UNAVAILABLE_MESSAGE = Str._(
+_NPM_SUMMARY_AVAILABLE = Str._("`npm` is available")
+_NPM_SUMMARY_UNAVAILABLE = Str._("`npm` is not available")
+_NPM_DETAILS = Str._(
     "npm (https://www.npmjs.com/) must be available for features that require Node.js packages to be installed. Ensure that the `npm` executable is available in your `PATH`."
 )
 
 
 class NpmUnavailable(UserFacingError, RuntimeError):
     def __init__(self):
-        super().__init__(_NPM_UNAVAILABLE_MESSAGE)
+        super().__init__(_NPM_DETAILS)
 
 
 async def npm(
@@ -39,7 +41,7 @@ async def npm(
     Run an npm command.
     """
     try:
-        return await run_process(
+        return await subprocess.run_process(
             ["npm", *arguments],
             cwd=cwd,
             # Use a shell on Windows so subprocess can find the executables it needs (see
@@ -55,7 +57,7 @@ def __init__(self):
         super().__init__()
         self._met: bool
         self._summary: Str
-        self._details = _NPM_UNAVAILABLE_MESSAGE
+        self._details = _NPM_DETAILS
 
     def _check(self) -> None:
         if hasattr(self, "_met"):
@@ -64,10 +66,15 @@ def _check(self) -> None:
             wait_to_thread(npm(["--version"]))
         except NpmUnavailable:
             self._met = False
-            self._summary = Str._("`npm` is not available")
+            self._summary = _NPM_SUMMARY_UNAVAILABLE
+        except CalledProcessError as error:
+            logging.getLogger(__name__).exception(error)
+            logging.getLogger(__name__).debug(_NPM_DETAILS.localize(DEFAULT_LOCALIZER))
+            self._met = False
+            self._summary = _NPM_SUMMARY_UNAVAILABLE
         else:
             self._met = True
-            self._summary = Str._("`npm` is available")
+            self._summary = _NPM_SUMMARY_AVAILABLE
         finally:
             logging.getLogger(__name__).debug(self._summary.localize(DEFAULT_LOCALIZER))
 
diff --git a/betty/tests/test__npm.py b/betty/tests/test__npm.py
index 5aa17111b..f6d3319e8 100644
--- a/betty/tests/test__npm.py
+++ b/betty/tests/test__npm.py
@@ -1,15 +1,41 @@
+from pathlib import Path
+from subprocess import CalledProcessError
+
+import pytest
 from pytest_mock import MockerFixture
 
-from betty._npm import NpmRequirement, NpmUnavailable
+from betty._npm import NpmRequirement, NpmUnavailable, npm
+
+
+class TestNpm:
+    async def test(self) -> None:
+        await npm(["--version"])
+
+    async def test_command_not_found(self, mocker: MockerFixture) -> None:
+        mocker.patch("betty.subprocess.run_process", side_effect=FileNotFoundError)
+        with pytest.raises(NpmUnavailable):
+            await npm(["--version"])
+
+    async def test_command_error(self, mocker: MockerFixture, tmp_path: Path) -> None:
+        mocker.patch(
+            "betty.subprocess.run_process",
+            side_effect=CalledProcessError(1, "", "", ""),
+        )
+        with pytest.raises(CalledProcessError):
+            await npm(["--version"])
 
 
 class TestNpmRequirement:
-    async def test_check_met(self) -> None:
+    def test_is_met(self) -> None:
         sut = NpmRequirement()
         assert sut.is_met()
 
-    async def test_check_unmet(self, mocker: MockerFixture) -> None:
-        m_npm = mocker.patch("betty._npm.npm")
-        m_npm.side_effect = NpmUnavailable()
+    def test_is_met_with_command_not_found(self, mocker: MockerFixture) -> None:
+        mocker.patch("betty._npm.npm", side_effect=NpmUnavailable)
+        sut = NpmRequirement()
+        assert not sut.is_met()
+
+    def test_is_met_with_command_error(self, mocker: MockerFixture) -> None:
+        mocker.patch("betty._npm.npm", side_effect=CalledProcessError(1, "", "", ""))
         sut = NpmRequirement()
         assert not sut.is_met()