diff --git a/docs/Implementation-specification.md b/docs/Implementation-specification.md index 0149cfa..a7bae6d 100644 --- a/docs/Implementation-specification.md +++ b/docs/Implementation-specification.md @@ -836,16 +836,23 @@ Development: ### Installation Methods -#### From PyPI +**Note:** PWA Forge is not yet published to PyPI. Install from source: + +#### From Source (Recommended) ```bash -pip install pwa-forge +git clone https://github.com/bigr/pwa_forge.git +cd pwa_forge +pip install -e . ``` -#### From Source +#### Via pip with git ```bash -git clone https://github.com/yourusername/pwa-forge.git -cd pwa-forge -pip install -e . +pip install git+https://github.com/bigr/pwa_forge.git +``` + +#### From PyPI (Future) +```bash +pip install pwa-forge # Not yet available ``` #### System Package (Future) @@ -2661,7 +2668,7 @@ Examples: pwa-forge list --verbose pwa-forge audit chatgpt --fix - For more help: https://github.com/yourusername/pwa-forge + For more help: https://github.com/bigr/pwa_forge ``` ### Add Command Help diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index f7ffc78..61c50f8 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -786,7 +786,7 @@ If your issue isn't covered here: ``` 4. **Search existing issues:** - - GitHub Issues: https://github.com/yourusername/pwa-forge/issues + - GitHub Issues: https://github.com/bigr/pwa_forge/issues 5. **Create a bug report:** Include: diff --git a/docs/USAGE.md b/docs/USAGE.md index 2feb462..1d80881 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -24,14 +24,16 @@ Complete guide to using PWA Forge for managing Progressive Web Apps on Linux. ### Installation -```bash -# Install from PyPI (when published) -pip install pwa-forge +**Note:** PWA Forge is not yet published to PyPI. Please install from source: -# Or install from source -git clone https://github.com/yourusername/pwa-forge.git -cd pwa-forge +```bash +# Install from source +git clone https://github.com/bigr/pwa_forge.git +cd pwa_forge pip install -e . + +# Or install directly via pip with git +pip install git+https://github.com/bigr/pwa_forge.git ``` ### Create Your First PWA diff --git a/docs/testing/report1.pdf b/docs/testing/report1.pdf new file mode 100644 index 0000000..91b1c29 Binary files /dev/null and b/docs/testing/report1.pdf differ diff --git a/src/pwa_forge/cli.py b/src/pwa_forge/cli.py index 37769c6..556360f 100644 --- a/src/pwa_forge/cli.py +++ b/src/pwa_forge/cli.py @@ -68,6 +68,10 @@ def cli(ctx: click.Context, verbose: int, quiet: bool, no_color: bool) -> None: pwa-forge list pwa-forge remove chatgpt """ + # Validate mutually exclusive options + if quiet and verbose > 0: + raise click.UsageError("--quiet and --verbose are mutually exclusive") + # Load configuration config = load_config() diff --git a/src/pwa_forge/commands/add.py b/src/pwa_forge/commands/add.py index 230508e..54554a1 100644 --- a/src/pwa_forge/commands/add.py +++ b/src/pwa_forge/commands/add.py @@ -12,6 +12,7 @@ from pwa_forge.registry import Registry from pwa_forge.templates import render_template from pwa_forge.validation import ( + ValidationStatus, extract_name_from_url, generate_id, generate_wm_class, @@ -65,10 +66,10 @@ def add_app( logger.info(f"Adding PWA for URL: {url}") # Validate URL - is_valid, message = validate_url(url, verify=False) + is_valid, status, message = validate_url(url, verify=False) if not is_valid: raise AddCommandError(f"Invalid URL: {message}") - if "Warning" in message: + if status == ValidationStatus.WARNING: logger.warning(message) # Determine app name @@ -109,8 +110,14 @@ def add_app( # Handle icon icon_path = _handle_icon(icon, app_id, config.icons_dir, dry_run) if icon else None - # Get browser executable - browser_exec = _get_browser_executable(browser, config) + # Get browser executable (skip in dry-run to allow testing without browsers installed) + if not dry_run: + browser_exec = _get_browser_executable(browser, config) + else: + # In dry-run, use placeholder path that won't be written + browser_exec_str = f"/usr/bin/{browser}" + logger.info(f"[DRY-RUN] Would use browser: {browser_exec_str}") + browser_exec = Path(browser_exec_str) # Parse chrome flags parsed_flags = _parse_chrome_flags(chrome_flags) if chrome_flags else {} @@ -286,6 +293,14 @@ def _get_browser_executable(browser: str, config: Config) -> Path: "edge": ["/usr/bin/microsoft-edge-stable", "/usr/bin/microsoft-edge"], } + # Map browser names to common executable names for shutil.which() + browser_executables = { + "chrome": ["google-chrome-stable", "google-chrome"], + "chromium": ["chromium-browser", "chromium"], + "firefox": ["firefox"], + "edge": ["microsoft-edge-stable", "microsoft-edge"], + } + # Try configured path first if hasattr(config.browsers, browser): browser_path = getattr(config.browsers, browser) @@ -300,6 +315,13 @@ def _get_browser_executable(browser: str, config: Config) -> Path: logger.debug(f"Found browser at: {path}") return path + # Fallback: search in PATH using shutil.which() + for executable_name in browser_executables.get(browser, []): + which_path = shutil.which(executable_name) + if which_path: + logger.debug(f"Found browser in PATH: {which_path}") + return Path(which_path) + raise AddCommandError(f"Browser '{browser}' not found. Please install it or specify the path in config.") diff --git a/src/pwa_forge/registry.py b/src/pwa_forge/registry.py index cb7b77c..a028f69 100644 --- a/src/pwa_forge/registry.py +++ b/src/pwa_forge/registry.py @@ -2,9 +2,9 @@ from __future__ import annotations -import fcntl import json import logging +import sys from collections.abc import Iterator from contextlib import contextmanager from datetime import datetime @@ -13,6 +13,12 @@ logger = logging.getLogger(__name__) +# Platform-specific imports for file locking +if sys.platform == "win32": + import msvcrt +else: + import fcntl + class RegistryError(Exception): """Base exception for registry operations.""" @@ -47,17 +53,31 @@ def __init__(self, registry_path: Path) -> None: def _lock(self) -> Iterator[None]: """Acquire exclusive lock on registry file. + Uses platform-specific locking mechanisms: + - Unix/Linux/macOS: fcntl.flock + - Windows: msvcrt.locking + Yields: None - context is locked for the duration of the block. """ self._lock_path.parent.mkdir(parents=True, exist_ok=True) with open(self._lock_path, "w") as lock_file: try: - fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + if sys.platform == "win32": + # Windows: lock the entire file + msvcrt.locking(lock_file.fileno(), msvcrt.LK_LOCK, 1) + else: + # Unix-like systems: use fcntl + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) logger.debug(f"Acquired lock on {self._lock_path}") yield finally: - fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + if sys.platform == "win32": + # Windows: unlock the file + msvcrt.locking(lock_file.fileno(), msvcrt.LK_UNLCK, 1) + else: + # Unix-like systems: unlock + fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) logger.debug(f"Released lock on {self._lock_path}") def _read(self) -> dict[str, Any]: @@ -138,18 +158,34 @@ def add_app(self, app_data: dict[str, Any]) -> None: app_id = app_data["id"] - # Check if app already exists - try: - self.get_app(app_id) - raise AppExistsError(f"PWA '{app_id}' already exists in registry") - except AppNotFoundError: - pass # This is expected - - # Add app to registry - data = self._read() - data["apps"].append(app_data) - self._write(data) - logger.info(f"Added app to registry: {app_id}") + # Atomic read-check-write within a single lock to prevent race conditions + with self._lock(): + # Read current registry + if not self.registry_path.exists(): + data: dict[str, Any] = {"version": 1, "apps": [], "handlers": []} + else: + try: + content = self.registry_path.read_text() + data = json.loads(content) + except json.JSONDecodeError as e: + logger.error(f"Invalid JSON in registry file: {e}") + raise RegistryError(f"Corrupted registry file: {e}") from e + + # Check if app already exists + apps: list[dict[str, Any]] = data.get("apps", []) + for app in apps: + if app.get("id") == app_id: + raise AppExistsError(f"PWA '{app_id}' already exists in registry") + + # Add app to registry + apps.append(app_data) + data["apps"] = apps + + # Write atomically + self.registry_path.parent.mkdir(parents=True, exist_ok=True) + content = json.dumps(data, indent=2) + self.registry_path.write_text(content) + logger.info(f"Added app to registry: {app_id}") def update_app(self, app_id: str, updates: dict[str, Any]) -> None: """Update an existing PWA entry. diff --git a/src/pwa_forge/validation.py b/src/pwa_forge/validation.py index 7aae712..643126f 100644 --- a/src/pwa_forge/validation.py +++ b/src/pwa_forge/validation.py @@ -11,7 +11,15 @@ logger = logging.getLogger(__name__) -def validate_url(url: str, verify: bool = False, timeout: int = 5) -> tuple[bool, str]: +class ValidationStatus: + """URL validation result status codes.""" + + OK = "OK" + WARNING = "WARNING" + ERROR = "ERROR" + + +def validate_url(url: str, verify: bool = False, timeout: int = 5) -> tuple[bool, str, str]: """Validate URL format and optionally check accessibility. Args: @@ -20,30 +28,33 @@ def validate_url(url: str, verify: bool = False, timeout: int = 5) -> tuple[bool timeout: Connection timeout in seconds for verification. Returns: - Tuple of (is_valid, message) where message is "OK" or error description. + Tuple of (is_valid, status, message) where: + - is_valid: True if URL is valid (may have warnings) + - status: One of ValidationStatus.OK, ValidationStatus.WARNING, ValidationStatus.ERROR + - message: Description of the validation result """ # Parse URL try: parsed = urlparse(url) except Exception as e: logger.debug(f"URL parsing failed for '{url}': {e}") - return False, f"Invalid URL format: {e}" + return False, ValidationStatus.ERROR, f"Invalid URL format: {e}" # Check scheme if parsed.scheme not in ("http", "https"): logger.debug(f"Invalid URL scheme: {parsed.scheme}") - return False, "URL must use http:// or https://" + return False, ValidationStatus.ERROR, "URL must use http:// or https://" # Check host if not parsed.netloc: logger.debug("URL missing hostname") - return False, "URL must include a hostname" + return False, ValidationStatus.ERROR, "URL must include a hostname" # Warn about localhost hostname = parsed.netloc.split(":")[0] if ":" in parsed.netloc else parsed.netloc if hostname in ("localhost", "127.0.0.1", "::1"): logger.warning(f"Localhost URL detected: {url}") - return True, "Warning: localhost URLs won't work from system launcher" + return True, ValidationStatus.WARNING, "localhost URLs won't work from system launcher" # Optional connectivity check if verify: @@ -52,13 +63,13 @@ def validate_url(url: str, verify: bool = False, timeout: int = 5) -> tuple[bool response = requests.head(url, timeout=timeout, allow_redirects=True) if response.status_code >= 400: logger.warning(f"URL returned HTTP {response.status_code}: {url}") - return False, f"URL returned HTTP {response.status_code}" + return False, ValidationStatus.ERROR, f"URL returned HTTP {response.status_code}" except requests.RequestException as e: logger.warning(f"URL not accessible: {url} - {e}") - return False, f"URL not accessible: {e}" + return False, ValidationStatus.ERROR, f"URL not accessible: {e}" logger.debug(f"URL validation passed: {url}") - return True, "OK" + return True, ValidationStatus.OK, "OK" def generate_id(name: str) -> str: diff --git a/tests/e2e/test_e2e_system.py b/tests/e2e/test_e2e_system.py index 99e8d4a..925bc33 100644 --- a/tests/e2e/test_e2e_system.py +++ b/tests/e2e/test_e2e_system.py @@ -146,6 +146,9 @@ def mock_exists(self: Path) -> bool: monkeypatch.setattr(Path, "exists", mock_exists) + # Mock shutil.which to return None (browser not in PATH) + monkeypatch.setattr("shutil.which", lambda _: None) + # Add PWA should fail from pwa_forge.commands.add import AddCommandError diff --git a/tests/unit/test_add_browser_detection.py b/tests/unit/test_add_browser_detection.py new file mode 100644 index 0000000..8cb2dd9 --- /dev/null +++ b/tests/unit/test_add_browser_detection.py @@ -0,0 +1,184 @@ +"""Unit tests for browser detection in add command.""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import patch + +import pytest +from pwa_forge.commands.add import AddCommandError, _get_browser_executable +from pwa_forge.config import Config + + +class TestBrowserDetection: + """Test browser executable detection logic.""" + + def test_browser_found_in_config(self) -> None: + """Test browser found via configured path.""" + config = Config() + + # Mock the browser path to a temporary file + with ( + patch.object(config.browsers, "firefox", "/usr/bin/firefox"), + patch("pathlib.Path.exists", return_value=True), + ): + result = _get_browser_executable("firefox", config) + assert result == Path("/usr/bin/firefox") + + def test_browser_found_in_known_paths(self) -> None: + """Test browser found in hard-coded known paths.""" + config = Config() + + # Make config path not exist, but known path exist + with patch("pathlib.Path.exists") as mock_exists: + # First call (config path) returns False, second call (known path) returns True + mock_exists.side_effect = [False, True] + + result = _get_browser_executable("firefox", config) + assert result == Path("/usr/bin/firefox") + + def test_browser_found_via_which(self) -> None: + """Test browser found via shutil.which() fallback.""" + config = Config() + + # All hard-coded paths fail, but shutil.which() succeeds + with ( + patch("pathlib.Path.exists", return_value=False), + patch("shutil.which", return_value="/snap/bin/firefox"), + ): + result = _get_browser_executable("firefox", config) + assert result == Path("/snap/bin/firefox") + + def test_browser_not_found_raises_error(self) -> None: + """Test that missing browser raises AddCommandError.""" + config = Config() + + # All detection methods fail + with ( + patch("pathlib.Path.exists", return_value=False), + patch("shutil.which", return_value=None), + pytest.raises(AddCommandError, match="Browser 'firefox' not found"), + ): + _get_browser_executable("firefox", config) + + def test_chrome_found_with_multiple_names(self) -> None: + """Test chrome can be found under various executable names.""" + config = Config() + + # Configured and known paths don't exist + with ( + patch("pathlib.Path.exists", return_value=False), + patch("shutil.which") as mock_which, + ): + # shutil.which() tries google-chrome-stable first (fails), then google-chrome (succeeds) + mock_which.side_effect = [None, "/usr/local/bin/google-chrome"] + + result = _get_browser_executable("chrome", config) + assert result == Path("/usr/local/bin/google-chrome") + + # Verify it tried both names + assert mock_which.call_count == 2 + + def test_chromium_browser_executable_name(self) -> None: + """Test chromium with chromium-browser executable name.""" + config = Config() + + with ( + patch("pathlib.Path.exists", return_value=False), + patch("shutil.which") as mock_which, + ): + mock_which.side_effect = ["/usr/bin/chromium-browser", None] + + result = _get_browser_executable("chromium", config) + assert result == Path("/usr/bin/chromium-browser") + + def test_edge_browser_detection(self) -> None: + """Test Microsoft Edge browser detection.""" + config = Config() + + with ( + patch("pathlib.Path.exists", return_value=False), + patch("shutil.which", return_value="/opt/microsoft/msedge/microsoft-edge"), + ): + result = _get_browser_executable("edge", config) + assert result == Path("/opt/microsoft/msedge/microsoft-edge") + + +class TestDryRunBehavior: + """Test dry-run mode behavior.""" + + def test_dry_run_does_not_require_browser(self, tmp_path: Path) -> None: + """Test that dry-run mode works without browser installed.""" + from pwa_forge.commands.add import add_app + + config = Config() + config.directories.desktop = tmp_path / "applications" + config.directories.icons = tmp_path / "icons" + config.directories.wrappers = tmp_path / "wrappers" + config.directories.apps = tmp_path / "apps" + + # Create directories + for directory in [ + config.directories.desktop, + config.directories.icons, + config.directories.wrappers, + config.directories.apps, + ]: + directory.mkdir(parents=True, exist_ok=True) + + # Mock all browser detection to fail, and mock the data dir for registry + with ( + patch("pathlib.Path.exists", return_value=False), + patch("shutil.which", return_value=None), + patch("pwa_forge.utils.paths.get_app_data_dir", return_value=tmp_path / "data"), + ): + # This should NOT raise an error in dry-run mode + result = add_app( + url="https://example.com", + config=config, + name="Test App", + app_id="test-app", + browser="chrome", + dry_run=True, + ) + + # Verify result contains expected fields + assert result["id"] == "test-app" + assert result["name"] == "Test App" + assert result["browser"] == "chrome" + + def test_non_dry_run_requires_browser(self, tmp_path: Path) -> None: + """Test that non-dry-run mode requires browser to be found.""" + from pwa_forge.commands.add import AddCommandError, add_app + + config = Config() + config.directories.desktop = tmp_path / "applications" + config.directories.icons = tmp_path / "icons" + config.directories.wrappers = tmp_path / "wrappers" + config.directories.apps = tmp_path / "apps" + + # Create directories + for directory in [ + config.directories.desktop, + config.directories.icons, + config.directories.wrappers, + config.directories.apps, + ]: + directory.mkdir(parents=True, exist_ok=True) + + # Mock all browser detection to fail + with ( + patch("pathlib.Path.exists", return_value=False), + patch("shutil.which", return_value=None), + patch("pwa_forge.utils.paths.get_app_data_dir", return_value=tmp_path / "data"), + pytest.raises(AddCommandError, match="Browser 'chrome' not found"), + ): + # This SHOULD raise an error in non-dry-run mode + add_app( + url="https://example.com", + config=config, + name="Test App", + app_id="test-app-2", + browser="chrome", + dry_run=False, + ) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index fa982f8..a5e9066 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -260,6 +260,17 @@ def test_verbose_flag(self) -> None: result = runner.invoke(cli.cli, ["-vv", "version"]) assert result.exit_code == 0 + def test_quiet_and_verbose_mutually_exclusive(self) -> None: + """Test that --quiet and --verbose cannot be used together.""" + runner = CliRunner() + result = runner.invoke(cli.cli, ["--quiet", "--verbose", "version"]) + assert result.exit_code != 0 + assert "mutually exclusive" in result.output + + result = runner.invoke(cli.cli, ["-q", "-v", "version"]) + assert result.exit_code != 0 + assert "mutually exclusive" in result.output + class TestGenerateHandlerCommand: """Test generate-handler command.""" diff --git a/tests/unit/test_validation.py b/tests/unit/test_validation.py index faa582a..230f27b 100644 --- a/tests/unit/test_validation.py +++ b/tests/unit/test_validation.py @@ -6,6 +6,7 @@ import requests from pwa_forge.validation import ( + ValidationStatus, extract_name_from_url, generate_id, generate_wm_class, @@ -19,62 +20,72 @@ class TestValidateUrl: def test_valid_http_url(self) -> None: """Test validation of valid HTTP URL.""" - is_valid, message = validate_url("http://example.com") + is_valid, status, message = validate_url("http://example.com") assert is_valid is True + assert status == ValidationStatus.OK assert message == "OK" def test_valid_https_url(self) -> None: """Test validation of valid HTTPS URL.""" - is_valid, message = validate_url("https://example.com") + is_valid, status, message = validate_url("https://example.com") assert is_valid is True + assert status == ValidationStatus.OK assert message == "OK" def test_valid_url_with_path(self) -> None: """Test validation of URL with path.""" - is_valid, message = validate_url("https://example.com/path/to/app") + is_valid, status, message = validate_url("https://example.com/path/to/app") assert is_valid is True + assert status == ValidationStatus.OK assert message == "OK" def test_valid_url_with_query(self) -> None: """Test validation of URL with query parameters.""" - is_valid, message = validate_url("https://example.com?param=value") + is_valid, status, message = validate_url("https://example.com?param=value") assert is_valid is True + assert status == ValidationStatus.OK assert message == "OK" def test_invalid_scheme_ftp(self) -> None: """Test rejection of FTP URLs.""" - is_valid, message = validate_url("ftp://example.com") + is_valid, status, message = validate_url("ftp://example.com") assert is_valid is False + assert status == ValidationStatus.ERROR assert "http" in message.lower() def test_invalid_scheme_file(self) -> None: """Test rejection of file URLs.""" - is_valid, message = validate_url("file:///path/to/file") + is_valid, status, message = validate_url("file:///path/to/file") assert is_valid is False + assert status == ValidationStatus.ERROR assert "http" in message.lower() def test_missing_hostname(self) -> None: """Test rejection of URL without hostname.""" - is_valid, message = validate_url("https://") + is_valid, status, message = validate_url("https://") assert is_valid is False + assert status == ValidationStatus.ERROR assert "hostname" in message.lower() def test_localhost_url(self) -> None: """Test warning for localhost URLs.""" - is_valid, message = validate_url("http://localhost:8080") + is_valid, status, message = validate_url("http://localhost:8080") assert is_valid is True + assert status == ValidationStatus.WARNING assert "localhost" in message.lower() def test_localhost_ip(self) -> None: """Test warning for 127.0.0.1 URLs.""" - is_valid, message = validate_url("http://127.0.0.1:3000") + is_valid, status, message = validate_url("http://127.0.0.1:3000") assert is_valid is True + assert status == ValidationStatus.WARNING assert "localhost" in message.lower() def test_invalid_url_format(self) -> None: """Test rejection of malformed URLs.""" - is_valid, _ = validate_url("not a url") + is_valid, status, _ = validate_url("not a url") assert is_valid is False + assert status == ValidationStatus.ERROR def test_url_parsing_exception(self) -> None: """Test handling of URL parsing exceptions.""" @@ -85,24 +96,27 @@ def test_url_parsing_exception(self) -> None: with patch("pwa_forge.validation.urlparse") as mock_urlparse: mock_urlparse.side_effect = ValueError("Invalid URL") - is_valid, message = validate_url("http://invalid") + is_valid, status, message = validate_url("http://invalid") assert is_valid is False + assert status == ValidationStatus.ERROR assert "Invalid URL format" in message def test_url_connectivity_check_success(self) -> None: """Test URL connectivity check when verify=True.""" # This will try to make a real HTTP request # We can't control the network, so this may pass or fail depending on connectivity - is_valid, message = validate_url("https://httpbin.org/status/200", verify=True) + is_valid, status, message = validate_url("https://httpbin.org/status/200", verify=True) # Just check that it doesn't crash assert isinstance(is_valid, bool) + assert isinstance(status, str) assert isinstance(message, str) def test_url_connectivity_check_failure(self) -> None: """Test URL connectivity check failure.""" # Use a non-existent domain - is_valid, message = validate_url("https://this-domain-does-not-exist-12345.com", verify=True) + is_valid, status, message = validate_url("https://this-domain-does-not-exist-12345.com", verify=True) assert is_valid is False + assert status == ValidationStatus.ERROR assert "not accessible" in message def test_url_connectivity_check_http_error(self) -> None: @@ -111,16 +125,18 @@ def test_url_connectivity_check_http_error(self) -> None: mock_response = Mock() mock_response.status_code = 404 mock_head.return_value = mock_response - is_valid, message = validate_url("https://example.com/notfound", verify=True) + is_valid, status, message = validate_url("https://example.com/notfound", verify=True) assert is_valid is False + assert status == ValidationStatus.ERROR assert "404" in message def test_url_connectivity_check_request_exception(self) -> None: """Test URL connectivity check with request exception.""" with patch("pwa_forge.validation.requests.head") as mock_head: mock_head.side_effect = requests.RequestException("Connection timeout") - is_valid, message = validate_url("https://example.com", verify=True) + is_valid, status, message = validate_url("https://example.com", verify=True) assert is_valid is False + assert status == ValidationStatus.ERROR assert "not accessible" in message