Skip to content

Commit 2acb8f4

Browse files
committed
test: improve extract.py coverage and address async safety issues
Coverage improvements: - Add tests for image download with None URLs (lines 124-136) - Add tests for directory handling scenarios (lines 420-454) - Test deletion when final_dir exists - Test renaming when rename_existing_folders=True - Test using existing directory when renaming disabled - Simplify download_ad tests to focus on observable behavior - Improve type annotations (tmp_path: Path instead of Any) - Coverage increased to 87.70% Async safety improvements: - Remove unused # noqa: ASYNC240 from _exists, _isdir, ainput - Eliminate TOCTOU race in directory creation using mkdir(exist_ok=True) - Offload dicts.save_dict to executor (blocking I/O) Path handling improvements: - Migrate to pathlib.Path for OS-agnostic file operations - Replace string concatenation with Path operations - Use Path.name instead of rsplit for filename extraction Exception handling improvements: - Narrow exception handling in _download_and_save_image_sync - Catch only expected errors (URLError, HTTPError, OSError, shutil.Error) - Don't swallow programming errors Translation updates: - Update German translation for directory creation message
1 parent aafde54 commit 2acb8f4

File tree

4 files changed

+210
-195
lines changed

4 files changed

+210
-195
lines changed

src/kleinanzeigen_bot/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -937,7 +937,7 @@ async def publish_ad(self, ad_file:str, ad_cfg:Ad, ad_cfg_orig:dict[str, Any], p
937937
LOG.warning("# Payment form detected! Please proceed with payment.")
938938
LOG.warning("############################################")
939939
await self.web_scroll_page_down()
940-
await ainput(_("Press a key to continue...")) # noqa: ASYNC240
940+
await ainput(_("Press a key to continue..."))
941941
except TimeoutError:
942942
pass
943943

src/kleinanzeigen_bot/extract.py

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from gettext import gettext as _
66

77
import json, mimetypes, re, shutil # isort: skip
8+
import urllib.error as urllib_error
89
import urllib.request as urllib_request
910
from datetime import datetime
1011
from pathlib import Path
@@ -38,13 +39,13 @@ def _path_is_dir(path:Path | str) -> bool:
3839

3940

4041
async def _exists(path:Path | str) -> bool:
41-
result = await asyncio.get_running_loop().run_in_executor(None, _path_exists, path) # noqa: ASYNC240
42+
result = await asyncio.get_running_loop().run_in_executor(None, _path_exists, path)
4243
LOG.debug("Path exists check: %s -> %s", path, result)
4344
return result
4445

4546

4647
async def _isdir(path:Path | str) -> bool:
47-
result = await asyncio.get_running_loop().run_in_executor(None, _path_is_dir, path) # noqa: ASYNC240
48+
result = await asyncio.get_running_loop().run_in_executor(None, _path_is_dir, path)
4849
LOG.debug("Path is_dir check: %s -> %s", path, result)
4950
return result
5051

@@ -69,35 +70,36 @@ async def download_ad(self, ad_id:int) -> None:
6970

7071
# create sub-directory for ad(s) to download (if necessary):
7172
relative_directory = Path("downloaded-ads")
72-
# make sure configured base directory exists
73-
if not await _exists(relative_directory) or not await _isdir(relative_directory):
74-
LOG.debug("Creating base directory: %s", relative_directory)
75-
await asyncio.get_running_loop().run_in_executor(None, relative_directory.mkdir)
76-
LOG.info("Created ads directory at ./%s.", relative_directory)
73+
# make sure configured base directory exists (using exist_ok=True to avoid TOCTOU race)
74+
await asyncio.get_running_loop().run_in_executor(None, lambda: relative_directory.mkdir(exist_ok = True)) # noqa: ASYNC240
75+
LOG.info("Ensured ads directory exists at ./%s.", relative_directory)
7776

7877
# Extract ad info and determine final directory path
7978
ad_cfg, final_dir = await self._extract_ad_page_info_with_directory_handling(
8079
relative_directory, ad_id
8180
)
8281

83-
# Save the ad configuration file
82+
# Save the ad configuration file (offload to executor to avoid blocking the event loop)
8483
ad_file_path = str(Path(final_dir) / f"ad_{ad_id}.yaml")
85-
dicts.save_dict(
86-
ad_file_path,
87-
ad_cfg.model_dump(),
88-
header = "# yaml-language-server: $schema=https://raw.githubusercontent.com/Second-Hand-Friends/kleinanzeigen-bot/refs/heads/main/schemas/ad.schema.json")
84+
header_string = "# yaml-language-server: $schema=https://raw.githubusercontent.com/Second-Hand-Friends/kleinanzeigen-bot/refs/heads/main/schemas/ad.schema.json"
85+
await asyncio.get_running_loop().run_in_executor(
86+
None,
87+
lambda: dicts.save_dict(ad_file_path, ad_cfg.model_dump(), header = header_string)
88+
)
8989

9090
@staticmethod
9191
def _download_and_save_image_sync(url:str, directory:str, filename_prefix:str, img_nr:int) -> str | None:
9292
try:
9393
with urllib_request.urlopen(url) as response: # noqa: S310 Audit URL open for permitted schemes.
9494
content_type = response.info().get_content_type()
9595
file_ending = mimetypes.guess_extension(content_type) or ""
96-
img_path = f"{directory}/{filename_prefix}{img_nr}{file_ending}"
96+
# Use pathlib.Path for OS-agnostic path handling
97+
img_path = Path(directory) / f"{filename_prefix}{img_nr}{file_ending}"
9798
with open(img_path, "wb") as f:
9899
shutil.copyfileobj(response, f)
99-
return img_path
100-
except Exception as e:
100+
return str(img_path)
101+
except (urllib_error.URLError, urllib_error.HTTPError, OSError, shutil.Error) as e:
102+
# Narrow exception handling to expected network/filesystem errors
101103
LOG.warning("Failed to download image %s: %s", url, e)
102104
return None
103105

@@ -142,7 +144,8 @@ async def _download_images_from_ad_page(self, directory:str, ad_id:int) -> list[
142144

143145
if img_path:
144146
dl_counter += 1
145-
img_paths.append(img_path.rsplit("/", maxsplit = 1)[-1])
147+
# Use pathlib.Path for OS-agnostic path handling
148+
img_paths.append(Path(img_path).name)
146149

147150
img_nr += 1
148151
LOG.info("Downloaded %s.", i18n.pluralize("image", dl_counter))

src/kleinanzeigen_bot/resources/translations.de.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ kleinanzeigen_bot/__init__.py:
173173
kleinanzeigen_bot/extract.py:
174174
#################################################
175175
download_ad:
176-
"Created ads directory at ./%s.": "Verzeichnis für Anzeigen erstellt unter ./%s."
176+
"Ensured ads directory exists at ./%s.": "Verzeichnis für Anzeigen sichergestellt unter ./%s."
177177

178178
_download_and_save_image_sync:
179179
"Failed to download image %s: %s": "Fehler beim Herunterladen des Bildes %s: %s"

0 commit comments

Comments
 (0)