Add tests for pbs_installer_supported_arch#10686
Open
stavroulagalani wants to merge 1 commit intopython-poetry:mainfrom
Open
Add tests for pbs_installer_supported_arch#10686stavroulagalani wants to merge 1 commit intopython-poetry:mainfrom
stavroulagalani wants to merge 1 commit intopython-poetry:mainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds targeted unit tests for pbs_installer_supported_arch to verify accepted and rejected architectures and slightly extends the test_helpers imports accordingly. File-Level Changes
Assessment against linked issues
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 2 issues, and left some high level feedback:
- The inline banner comment (
#---------------------2 new tests-----------------------) is unnecessary noise in the test file and could be removed to keep the style consistent with existing tests. - Consider parametrizing the supported/unsupported architecture checks with
pytest.mark.parametrizeto reduce repetition and make it easier to add more cases in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The inline banner comment (`#---------------------2 new tests-----------------------`) is unnecessary noise in the test file and could be removed to keep the style consistent with existing tests.
- Consider parametrizing the supported/unsupported architecture checks with `pytest.mark.parametrize` to reduce repetition and make it easier to add more cases in the future.
## Individual Comments
### Comment 1
<location> `tests/test_helpers.py:51-54` </location>
<code_context>
+
+#---------------------2 new tests-----------------------
+
+def test_pbs_installer_supported_arch_accepts_supported_archs() -> None:
+ assert pbs_installer_supported_arch("x86_64") is True
+ assert pbs_installer_supported_arch("amd64") is True
+ assert pbs_installer_supported_arch("arm64") is True
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for architecture aliases and edge cases (e.g. aarch64, case differences, empty string).
Beyond the main happy-path values, please add tests for alias and edge inputs that this helper should handle, e.g. `"aarch64"`, case variants like `"ARM64"`, and clearly invalid-but-string inputs such as `""` or whitespace-only. That will document and lock in the expected normalization and failure behavior.
Suggested implementation:
```python
from tests.helpers import flatten_dict
from tests.helpers import isolated_environment
from tests.helpers import pbs_installer_supported_arch
def test_flatten_dict() -> None:
with isolated_environment(environ={"NEW_VAR": "new_value"}):
assert os.environ["NEW_VAR"] == "new_value"
assert "NEW_VAR" not in os.environ
def test_pbs_installer_supported_arch_accepts_supported_archs() -> None:
assert pbs_installer_supported_arch("x86_64") is True
assert pbs_installer_supported_arch("amd64") is True
assert pbs_installer_supported_arch("arm64") is True
def test_pbs_installer_supported_arch_accepts_aliases_and_case_variants() -> None:
# Alias for arm64 should be accepted
assert pbs_installer_supported_arch("aarch64") is True
# Case-insensitive handling of supported architectures
assert pbs_installer_supported_arch("ARM64") is True
assert pbs_installer_supported_arch("X86_64") is True
assert pbs_installer_supported_arch("AMD64") is True
def test_pbs_installer_supported_arch_rejects_invalid_strings() -> None:
# Empty and whitespace-only strings should be rejected
assert pbs_installer_supported_arch("") is False
assert pbs_installer_supported_arch(" ") is False
assert pbs_installer_supported_arch(" ") is False
```
These changes assume:
1. `os` is already imported earlier in `tests/test_helpers.py`, since `test_flatten_dict` uses `os.environ`.
2. `pbs_installer_supported_arch("aarch64")` and case variants like `"ARM64"` are expected to be treated as supported; if the helper currently does not normalize aliases/case that way, you will need to update its implementation in `tests/helpers.py` accordingly.
3. If there are additional expectations for invalid inputs (e.g., non-string types), you may want to extend `test_pbs_installer_supported_arch_rejects_invalid_strings` to cover them.
</issue_to_address>
### Comment 2
<location> `tests/test_helpers.py:57-59` </location>
<code_context>
+ assert pbs_installer_supported_arch("arm64") is True
+
+
+def test_pbs_installer_supported_arch_rejects_unsupported_archs() -> None:
+ assert pbs_installer_supported_arch("sparc") is False
+ assert pbs_installer_supported_arch("mips") is False
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding negative tests for near-miss architectures (e.g. x86, armv7l) to ensure correct rejection.
Currently the rejection test only uses clearly unsupported architectures like `"sparc"` and `"mips"`. Including realistic “near miss” values such as `"x86"`, `"i386"`, or `"armv7l"` (if they’re meant to be rejected) would better exercise the classification logic and reduce the risk of regressions in real-world scenarios.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+54
| def test_pbs_installer_supported_arch_accepts_supported_archs() -> None: | ||
| assert pbs_installer_supported_arch("x86_64") is True | ||
| assert pbs_installer_supported_arch("amd64") is True | ||
| assert pbs_installer_supported_arch("arm64") is True |
There was a problem hiding this comment.
suggestion (testing): Add tests for architecture aliases and edge cases (e.g. aarch64, case differences, empty string).
Beyond the main happy-path values, please add tests for alias and edge inputs that this helper should handle, e.g. "aarch64", case variants like "ARM64", and clearly invalid-but-string inputs such as "" or whitespace-only. That will document and lock in the expected normalization and failure behavior.
Suggested implementation:
from tests.helpers import flatten_dict
from tests.helpers import isolated_environment
from tests.helpers import pbs_installer_supported_arch
def test_flatten_dict() -> None:
with isolated_environment(environ={"NEW_VAR": "new_value"}):
assert os.environ["NEW_VAR"] == "new_value"
assert "NEW_VAR" not in os.environ
def test_pbs_installer_supported_arch_accepts_supported_archs() -> None:
assert pbs_installer_supported_arch("x86_64") is True
assert pbs_installer_supported_arch("amd64") is True
assert pbs_installer_supported_arch("arm64") is True
def test_pbs_installer_supported_arch_accepts_aliases_and_case_variants() -> None:
# Alias for arm64 should be accepted
assert pbs_installer_supported_arch("aarch64") is True
# Case-insensitive handling of supported architectures
assert pbs_installer_supported_arch("ARM64") is True
assert pbs_installer_supported_arch("X86_64") is True
assert pbs_installer_supported_arch("AMD64") is True
def test_pbs_installer_supported_arch_rejects_invalid_strings() -> None:
# Empty and whitespace-only strings should be rejected
assert pbs_installer_supported_arch("") is False
assert pbs_installer_supported_arch(" ") is False
assert pbs_installer_supported_arch(" ") is FalseThese changes assume:
osis already imported earlier intests/test_helpers.py, sincetest_flatten_dictusesos.environ.pbs_installer_supported_arch("aarch64")and case variants like"ARM64"are expected to be treated as supported; if the helper currently does not normalize aliases/case that way, you will need to update its implementation intests/helpers.pyaccordingly.- If there are additional expectations for invalid inputs (e.g., non-string types), you may want to extend
test_pbs_installer_supported_arch_rejects_invalid_stringsto cover them.
Comment on lines
+57
to
+59
| def test_pbs_installer_supported_arch_rejects_unsupported_archs() -> None: | ||
| assert pbs_installer_supported_arch("sparc") is False | ||
| assert pbs_installer_supported_arch("mips") is False |
There was a problem hiding this comment.
suggestion (testing): Consider adding negative tests for near-miss architectures (e.g. x86, armv7l) to ensure correct rejection.
Currently the rejection test only uses clearly unsupported architectures like "sparc" and "mips". Including realistic “near miss” values such as "x86", "i386", or "armv7l" (if they’re meant to be rejected) would better exercise the classification logic and reduce the risk of regressions in real-world scenarios.
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
1 participant
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.
Adds unit tests for
pbs_installer_supported_archto improve coverage oftests/helpers.py.Resolves: #9161
Summary by Sourcery
Tests: