test: cover is_dir() OSError guard in _full_scandir_discovery (#846)#852
Open
test: cover is_dir() OSError guard in _full_scandir_discovery (#846)#852
is_dir() OSError guard in _full_scandir_discovery (#846)#852Conversation
Add test_full_scandir_is_dir_oserror_skips_entry to verify that a root-level DirEntry whose is_dir() raises OSError (e.g. broken symlink or EACCES on lstat) is silently skipped rather than crashing discovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds targeted unit-test coverage for a previously untested OSError guard in session discovery (_full_scandir_discovery), ensuring discovery skips a root-level entry whose DirEntry.is_dir() raises instead of crashing.
Changes:
- Add
test_full_scandir_is_dir_oserror_skips_entryto cover thetry/except OSErroraroundsession_entry.is_dir(follow_symlinks=False). - Extend test imports to include
MagicMockto simulate a failingDirEntry.is_dir()call.
microsasa
added
the
aw-review-response-attempted
- Remove eager original_scandir(path) call that leaked a ScandirIterator when the root branch returns _WrappedCtx instead - Use 'with original_scandir(path) as it:' in __enter__ to deterministically close the iterator Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Commit pushed:
|
microsasa
added
aw-review-response-1
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Commit pushed:
|
microsasa
added
aw-review-response-2
microsasa
added
the
aw-review-response-attempted
Keep ScandirIterator open for the lifetime of the _WrappedCtx context manager so DirEntry.is_dir() calls don't fail on DT_UNKNOWN filesystems (where closing the iterator invalidates the dir fd needed for lstat). The iterator is now yielded lazily and closed in __exit__, matching the real os.scandir lifecycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Commit pushed:
|
microsasa
added
aw-review-response-3
microsasa
added
the
aw-review-response-attempted
Change _patched_scandir return type from Iterator to AbstractContextManager[Iterator[os.DirEntry[str]]] so both branches type-check without # type: ignore[return-value] comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Commit pushed:
|
Comment on lines
+656
to
+679
| def __init__(self) -> None: | ||
| self._it: Iterator[os.DirEntry[str]] | None = None | ||
|
|
||
| def _iter_wrapped_entries(self) -> Iterator[os.DirEntry[str]]: | ||
| if self._it is None: | ||
| return | ||
| for e in self._it: | ||
| if e.name == "sess-bad": | ||
| m = MagicMock(spec=os.DirEntry) | ||
| m.name = e.name | ||
| m.path = e.path | ||
| m.is_dir.side_effect = OSError("lstat failed") | ||
| yield cast(os.DirEntry[str], m) | ||
| else: | ||
| yield e | ||
|
|
||
| def __enter__(self) -> Iterator[os.DirEntry[str]]: | ||
| self._it = original_scandir(path) | ||
| return self._iter_wrapped_entries() | ||
|
|
||
| def __exit__(self, *a: object) -> None: | ||
| if self._it is not None: | ||
| self._it.close() # type: ignore[union-attr] | ||
| self._it = None |
There was a problem hiding this comment.
self._it is typed as Iterator[...] | None, but it’s always the concrete iterator returned by os.scandir(...), which provides .close(). Typing it as the concrete scandir iterator type (or a small Protocol exposing __iter__ + close()) would let you call .close() without # type: ignore[union-attr], keeping the test strictly type-safe.
Owner
Author
|
❌ Pipeline orchestrator: review-response loop reached 3 rounds. Marking as stuck for human review. |
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
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.
Closes #846
What
Adds
test_full_scandir_is_dir_oserror_skips_entrytoTestDiscoverWithIdentityNoAbsentPlanStatintests/copilot_usage/test_parser.py, covering the previously untestedtry/except OSErrorguard aroundsession_entry.is_dir(follow_symlinks=False)in_full_scandir_discovery.How
The test creates two session directories (
sess-good,sess-bad), then patchesos.scandirto wrap the root-level entries so thatsess-bad'sDirEntry.is_dir()raisesOSError("lstat failed"). It asserts that_discover_with_identityreturns exactly one session (the good one), confirming the faulting entry is silently skipped.Why
Without this test, a refactor that removes the inner
try/except OSErroraroundis_dir()would introduce a crash on broken symlinks with no test catching the regression.