Merged
Conversation
- Replace [fs](https://pypi.org/project/fs/) by [fsspec](https://pypi.org/project/fsspec/) because of deprecation of [pkg_resource](https://setuptools.pypa.io/en/latest/history.html#deprecations-and-removals) starting pyhton>3.12 from Setuptools to fix CI/CD. - Update `test_filer.py` to use `fsspec`
Reviewer's GuideReplaces deprecated fs-based filesystem usage in tests with fsspec, updates the directory tree helper and expectations accordingly, and switches the test dependency from fs to fsspec. Flow diagram for updated fsspec-based test_filer workflowflowchart TD
A[Test_run_start] --> B[Initialize_fsspec_filesystem]
B --> C[Create_test_directory_tree_via_fsspec]
C --> D[Invoke_filer_code_under_test]
D --> E[Collect_directory_listing_or_metadata]
E --> F[Assert_expectations_in_tests]
F --> G[Test_run_end]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
getTreeimplementation relies on the iteration order fromfs.walkbut then asserts against a fixed string; to avoid flaky tests, consider sortingdirsandfilesbefore writing them to the buffer. - The helper functions introduce new dependencies (
fsspec.core.url_to_fs,shutil.rmtree) but there are no corresponding imports shown in this diff; ensure required modules are imported so tests remain self-contained. - The normalization helper uses the prefix
dist1/dist2while the actual directory variables aredst1/dst2; consider aligning the naming to avoid confusion and potential future mistakes when reading or modifying these tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `getTree` implementation relies on the iteration order from `fs.walk` but then asserts against a fixed string; to avoid flaky tests, consider sorting `dirs` and `files` before writing them to the buffer.
- The helper functions introduce new dependencies (`fsspec.core.url_to_fs`, `shutil.rmtree`) but there are no corresponding imports shown in this diff; ensure required modules are imported so tests remain self-contained.
- The normalization helper uses the prefix `dist1`/`dist2` while the actual directory variables are `dst1`/`dst2`; consider aligning the naming to avoid confusion and potential future mistakes when reading or modifying these tests.
## Individual Comments
### Comment 1
<location path="tests/test_filer.py" line_range="31-38" />
<code_context>
+
+ return out.getvalue()
+
+def normalize_tree(tree_str, abs_root, prefix):
+ """Convert absolute paths from getTree into relative paths."""
+ lines = []
+ for line in tree_str.splitlines():
+ stripped = line.replace(abs_root, prefix)
+ stripped = stripped.lstrip("/")
+ lines.append(stripped)
+ return "\n".join(lines)
+
+def rmDir(d):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Make `normalize_tree` stricter about root replacement to avoid accidental substitutions.
`normalize_tree` currently uses `line.replace(abs_root, prefix)`, which may also modify occurrences of `abs_root` inside filenames or nested paths. To better match the intended behavior, only rewrite when the line actually begins with `abs_root`, e.g.:
```python
if line.startswith(abs_root):
stripped = prefix + line[len(abs_root):]
else:
stripped = line
stripped = stripped.lstrip("/")
```
```suggestion
def normalize_tree(tree_str, abs_root, prefix):
"""Convert absolute paths from getTree into relative paths."""
lines = []
for line in tree_str.splitlines():
if line.startswith(abs_root):
stripped = prefix + line[len(abs_root):]
else:
stripped = line
stripped = stripped.lstrip("/")
lines.append(stripped)
return "\n".join(lines)
```
</issue_to_address>
### Comment 2
<location path="tests/test_filer.py" line_range="163-169" />
<code_context>
# Let's try to copy
copyDir(src, dst1)
+ tree = getTree(dst1)
+ abs_dst1 = os.path.abspath(dst1)
+ normalizedTree = normalize_tree(tree, abs_dst1, "dist1")
+
+ expected = "dist1\na/\n3.txt\ndist1/a\n2.txt\n1.txt".strip()
-
- self.assertEqual(getTree(dst1),
- stripLines('''
- |-- a
- | |-- 1.txt
- | `-- 2.txt
- `-- 3.txt
- '''
- )
- )
+ self.assertEqual(normalizedTree, expected)
# Copying to non-existing dst -----------------------------------------
</code_context>
<issue_to_address>
**suggestion (testing):** Revisit the expected tree format to better reflect directory structure and avoid brittle string comparisons.
The new expectation asserts against a flat string, which has two drawbacks:
* It obscures parent/child relationships by mixing root and child paths as plain lines.
* It bakes in the exact traversal order from `fsspec.walk`, making the test brittle to incidental ordering (e.g., `2.txt` before `1.txt`).
To keep the test focused on contents rather than ordering, consider either:
* Converting `normalizedTree` to a structured set of relative paths (e.g., `{"a/1.txt", "a/2.txt", "3.txt"}`) and asserting on that, or
* Comparing sorted lines from `normalizedTree.splitlines()` and `expected.splitlines()` if order is irrelevant.
```suggestion
tree = getTree(dst1)
abs_dst1 = os.path.abspath(dst1)
normalizedTree = normalize_tree(tree, abs_dst1, "dist1")
expected = "dist1\na/\n3.txt\ndist1/a\n2.txt\n1.txt".strip()
normalized_lines = sorted(normalizedTree.splitlines())
expected_lines = sorted(expected.splitlines())
self.assertEqual(normalized_lines, expected_lines)
```
</issue_to_address>
### Comment 3
<location path="tests/test_filer.py" line_range="171-178" />
<code_context>
- )
+ self.assertEqual(normalizedTree, expected)
# Copying to non-existing dst -----------------------------------------
- self.assertFalse(os.path.exists(dst2)) # dst2 should not exist
-
- # Let's try to copy
+ # # Let's try to copy
copyDir(src, dst2)
+ tree = getTree(dst2)
+ abs_dst2 = os.path.abspath(dst2)
+ normalizedTree = normalize_tree(tree, abs_dst2, "dist2")
- self.assertEqual(getTree(dst2),
- stripLines('''
- |-- a
- | |-- 1.txt
- | `-- 2.txt
- `-- 3.txt
- '''
- )
- )
+ expected = "dist2\na/\n3.txt\ndist2/a\n2.txt\n1.txt".strip()
+
+ self.assertEqual(normalizedTree, expected)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider keeping an explicit precondition check that `dst2` does not exist before running `copyDir`.
This check ensures the test is explicitly verifying the "copy to a non-existing destination" scenario. Without it, a pre-existing `dst2` (e.g., created by another test or fixture) could cause the test to pass while exercising a different code path, weakening its value and potentially hiding regressions. Reintroducing the assertion keeps the test’s preconditions clear and robust.
Suggested implementation:
```python
# Copying to non-existing dst -----------------------------------------
self.assertFalse(os.path.exists(dst2)) # dst2 should not exist
# Let's try to copy
copyDir(src, dst2)
tree = getTree(dst2)
abs_dst2 = os.path.abspath(dst2)
normalizedTree = normalize_tree(tree, abs_dst2, "dist2")
```
If `os` is not already imported at the top of `tests/test_filer.py`, add `import os` to the imports section.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
test_filer.pyto usefsspecSummary by Sourcery
Replace the deprecated fs-based test filesystem usage with fsspec and adjust directory tree assertions accordingly.
Build:
Tests: