test: Windows pytest tempdir hardening#56
Closed
Dj-Shortcut wants to merge 14 commits intoAlanRockefeller:mainfrom
Closed
test: Windows pytest tempdir hardening#56Dj-Shortcut wants to merge 14 commits intoAlanRockefeller:mainfrom
Dj-Shortcut wants to merge 14 commits intoAlanRockefeller:mainfrom
Conversation
Force pytest to use a clean basetemp (tmp/pytest-clean) and ignore legacy temp/cache directories to avoid ACL-related PermissionError on Windows. No changes to application code or test logic.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralized TurboJPEG discovery/initialization into a new helper; updated JPEG and prefetcher modules to use it; made logging setup resilient to unwritable app-data paths; added pytest config and gitignore entries; expanded README with Windows TurboJPEG instructions and troubleshooting. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6dd99dc25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/logging_setup.py`:
- Around line 22-27: The function _can_create_directory currently treats a
candidate path as creatable if its nearest existing parent is writable, which
incorrectly allows candidate paths that already exist as regular files; update
_can_create_directory to first reject any path that exists and is not a
directory (i.e., if path.exists() and not path.is_dir(): return False), then
proceed with the existing parent-traversal and writable check; this prevents
get_app_data_dir and setup_logging from selecting a file-backed candidate and
avoids later mkdir() failures.
In `@faststack/tests/test_turbo.py`:
- Around line 108-126: Ensure the test exercises the RotatingFileHandler error
path and fully restores logger state: monkeypatch get_app_data_dir as you
already do but also monkeypatch Path.mkdir (or logging_setup.Path.mkdir) to a
no-op so directory creation doesn't raise, while mocking
logging_setup.logging.handlers.RotatingFileHandler to raise OSError("disk
full"); call logging_setup.setup_logging(debug=True) and then assert a
StreamHandler remains; finally restore both root_logger.handlers and
root_logger.level to their original values so later tests are unaffected.
In `@README.md`:
- Around line 91-99: Update the README troubleshooting snippet to match the new
log text emitted by faststack/imaging/turbo.py: replace the two-line old message
("Unable to locate turbojpeg library automatically." / "You may specify the
turbojpeg library path manually.") with the current log "TurboJPEG
initialization failed for all attempted locations. Falling back to Pillow." so
users will see the exact message produced at startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bd0bb55-98c8-416f-9310-093bd6dd10ab
📒 Files selected for processing (8)
.gitignoreREADME.mdfaststack/imaging/jpeg.pyfaststack/imaging/turbo.pyfaststack/logging_setup.pyfaststack/tests/test_turbo.pyfaststack/thumbnail_view/prefetcher.pypytest.ini
There was a problem hiding this comment.
🧹 Nitpick comments (2)
faststack/imaging/turbo.py (2)
82-87: Consider narrowing exception scope (Ruff BLE001).Catching bare
Exceptionis flagged by static analysis. In this fallback pattern, broad catching is somewhat justified since native library loading can raise varied exceptions (OSError,RuntimeError, library-specific errors). However, if you want to address the linter warning, you could catch a tuple of expected types:♻️ Proposed refinement
try: decoder = TurboJPEG() if candidate is None else TurboJPEG(candidate) - except Exception as exc: + except (OSError, RuntimeError, TypeError, ValueError) as exc: source = "default loader" if candidate is None else candidate failures.append(f"{source}: {exc}") continueAlternatively, keep
Exceptionif you've found the library can raise unpredictable errors—the current approach ensures graceful fallback and logs failures appropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/turbo.py` around lines 82 - 87, The try/except around TurboJPEG() currently catches a bare Exception; replace it with a narrower exception tuple to satisfy the linter and still handle expected loader failures — catch OSError and RuntimeError (and any library-specific error class if known) in the except clause that logs to failures and uses the same source/candidate logic; update the except header referencing TurboJPEG, candidate and failures so only those expected errors are caught while preserving the existing failures.append(...) and continue behavior.
12-16: Consider clarifyingTJPF_RGBexport or removing if unused.
TJPF_RGBis imported but not used within this module. If it's intended for re-export (so callers can import from this module instead ofturbojpegdirectly), consider adding an__all__to make exports explicit. Otherwise, remove the unused import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/turbo.py` around lines 12 - 16, The module imports TJPF_RGB but never uses it; either remove the unused import or explicitly export it so callers can import it from this module: update the try/except block to only import TurboJPEG if TJPF_RGB is not needed, or keep TJPF_RGB and add an explicit __all__ = ["TurboJPEG", "TJPF_RGB"] (or include whichever names you want to expose) to make exports explicit; reference the TurboJPEG and TJPF_RGB symbols and the module-level __all__ when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@faststack/imaging/turbo.py`:
- Around line 82-87: The try/except around TurboJPEG() currently catches a bare
Exception; replace it with a narrower exception tuple to satisfy the linter and
still handle expected loader failures — catch OSError and RuntimeError (and any
library-specific error class if known) in the except clause that logs to
failures and uses the same source/candidate logic; update the except header
referencing TurboJPEG, candidate and failures so only those expected errors are
caught while preserving the existing failures.append(...) and continue behavior.
- Around line 12-16: The module imports TJPF_RGB but never uses it; either
remove the unused import or explicitly export it so callers can import it from
this module: update the try/except block to only import TurboJPEG if TJPF_RGB is
not needed, or keep TJPF_RGB and add an explicit __all__ = ["TurboJPEG",
"TJPF_RGB"] (or include whichever names you want to expose) to make exports
explicit; reference the TurboJPEG and TJPF_RGB symbols and the module-level
__all__ when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67989e61-beaa-41a9-8ecb-e6f865405b25
📒 Files selected for processing (2)
faststack/imaging/turbo.pyfaststack/tests/test_turbo.py
✅ Files skipped from review due to trivial changes (1)
- faststack/tests/test_turbo.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
faststack/tests/test_turbo.py (1)
153-171:⚠️ Potential issue | 🟡 MinorMake this test hit the
RotatingFileHandlerpath and fully restore logger state.
Path("/bad-path")usually makesmkdir()fail beforeRotatingFileHandleris constructed, so the mock does not verify the branch this test is named for. The assertion is also too broad becauseFileHandleris aStreamHandler, andsetup_logging(debug=True)leaves multiple logger levels mutated after the test.🧪 Proposed fix
-def test_setup_logging_keeps_console_handler_when_file_logging_fails(monkeypatch): +def test_setup_logging_keeps_console_handler_when_file_logging_fails(monkeypatch, tmp_path): logging_setup = importlib.import_module("faststack.logging_setup") - monkeypatch.setattr(logging_setup, "get_app_data_dir", lambda: Path("/bad-path")) + monkeypatch.setattr(logging_setup, "get_app_data_dir", lambda: tmp_path / "appdata") + rotating_file_handler = Mock(side_effect=OSError("disk full")) monkeypatch.setattr( logging_setup.logging.handlers, "RotatingFileHandler", - Mock(side_effect=OSError("disk full")), + rotating_file_handler, ) root_logger = logging.getLogger() original_handlers = list(root_logger.handlers) + original_root_level = root_logger.level + cache_logger = logging.getLogger("faststack.imaging.cache") + prefetch_logger = logging.getLogger("faststack.imaging.prefetch") + pil_logger = logging.getLogger("PIL") + original_cache_level = cache_logger.level + original_prefetch_level = prefetch_logger.level + original_pil_level = pil_logger.level try: logging_setup.setup_logging(debug=True) + rotating_file_handler.assert_called_once() assert any( - isinstance(handler, logging.StreamHandler) for handler in root_logger.handlers + isinstance(handler, logging.StreamHandler) + and not isinstance(handler, logging.FileHandler) + for handler in root_logger.handlers ) finally: root_logger.handlers = original_handlers + root_logger.setLevel(original_root_level) + cache_logger.setLevel(original_cache_level) + prefetch_logger.setLevel(original_prefetch_level) + pil_logger.setLevel(original_pil_level)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/test_turbo.py` around lines 153 - 171, The test currently never reaches the RotatingFileHandler construction and doesn't fully restore logging state; update test_setup_logging_keeps_console_handler_when_file_logging_fails to monkeypatch get_app_data_dir to return a writable temporary Path (so mkdir succeeds) and mock logging_setup.logging.handlers.RotatingFileHandler to raise OSError to exercise that branch, then before calling logging_setup.setup_logging(debug=True) capture root_logger.handlers and root_logger.level (and any other mutated logger levels if setup_logging touches them) and after the test restore root_logger.handlers and root_logger.level to their originals to fully revert logger state and avoid leaking level/handler changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/logging_setup.py`:
- Around line 48-50: The app-data candidate Paths are being computed eagerly
(via Path.home(), gettempdir(), Path.cwd()) causing exceptions to propagate
before env-var candidates (FASTSTACK_APPDATA, APPDATA) are checked; change
setup_logging() so that candidates is built lazily: replace the precomputed
append calls with a sequence that evaluates each candidate inside the loop (or
use a generator/iterable) and wrap each evaluation in a try/except that ignores
RuntimeError/OSError so only successfully-resolved Paths are considered;
reference the existing candidates list and the functions Path.home(),
gettempdir(), Path.cwd(), and ensure the lazy resolution approach is applied to
the same logic later (the other occurrence noted around the code at the second
mention).
In `@faststack/tests/test_turbo.py`:
- Around line 78-95: The test
test_get_app_data_dir_falls_back_when_appdata_is_not_creatable can be flakey
because get_app_data_dir() prefers FASTSTACK_APPDATA; before setting APPDATA you
should clear FASTSTACK_APPDATA to ensure the code exercises the APPDATA
branch—use monkeypatch.delenv("FASTSTACK_APPDATA", raising=False) (in the same
test) so the test reliably falls back to the blocked APPDATA path and then to
the home-based fallback_dir.
---
Duplicate comments:
In `@faststack/tests/test_turbo.py`:
- Around line 153-171: The test currently never reaches the RotatingFileHandler
construction and doesn't fully restore logging state; update
test_setup_logging_keeps_console_handler_when_file_logging_fails to monkeypatch
get_app_data_dir to return a writable temporary Path (so mkdir succeeds) and
mock logging_setup.logging.handlers.RotatingFileHandler to raise OSError to
exercise that branch, then before calling
logging_setup.setup_logging(debug=True) capture root_logger.handlers and
root_logger.level (and any other mutated logger levels if setup_logging touches
them) and after the test restore root_logger.handlers and root_logger.level to
their originals to fully revert logger state and avoid leaking level/handler
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68d0900f-a556-4696-af2f-debf9b77e3ba
📒 Files selected for processing (2)
faststack/logging_setup.pyfaststack/tests/test_turbo.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/logging_setup.py`:
- Around line 46-50: The fallback order in deferred_candidates currently prefers
gettempdir() before the repository-local fallback, which causes
get_app_data_dir() to return a volatile temp location; update
deferred_candidates (and the similar block at the later occurrence) to prefer
Path.cwd() / "var" / "appdata" (or any non-temp persistent local dirs) before
using Path(gettempdir()) / "faststack", so that Path.home() remains first, then
the project-local var/appdata, and only then the system temp directory; modify
the two occurrences (identify by the variable name deferred_candidates)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d9b2bbb-7d32-45e7-b6f5-d9b02eb45bf2
📒 Files selected for processing (3)
README.mdfaststack/logging_setup.pyfaststack/tests/test_turbo.py
✅ Files skipped from review due to trivial changes (1)
- README.md
Owner
|
The contents of this PR will be merged into main by PR # 58. Thanks for helping out! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes pytest failures on Windows caused by inaccessible legacy temp directories
(e.g. var/pytest-temp, .pytest-local, .pytest-tmp), which can trigger
PermissionError (WinError 5) during cleanup.
This change:
No changes to application code or test logic.
Split out from the DLL/JPEG PR to keep that review focused.
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests
Chores
related to : #53