CLI: Improve

docs/source/cli.rst

@@ -71,11 +71,19 @@ Build JavaScript application
 ----------------------------
 
 The ``build`` subcommand invokes ``npm run build``, optionally accepting
-a target directory.
+a target directory. By default, it uses the current working directory,
+where it expects a regular NPM ``package.json`` file.
 
 .. code-block:: shell
 
-    responder build [<target>]
+    responder build
+
+When specifying a target directory, responder will change to that
+directory beforehand.
+
+.. code-block:: shell
+
+    responder build /path/to/project
 
 
 .. _helloworld.py: https://github.com/kennethreitz/responder/blob/main/examples/helloworld.py

responder/util/cmd.py

@@ -2,8 +2,32 @@
 import shutil
 import subprocess
 import threading
+from pathlib import Path
 
-RESPONDER_BIN = shutil.which("responder")
+
+class ResponderProgram:
+    """
+    Provide full path to the `responder` program.
+    """
+
+    @staticmethod
+    def path():
+        program = shutil.which("responder")
+        if program is None:
+            raise RuntimeError("Could not find 'responder' executable in PATH")
+        return program
+
+    @classmethod
+    def build(cls, path: Path):
+        """
+        Invoke `responder build`, and return stdout and stderr.
+        """
+        command = [
+            cls.path(),
+            "build",
+            str(path),
+        ]
+        return subprocess.call(command)
 
 
 class ResponderServer(threading.Thread):
@@ -13,13 +37,28 @@ class ResponderServer(threading.Thread):
 
     def __init__(self, target: str, port: int = 5042, limit_max_requests: int = None):
         super().__init__()
+
+        # Sanity checks, as suggested by @coderabbitai. Thanks.
+        if not target or not isinstance(target, str):
+            raise ValueError("Target must be a non-empty string")
+        if not isinstance(port, int) or port < 1:
+            raise ValueError("Port must be a positive integer")
+        if limit_max_requests is not None and (
+            not isinstance(limit_max_requests, int) or limit_max_requests < 1
+        ):
+            raise ValueError("limit_max_requests must be a positive integer if specified")
+
         self.target = target
         self.port = port
         self.limit_max_requests = limit_max_requests
 
+        # Allow the thread to be terminated when the main program exits.
+        self.process = None
+        self.daemon = True
+
     def run(self):
         command = [
-            RESPONDER_BIN,
+            ResponderProgram.path(),
             "run",
             self.target,
         ]
@@ -28,4 +67,21 @@ def run(self):
         env = {}
         if self.port is not None:
             env = {"PORT": str(self.port)}
-        subprocess.check_call(command, env=env)
+
+        self.process = subprocess.Popen(
+            command,
+            env=env,
+            universal_newlines=True,
+        )
+        self.process.wait()
+
+    def stop(self):
+        """
+        Gracefully stop the process.
+        """
+        if self.process and self.process.poll() is None:
+            self.process.terminate()
+            try:
+                self.process.wait(timeout=5)  # Wait up to 5 seconds for graceful shutdown
+            except subprocess.TimeoutExpired:
+                self.process.kill()  # Force kill if not terminated

responder/util/python.py

@@ -9,6 +9,14 @@ def load_target(target: str, default_property: str = "api", method: str = "run")
     Load Python code from file or module.
     """
 
+    # Sanity checks, as suggested by @coderabbitai. Thanks.
+    if not target or ":" in target and len(target.split(":")) != 2:
+        raise ValueError(
+            "Invalid target format. "
+            "Use '<module>', '<module>:<property>', "
+            "'/path/to/app.py', or '/path/to/app.py:<property>"
+        )
+
     # Decode launch target location address.
     # Module: acme.app:foo
     # Path:   /path/to/acme/app.py:foo
@@ -21,8 +29,9 @@ def load_target(target: str, default_property: str = "api", method: str = "run")
 
     # Import launch target. Treat input location either as a filesystem path
     # (/path/to/acme/app.py), or as a module address specification (acme.app).
-    if Path(target).exists():
-        app = load_file_module(target)
+    path = Path(target)
+    if path.is_file():
+        app = load_file_module(path)
     else:
         app = importlib.import_module(target)
 
@@ -36,19 +45,22 @@ def load_target(target: str, default_property: str = "api", method: str = "run")
             )
         if not hasattr(api, method):
             raise AttributeError(
-                f"{msg_prefix}: API instance '{prop}' has no method 'run'"
+                f"{msg_prefix}: API instance '{prop}' has no method '{method}'"
             )
         return api
     except ImportError as ex:
         raise ImportError(f"{msg_prefix}: {ex}") from ex
 
 
-def load_file_module(module: str):
+def load_file_module(path: Path):
     """
     Load Python file as Python module.
     """
-    spec = importlib.util.spec_from_file_location("__app__", module)
+    name = f"__{path.stem}__"
+    spec = importlib.util.spec_from_file_location(name, path)
+    if spec is None:
+        raise ImportError(f"Failed loading module from file: {path}")
     app = importlib.util.module_from_spec(spec)
-    sys.modules["__app__"] = app
+    sys.modules[name] = app
     spec.loader.exec_module(app)
     return app

tests/test_cli.py

@@ -1,13 +1,15 @@
 # ruff: noqa: S603, S607
 import json
 import subprocess
-import time
+from pathlib import Path
 
 import pytest
 import requests
+from _pytest.capture import CaptureFixture
 
 from responder.__version__ import __version__
-from responder.util.cmd import RESPONDER_BIN, ResponderServer
+from responder.util.cmd import ResponderProgram, ResponderServer
+from tests.util import random_port, wait_server
 
 pytest.importorskip("docopt", reason="docopt-ng package not installed")
 
@@ -25,28 +27,56 @@ def test_cli_version(capfd):
     assert stdout == __version__
 
 
-def test_cli_build(capfd, tmp_path):
+def responder_build(path: Path, capfd: CaptureFixture):
+    ResponderProgram.build(path=path)
+    output = capfd.readouterr()
+
+    stdout = output.out.strip()
+    stderr = output.err.strip()
+
+    return stdout, stderr
+
+
+def test_cli_build_success(capfd, tmp_path):
     """
     Verify that `responder build` works as expected.
     """
 
     # Temporary surrogate `package.json` file.
-    package_json = {"scripts": {"build": "echo foobar"}}
+    package_json = {"scripts": {"build": "echo Hotzenplotz"}}
     package_json_file = tmp_path / "package.json"
     package_json_file.write_text(json.dumps(package_json))
 
     # Invoke `responder build`.
-    command = [
-        RESPONDER_BIN,
-        "build",
-        str(tmp_path),
-    ]
-    subprocess.check_call(command)
+    stdout, stderr = responder_build(tmp_path, capfd)
+    assert "Hotzenplotz" in stdout
 
-    output = capfd.readouterr()
 
-    stdout = output.out.strip()
-    assert "foobar" in stdout
+def test_cli_build_missing_package_json(capfd, tmp_path):
+    """
+    Verify `responder build`, while `package.json` file is missing.
+    """
+
+    # Invoke `responder build`.
+    stdout, stderr = responder_build(tmp_path, capfd)
+    assert "npm error code ENOENT" in stderr
+    assert "Could not read package.json" in stderr
+    assert "Error: ENOENT: no such file or directory" in stderr
+
+
+def test_cli_build_invalid_package_json(capfd, tmp_path):
+    """
+    Verify `responder build` using an invalid `package.json` file.
+    """
+
+    # Temporary surrogate `package.json` file.
+    package_json_file = tmp_path / "package.json"
+    package_json_file.write_text("foobar")
+
+    # Invoke `responder build`.
+    stdout, stderr = responder_build(tmp_path, capfd)
+    assert "npm error code EJSONPARSE" in stderr
+    assert '"foobar" is not valid JSON' in stderr
 
 
 def test_cli_run(capfd):
@@ -58,14 +88,15 @@ def test_cli_run(capfd):
     # Start a Responder service instance in the background, using its CLI.
     # Make it terminate itself after serving one HTTP request.
     server = ResponderServer(
-        target="examples/helloworld.py", port=9876, limit_max_requests=1
+        target="examples/helloworld.py", port=random_port(), limit_max_requests=1
     )
-    server.start()
-    time.sleep(0.5)
-
-    response = requests.get(f"http://127.0.0.1:{server.port}/hello", timeout=1)
-    assert "hello, world!" == response.text
-    server.join()
+    try:
+        server.start()
+        wait_server(server.port)
+        response = requests.get(f"http://127.0.0.1:{server.port}/hello", timeout=1)
+        assert "hello, world!" == response.text
+    finally:
+        server.join()
 
     output = capfd.readouterr()
 

tests/util.py

@@ -0,0 +1,30 @@
+import time
+
+import requests
+
+
+def random_port():
+    """
+    Return a random available port.
+    """
+    import socket
+
+    sock = socket.socket()
+    sock.bind(("", 0))
+    port = sock.getsockname()[1]
+    sock.close()
+    return port
+
+
+def wait_server(port: int):
+    """
+    Wait for server to be ready.
+    """
+    for _ in range(20):  # 2 second timeout
+        try:
+            requests.get(f"http://127.0.0.1:{port}/", timeout=0.1)
+            break
+        except requests.exceptions.RequestException:
+            time.sleep(0.1)
+    else:
+        raise RuntimeError("Server failed to start")