Conversation
![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 left some high level feedback:
- Several of the constants tests (e.g., in
test_constants.py) repeat the same assertion pattern over different values and could be made more concise and maintainable usingpytest.mark.parametrize. - The multiple
calculate_local_asn_from_ipv4invalid-input tests intest_bgp.pyshare a lot of boilerplate and could also benefit from parametrization over the invalid input and expected error pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several of the constants tests (e.g., in `test_constants.py`) repeat the same assertion pattern over different values and could be made more concise and maintainable using `pytest.mark.parametrize`.
- The multiple `calculate_local_asn_from_ipv4` invalid-input tests in `test_bgp.py` share a lot of boilerplate and could also benefit from parametrization over the invalid input and expected error pattern.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
berendt
force-pushed
the
gh2198
branch
2 times, most recently
from
b3739b4 to
48c9f0c
Compare
- bgp.py: calculate_local_asn_from_ipv4 (docstring example, CIDR stripping, custom prefix, zero-padding, boundary octets, invalid formats), calculate_minimum_as_for_group (multiple/empty groups, invalid IPs skipped with logger.debug, None/empty primary_ip4 silently skipped, custom prefix, str() conversion of non-str IPs), and find_interconnected_spine_groups delegation to connections.find_interconnected_devices. - constants.py: invariants of module-level tables (default values, sorted-and-deduped DEFAULT_SONIC_ROLES, sample port-speed mappings across families, HIGH_SPEED_PORTS consistency with PORT_TYPE_TO_SPEED_MAP, HWSKU prefix and uniqueness). Importing these tests triggers osism/tasks/conductor/__init__, which transitively loads osism/tasks/conductor/utils.py with its module-level ``from ansible import ...`` statements. ansible-core lives in the optional ``[ansible]`` extra and is not installed in the unit-test environment, so tests/conftest.py registers lightweight sys.modules stubs for ``ansible``, ``ansible.constants``, ``ansible.errors`` and ``ansible.parsing.vault`` before collection. The stubs only satisfy the import statements and are skipped when the real package is installed. Closes #2198 AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
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.
Covers both modules with 100% statement coverage:
Closes #2198
AI-assisted: Claude Code