mirror of
https://github.com/Second-Hand-Friends/kleinanzeigen-bot.git
synced 2026-03-12 02:31:45 +01:00
fix: improve Windows browser autodetection paths and diagnose fallback (#816)
## ℹ️ Description This pull request fixes Windows browser auto-detection failures reported by users where `diagnose`/startup could not find an installed browser even when Chrome or Edge were present in standard locations. It also makes diagnostics resilient when auto-detection fails by avoiding an assertion-driven abort and continuing with a clear failure log. - Link to the related issue(s): Issue #815 - Describe the motivation and context for this change. - Users reported `Installed browser could not be detected` on Windows despite having a browser installed. - The previous Windows candidate list used a mix of incomplete paths and direct `os.environ[...]` lookups that could raise when variables were missing. - The updated path candidates and ordering were aligned with common Windows install locations used by Playwright’s channel/executable resolution logic (Chrome/Edge under `LOCALAPPDATA`, `PROGRAMFILES`, and `PROGRAMFILES(X86)`). ## 📋 Changes Summary - Expanded Windows browser path candidates in `get_compatible_browser()` to include common Google Chrome and Microsoft Edge install paths, while keeping Chromium and PATH fallbacks. - Replaced unsafe direct env-var indexing with safe retrieval (`os.environ.get(...)`) and added a fallback derivation for `LOCALAPPDATA` via `USERPROFILE\\AppData\\Local` when needed. - Kept legacy Chrome path candidates (`...\\Chrome\\Application\\chrome.exe`) as compatibility fallback. - Updated diagnostics flow to catch browser auto-detection assertion failures and continue with `(fail) No compatible browser found` instead of crashing. - Added/updated unit tests to verify: - Windows detection for LocalAppData Chrome/Edge/Chromium paths. - Missing Windows env vars no longer cause key lookup failures and still surface the intended final detection assertion. - `diagnose_browser_issues()` handles auto-detection assertion failures without raising and logs the expected failure message. ### ⚙️ Type of Change Select the type(s) of change(s) included in this pull request: - [x] 🐞 Bug fix (non-breaking change which fixes an issue) ## ✅ Checklist Before requesting a review, confirm the following: - [x] I have reviewed my changes to ensure they meet the project's standards. - [x] I have tested my changes and ensured that all tests pass (`pdm run test`). - [x] I have formatted the code (`pdm run format`). - [x] I have verified that linting passes (`pdm run lint`). - [x] I have updated documentation where necessary. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Hardened Windows browser auto-detection: checks additional common installation locations for Chrome/Chromium/Edge and treats detection failures as non-fatal, allowing diagnostics to continue with fallback behavior and debug logging when no browser is found. * **Tests** * Expanded Windows detection tests to cover more path scenarios and added cases verifying failure-mode diagnostics and logging. * **Style** * Minor formatting tweak in default configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -582,7 +582,8 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904
|
|||||||
self.config_file_path,
|
self.config_file_path,
|
||||||
default_config,
|
default_config,
|
||||||
header = ("# yaml-language-server: $schema=https://raw.githubusercontent.com/Second-Hand-Friends/kleinanzeigen-bot/main/schemas/config.schema.json"),
|
header = ("# yaml-language-server: $schema=https://raw.githubusercontent.com/Second-Hand-Friends/kleinanzeigen-bot/main/schemas/config.schema.json"),
|
||||||
exclude = {"ad_defaults": {"description"}},
|
exclude = {
|
||||||
|
"ad_defaults": {"description"}},
|
||||||
)
|
)
|
||||||
|
|
||||||
def load_config(self) -> None:
|
def load_config(self) -> None:
|
||||||
|
|||||||
@@ -4,7 +4,7 @@
|
|||||||
import asyncio, enum, inspect, json, os, platform, secrets, shutil, subprocess, urllib.request # isort: skip # noqa: S404
|
import asyncio, enum, inspect, json, os, platform, secrets, shutil, subprocess, urllib.request # isort: skip # noqa: S404
|
||||||
from collections.abc import Awaitable, Callable, Coroutine, Iterable
|
from collections.abc import Awaitable, Callable, Coroutine, Iterable
|
||||||
from gettext import gettext as _
|
from gettext import gettext as _
|
||||||
from pathlib import Path
|
from pathlib import Path, PureWindowsPath
|
||||||
from typing import Any, Final, Optional, cast
|
from typing import Any, Final, Optional, cast
|
||||||
|
|
||||||
try:
|
try:
|
||||||
@@ -441,7 +441,11 @@ class WebScrapingMixin:
|
|||||||
else:
|
else:
|
||||||
LOG.error("(fail) Browser binary not found: %s", self.browser_config.binary_location)
|
LOG.error("(fail) Browser binary not found: %s", self.browser_config.binary_location)
|
||||||
else:
|
else:
|
||||||
|
try:
|
||||||
browser_path = self.get_compatible_browser()
|
browser_path = self.get_compatible_browser()
|
||||||
|
except AssertionError as exc:
|
||||||
|
LOG.debug("Browser auto-detection failed: %s", exc)
|
||||||
|
browser_path = None
|
||||||
if browser_path:
|
if browser_path:
|
||||||
LOG.info("(ok) Auto-detected browser: %s", browser_path)
|
LOG.info("(ok) Auto-detected browser: %s", browser_path)
|
||||||
# Set the binary location for Chrome version detection
|
# Set the binary location for Chrome version detection
|
||||||
@@ -579,15 +583,31 @@ class WebScrapingMixin:
|
|||||||
]
|
]
|
||||||
|
|
||||||
case "Windows":
|
case "Windows":
|
||||||
|
def win_path(*parts:str) -> str:
|
||||||
|
return str(PureWindowsPath(*parts))
|
||||||
|
|
||||||
|
program_files = os.environ.get("PROGRAMFILES", "C:\\Program Files")
|
||||||
|
program_files_x86 = os.environ.get("PROGRAMFILES(X86)", "C:\\Program Files (x86)")
|
||||||
|
local_app_data = os.environ.get("LOCALAPPDATA")
|
||||||
|
if not local_app_data:
|
||||||
|
user_profile = os.environ.get("USERPROFILE")
|
||||||
|
if user_profile:
|
||||||
|
local_app_data = win_path(user_profile, "AppData", "Local")
|
||||||
|
|
||||||
browser_paths = [
|
browser_paths = [
|
||||||
os.environ.get("PROGRAMFILES", "C:\\Program Files") + r"\Microsoft\Edge\Application\msedge.exe",
|
win_path(local_app_data, "Google", "Chrome", "Application", "chrome.exe") if local_app_data else None,
|
||||||
os.environ.get("PROGRAMFILES(X86)", "C:\\Program Files (x86)") + r"\Microsoft\Edge\Application\msedge.exe",
|
win_path(program_files, "Google", "Chrome", "Application", "chrome.exe"),
|
||||||
os.environ["PROGRAMFILES"] + r"\Chromium\Application\chrome.exe",
|
win_path(program_files_x86, "Google", "Chrome", "Application", "chrome.exe"),
|
||||||
os.environ["PROGRAMFILES(X86)"] + r"\Chromium\Application\chrome.exe",
|
win_path(local_app_data, "Microsoft", "Edge", "Application", "msedge.exe") if local_app_data else None,
|
||||||
os.environ["LOCALAPPDATA"] + r"\Chromium\Application\chrome.exe",
|
win_path(program_files, "Microsoft", "Edge", "Application", "msedge.exe"),
|
||||||
os.environ["PROGRAMFILES"] + r"\Chrome\Application\chrome.exe",
|
win_path(program_files_x86, "Microsoft", "Edge", "Application", "msedge.exe"),
|
||||||
os.environ["PROGRAMFILES(X86)"] + r"\Chrome\Application\chrome.exe",
|
win_path(local_app_data, "Chromium", "Application", "chrome.exe") if local_app_data else None,
|
||||||
os.environ["LOCALAPPDATA"] + r"\Chrome\Application\chrome.exe",
|
win_path(program_files, "Chromium", "Application", "chrome.exe"),
|
||||||
|
win_path(program_files_x86, "Chromium", "Application", "chrome.exe"),
|
||||||
|
# Intentional fallback for portable/custom distributions installed under a bare "Chrome" directory.
|
||||||
|
win_path(program_files, "Chrome", "Application", "chrome.exe"),
|
||||||
|
win_path(program_files_x86, "Chrome", "Application", "chrome.exe"),
|
||||||
|
win_path(local_app_data, "Chrome", "Application", "chrome.exe") if local_app_data else None,
|
||||||
shutil.which("msedge.exe"),
|
shutil.which("msedge.exe"),
|
||||||
shutil.which("chromium.exe"),
|
shutil.which("chromium.exe"),
|
||||||
shutil.which("chrome.exe"),
|
shutil.which("chrome.exe"),
|
||||||
|
|||||||
@@ -878,6 +878,10 @@ class TestWebScrapingBrowserConfiguration:
|
|||||||
edge_path,
|
edge_path,
|
||||||
chrome_path,
|
chrome_path,
|
||||||
# Windows paths
|
# Windows paths
|
||||||
|
"C:\\Users\\runneradmin\\AppData\\Local\\Google\\Chrome\\Application\\chrome.exe",
|
||||||
|
"C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe",
|
||||||
|
"C:\\Program Files (x86)\\Google\\Chrome\\Application\\chrome.exe",
|
||||||
|
"C:\\Users\\runneradmin\\AppData\\Local\\Microsoft\\Edge\\Application\\msedge.exe",
|
||||||
"C:\\Program Files\\Microsoft\\Edge\\Application\\msedge.exe",
|
"C:\\Program Files\\Microsoft\\Edge\\Application\\msedge.exe",
|
||||||
"C:\\Program Files (x86)\\Microsoft\\Edge\\Application\\msedge.exe",
|
"C:\\Program Files (x86)\\Microsoft\\Edge\\Application\\msedge.exe",
|
||||||
"C:\\Program Files\\Chromium\\Application\\chrome.exe",
|
"C:\\Program Files\\Chromium\\Application\\chrome.exe",
|
||||||
@@ -1085,10 +1089,32 @@ class TestWebScrapingBrowserConfiguration:
|
|||||||
|
|
||||||
# Test Windows with environment variables not set
|
# Test Windows with environment variables not set
|
||||||
monkeypatch.setattr(platform, "system", lambda: "Windows")
|
monkeypatch.setattr(platform, "system", lambda: "Windows")
|
||||||
# Set default values for environment variables
|
|
||||||
monkeypatch.setenv("PROGRAMFILES", "C:\\Program Files")
|
monkeypatch.setenv("PROGRAMFILES", "C:\\Program Files")
|
||||||
monkeypatch.setenv("PROGRAMFILES(X86)", "C:\\Program Files (x86)")
|
monkeypatch.setenv("PROGRAMFILES(X86)", "C:\\Program Files (x86)")
|
||||||
monkeypatch.setenv("LOCALAPPDATA", "C:\\Users\\TestUser\\AppData\\Local")
|
monkeypatch.setenv("LOCALAPPDATA", "C:\\Users\\TestUser\\AppData\\Local")
|
||||||
|
|
||||||
|
local_chrome_path = "C:\\Users\\TestUser\\AppData\\Local\\Google\\Chrome\\Application\\chrome.exe"
|
||||||
|
monkeypatch.setattr(os.path, "isfile", lambda p: p == local_chrome_path)
|
||||||
|
assert scraper.get_compatible_browser() == local_chrome_path
|
||||||
|
|
||||||
|
local_edge_path = "C:\\Users\\TestUser\\AppData\\Local\\Microsoft\\Edge\\Application\\msedge.exe"
|
||||||
|
monkeypatch.setattr(os.path, "isfile", lambda p: p == local_edge_path)
|
||||||
|
assert scraper.get_compatible_browser() == local_edge_path
|
||||||
|
|
||||||
|
local_chromium_path = "C:\\Users\\TestUser\\AppData\\Local\\Chromium\\Application\\chrome.exe"
|
||||||
|
monkeypatch.setattr(os.path, "isfile", lambda p: p == local_chromium_path)
|
||||||
|
assert scraper.get_compatible_browser() == local_chromium_path
|
||||||
|
|
||||||
|
monkeypatch.delenv("LOCALAPPDATA", raising = False)
|
||||||
|
monkeypatch.setenv("USERPROFILE", "C:\\Users\\FallbackUser")
|
||||||
|
fallback_local_chrome_path = "C:\\Users\\FallbackUser\\AppData\\Local\\Google\\Chrome\\Application\\chrome.exe"
|
||||||
|
monkeypatch.setattr(os.path, "isfile", lambda p: p == fallback_local_chrome_path)
|
||||||
|
assert scraper.get_compatible_browser() == fallback_local_chrome_path
|
||||||
|
|
||||||
|
monkeypatch.delenv("PROGRAMFILES", raising = False)
|
||||||
|
monkeypatch.delenv("PROGRAMFILES(X86)", raising = False)
|
||||||
|
monkeypatch.delenv("LOCALAPPDATA", raising = False)
|
||||||
|
monkeypatch.delenv("USERPROFILE", raising = False)
|
||||||
monkeypatch.setattr(os.path, "isfile", lambda p: False)
|
monkeypatch.setattr(os.path, "isfile", lambda p: False)
|
||||||
with pytest.raises(AssertionError, match = "Installed browser could not be detected"):
|
with pytest.raises(AssertionError, match = "Installed browser could not be detected"):
|
||||||
scraper.get_compatible_browser()
|
scraper.get_compatible_browser()
|
||||||
@@ -1350,7 +1376,7 @@ class TestWebScrapingDiagnostics:
|
|||||||
|
|
||||||
def test_diagnose_browser_issues_auto_detect_failure(self, scraper_with_config:WebScrapingMixin, caplog:pytest.LogCaptureFixture) -> None:
|
def test_diagnose_browser_issues_auto_detect_failure(self, scraper_with_config:WebScrapingMixin, caplog:pytest.LogCaptureFixture) -> None:
|
||||||
"""Test diagnostic when auto-detecting browser fails."""
|
"""Test diagnostic when auto-detecting browser fails."""
|
||||||
with patch.object(scraper_with_config, "get_compatible_browser", return_value = None):
|
with patch.object(scraper_with_config, "get_compatible_browser", side_effect = AssertionError("No browser found")):
|
||||||
scraper_with_config.browser_config.binary_location = None
|
scraper_with_config.browser_config.binary_location = None
|
||||||
scraper_with_config.diagnose_browser_issues()
|
scraper_with_config.diagnose_browser_issues()
|
||||||
|
|
||||||
@@ -1748,9 +1774,9 @@ class TestWebScrapingDiagnostics:
|
|||||||
with patch("platform.system", return_value = "Linux"), \
|
with patch("platform.system", return_value = "Linux"), \
|
||||||
patch("kleinanzeigen_bot.utils.web_scraping_mixin._is_admin", return_value = False), \
|
patch("kleinanzeigen_bot.utils.web_scraping_mixin._is_admin", return_value = False), \
|
||||||
patch("psutil.process_iter", return_value = []), \
|
patch("psutil.process_iter", return_value = []), \
|
||||||
patch.object(scraper_with_config, "get_compatible_browser", side_effect = AssertionError("No browser found")), \
|
patch.object(scraper_with_config, "get_compatible_browser", side_effect = AssertionError("No browser found")):
|
||||||
pytest.raises(AssertionError, match = "No browser found"):
|
|
||||||
scraper_with_config.diagnose_browser_issues()
|
scraper_with_config.diagnose_browser_issues()
|
||||||
|
assert "(fail) No compatible browser found" in caplog.text
|
||||||
|
|
||||||
def test_diagnose_browser_issues_user_data_dir_permissions_issue(
|
def test_diagnose_browser_issues_user_data_dir_permissions_issue(
|
||||||
self, scraper_with_config:WebScrapingMixin, caplog:pytest.LogCaptureFixture, tmp_path:Path
|
self, scraper_with_config:WebScrapingMixin, caplog:pytest.LogCaptureFixture, tmp_path:Path
|
||||||
|
|||||||
Reference in New Issue
Block a user