Add unit tests for tests/helpers.py to improve coverage (#9161) #10578
Conversation
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
Reviewer's GuideIntroduces a new test module tests/test_helpers_additional.py to cover eight helper functions from helpers.py, including object creation, file/directory operations, environment isolation, data transformation, and architecture support, thereby improving unit test coverage. 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 there - I've reviewed your changes - here's some feedback:
- The tests import helpers from
tests.helpers, which may shadow the production helpers module—update the import to point directly at the actual helpers package to ensure you’re testing the real code. - Add a test case for
switch_working_directorythat raises an exception inside the context to confirm the original working directory is restored on error. - Consider adding edge‐case tests for
copy_path, such as when the destination already exists or when handling symlinks, to ensure robust behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests import helpers from `tests.helpers`, which may shadow the production helpers module—update the import to point directly at the actual helpers package to ensure you’re testing the real code.
- Add a test case for `switch_working_directory` that raises an exception inside the context to confirm the original working directory is restored on error.
- Consider adding edge‐case tests for `copy_path`, such as when the destination already exists or when handling symlinks, to ensure robust behavior.
## Individual Comments
### Comment 1
<location> `tests/test_helpers_additional.py:22-37` </location>
<code_context>
+ assert str(dep.pretty_constraint) == ">=1.0"
+
+
+def test_copy_path_file_and_dir(tmp_path: Path):
+ # Copy a file
+ src_file = tmp_path / "a.txt"
+ src_file.write_text("hello")
+ dest_file = tmp_path / "b.txt"
+ helpers.copy_path(src_file, dest_file)
+ assert dest_file.exists()
+ assert dest_file.read_text() == "hello"
+
+ # Copy a directory
+ src_dir = tmp_path / "src"
+ src_dir.mkdir()
+ (src_dir / "c.txt").write_text("data")
+ dest_dir = tmp_path / "dest"
+ helpers.copy_path(src_dir, dest_dir)
+ assert (dest_dir / "c.txt").exists()
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Test copying when destination already exists and error conditions.
Please extend the tests to include scenarios where the destination already exists and where the source is missing, to verify error handling and overwriting behavior.
```suggestion
def test_copy_path_file_and_dir(tmp_path: Path):
# Copy a file
src_file = tmp_path / "a.txt"
src_file.write_text("hello")
dest_file = tmp_path / "b.txt"
helpers.copy_path(src_file, dest_file)
assert dest_file.exists()
assert dest_file.read_text() == "hello"
# Copy a file when destination already exists (should overwrite)
dest_file.write_text("old")
helpers.copy_path(src_file, dest_file)
assert dest_file.read_text() == "hello"
# Copy a directory
src_dir = tmp_path / "src"
src_dir.mkdir()
(src_dir / "c.txt").write_text("data")
dest_dir = tmp_path / "dest"
helpers.copy_path(src_dir, dest_dir)
assert (dest_dir / "c.txt").exists()
# Copy a directory when destination already exists (should overwrite file inside)
(dest_dir / "c.txt").write_text("old data")
helpers.copy_path(src_dir, dest_dir)
assert (dest_dir / "c.txt").read_text() == "data"
# Error: source file does not exist
missing_file = tmp_path / "missing.txt"
with pytest.raises(FileNotFoundError):
helpers.copy_path(missing_file, tmp_path / "should_not_exist.txt")
# Error: source directory does not exist
missing_dir = tmp_path / "missing_dir"
with pytest.raises(FileNotFoundError):
helpers.copy_path(missing_dir, tmp_path / "should_not_exist_dir")
```
</issue_to_address>
### Comment 2
<location> `tests/test_helpers_additional.py:52-56` </location>
<code_context>
+ assert os.environ.get(key) == val
+
+
+def test_flatten_dict_nested():
+ nested = {"a": {"b": {"c": 1}}, "x": 2}
+ flat = helpers.flatten_dict(nested)
+ assert flat["a.b.c"] == 1
+ assert flat["x"] == 2
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for edge cases in flatten_dict, such as empty dicts and non-dict values.
Add tests for empty dictionaries, non-dict values, and more deeply nested cases to improve coverage and error handling.
```suggestion
def test_flatten_dict_nested():
nested = {"a": {"b": {"c": 1}}, "x": 2}
flat = helpers.flatten_dict(nested)
assert flat["a.b.c"] == 1
assert flat["x"] == 2
def test_flatten_dict_empty():
empty = {}
flat = helpers.flatten_dict(empty)
assert flat == {}
def test_flatten_dict_non_dict_value():
# If flatten_dict expects a dict, passing a non-dict should raise an error
with pytest.raises(TypeError):
helpers.flatten_dict(42)
def test_flatten_dict_deeply_nested():
nested = {"a": {"b": {"c": {"d": {"e": 5}}}}, "x": 2}
flat = helpers.flatten_dict(nested)
assert flat["a.b.c.d.e"] == 5
assert flat["x"] == 2
def test_flatten_dict_mixed_types():
mixed = {"a": {"b": [1, 2, 3]}, "x": None}
flat = helpers.flatten_dict(mixed)
assert flat["a.b"] == [1, 2, 3]
assert flat["x"] is None
```
</issue_to_address>
### Comment 3
<location> `tests/test_helpers_additional.py:59-64` </location>
<code_context>
+ assert flat["x"] == 2
+
+
+def test_switch_working_directory_creates_and_restores(tmp_path: Path):
+ cwd = Path.cwd()
+ new_dir = tmp_path
+ with helpers.switch_working_directory(new_dir):
+ assert Path.cwd() == new_dir
+ assert Path.cwd() == cwd
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Test error handling for non-existent directories in switch_working_directory.
Add a test to ensure switch_working_directory raises an exception and leaves the working directory unchanged when given a non-existent directory.
```suggestion
def test_switch_working_directory_creates_and_restores(tmp_path: Path):
cwd = Path.cwd()
new_dir = tmp_path
with helpers.switch_working_directory(new_dir):
assert Path.cwd() == new_dir
assert Path.cwd() == cwd
def test_switch_working_directory_nonexistent_dir(tmp_path: Path):
cwd = Path.cwd()
non_existent_dir = tmp_path / "does_not_exist"
with pytest.raises(Exception):
with helpers.switch_working_directory(non_existent_dir):
pass
assert Path.cwd() == cwd
```
</issue_to_address>
### Comment 4
<location> `tests/test_helpers_additional.py:6` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 5
<location> `tests/test_helpers_additional.py:48-49` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 6
<location> `tests/test_helpers_additional.py:83-84` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 7
<location> `tests/test_helpers_additional.py:87-88` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/test_helpers_additional.py
Outdated
Comment on lines
22
to
37
| def test_copy_path_file_and_dir(tmp_path: Path): | ||
| # Copy a file | ||
| src_file = tmp_path / "a.txt" | ||
| src_file.write_text("hello") | ||
| dest_file = tmp_path / "b.txt" | ||
| helpers.copy_path(src_file, dest_file) | ||
| assert dest_file.exists() | ||
| assert dest_file.read_text() == "hello" | ||
|
|
||
| # Copy a directory | ||
| src_dir = tmp_path / "src" | ||
| src_dir.mkdir() | ||
| (src_dir / "c.txt").write_text("data") | ||
| dest_dir = tmp_path / "dest" | ||
| helpers.copy_path(src_dir, dest_dir) | ||
| assert (dest_dir / "c.txt").exists() |
There was a problem hiding this comment.
suggestion (testing): Test copying when destination already exists and error conditions.
Please extend the tests to include scenarios where the destination already exists and where the source is missing, to verify error handling and overwriting behavior.
Suggested change
| def test_copy_path_file_and_dir(tmp_path: Path): | |
| # Copy a file | |
| src_file = tmp_path / "a.txt" | |
| src_file.write_text("hello") | |
| dest_file = tmp_path / "b.txt" | |
| helpers.copy_path(src_file, dest_file) | |
| assert dest_file.exists() | |
| assert dest_file.read_text() == "hello" | |
| # Copy a directory | |
| src_dir = tmp_path / "src" | |
| src_dir.mkdir() | |
| (src_dir / "c.txt").write_text("data") | |
| dest_dir = tmp_path / "dest" | |
| helpers.copy_path(src_dir, dest_dir) | |
| assert (dest_dir / "c.txt").exists() | |
| def test_copy_path_file_and_dir(tmp_path: Path): | |
| # Copy a file | |
| src_file = tmp_path / "a.txt" | |
| src_file.write_text("hello") | |
| dest_file = tmp_path / "b.txt" | |
| helpers.copy_path(src_file, dest_file) | |
| assert dest_file.exists() | |
| assert dest_file.read_text() == "hello" | |
| # Copy a file when destination already exists (should overwrite) | |
| dest_file.write_text("old") | |
| helpers.copy_path(src_file, dest_file) | |
| assert dest_file.read_text() == "hello" | |
| # Copy a directory | |
| src_dir = tmp_path / "src" | |
| src_dir.mkdir() | |
| (src_dir / "c.txt").write_text("data") | |
| dest_dir = tmp_path / "dest" | |
| helpers.copy_path(src_dir, dest_dir) | |
| assert (dest_dir / "c.txt").exists() | |
| # Copy a directory when destination already exists (should overwrite file inside) | |
| (dest_dir / "c.txt").write_text("old data") | |
| helpers.copy_path(src_dir, dest_dir) | |
| assert (dest_dir / "c.txt").read_text() == "data" | |
| # Error: source file does not exist | |
| missing_file = tmp_path / "missing.txt" | |
| with pytest.raises(FileNotFoundError): | |
| helpers.copy_path(missing_file, tmp_path / "should_not_exist.txt") | |
| # Error: source directory does not exist | |
| missing_dir = tmp_path / "missing_dir" | |
| with pytest.raises(FileNotFoundError): | |
| helpers.copy_path(missing_dir, tmp_path / "should_not_exist_dir") |
jalpuche1
force-pushed
the
add-helpers-tests
branch
from
aae2968 to
93de237
Compare
jalpuche1
force-pushed
the
add-helpers-tests
branch
from
93de237 to
c9d88ea
Compare
radoering
requested changes
Nov 1, 2025
Member
radoering
left a comment
radoering
left a comment
There was a problem hiding this comment.
The tests for utils/helpers.py should be in tests/utils/test_helpers.py. Please move them there instead of creating a new test file.
Many of the tests are just smoke tests, but I suppose it is a start and better than nothing.
| assert "," in result | ||
|
|
||
|
|
||
| def test_ensure_path_creates_directory(tmp_path: Path) -> None: |
Member
There was a problem hiding this comment.
ensure_path does not create a directory. It just raises an error if the directory/file does not exist. Here, the different cases should be tested.
| assert result.is_dir() | ||
|
|
||
|
|
||
| def test_is_dir_writable_returns_true(tmp_path: Path) -> None: |
Member
There was a problem hiding this comment.
Here a parametrized test would make sense.
| assert helpers.pluralize(3, "file") == "files" | ||
|
|
||
|
|
||
| def test_get_file_hash(tmp_path: Path) -> None: |
Member
There was a problem hiding this comment.
Here, it probably makes more sense to write a file (bytes) and check for the fixed hash.
|
|
||
| # directory() is a context manager, not an iterator | ||
| with helpers.directory(tmp_path) as path: | ||
| assert path == tmp_path |
Member
There was a problem hiding this comment.
Here, we should check the cwd.
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.
Pull Request Check List
Relates-to: #9161
Summary
This PR adds new unit tests for existing functions in src/poetry/utils/helpers.py to improve test coverage and ensure consistent behavior across supported platforms.
Covered Functions
The following helper functions are now tested: