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 found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/tasks/conductor/sonic/test_config_generator_ports.py" line_range="283-292" />
<code_context>
+
+
+class TestGetBreakoutPortValidSpeeds:
+ @pytest.mark.parametrize(
+ "port_speed, expected",
+ [
+ ("10000", "10000,1000"),
+ ("25000", "25000,10000,1000"),
+ ("50000", "50000,25000,10000,1000"),
+ ("100000", "100000,50000,25000,10000,1000"),
+ ("200000", "200000,100000,50000,25000,10000,1000"),
+ ("40000", "40000,10000,1000"),
+ ],
+ )
+ def test_known_speeds(self, port_speed, expected):
+ assert _get_breakout_port_valid_speeds(port_speed) == expected
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for `_get_breakout_port_valid_speeds` with an unsupported/non-standard speed value.
Current tests only cover known speeds; they don’t exercise or document behaviour for unexpected values (e.g. "12345", "abc"). Please add a case for an unsupported speed to assert the current behaviour (return value or exception), so future changes don’t accidentally alter this contract.
Suggested implementation:
```python
class TestGetBreakoutPortValidSpeeds:
@pytest.mark.parametrize(
"port_speed, expected",
[
("10000", "10000,1000"),
("25000", "25000,10000,1000"),
("50000", "50000,25000,10000,1000"),
("100000", "100000,50000,25000,10000,1000"),
("200000", "200000,100000,50000,25000,10000,1000"),
("40000", "40000,10000,1000"),
],
)
def test_known_speeds(self, port_speed, expected):
assert _get_breakout_port_valid_speeds(port_speed) == expected
@pytest.mark.parametrize("port_speed", ["12345", "abc"])
def test_unsupported_speeds_return_input(self, port_speed):
"""Document behaviour for non-standard/unsupported speeds."""
assert _get_breakout_port_valid_speeds(port_speed) == port_speed
```
This test assumes that `_get_breakout_port_valid_speeds` currently returns the input value unchanged for unsupported speeds. If the actual behavior is different (e.g. raising an exception or returning `None`), adjust the assertion accordingly to match the real behavior before merging.
</issue_to_address>
### Comment 2
<location path="tests/unit/tasks/conductor/sonic/test_config_generator_ports.py" line_range="834-832" />
<code_context>
+
+ assert "tagged_vlans" not in config["PORT"]["Ethernet0"]
+
+ def test_unknown_netbox_interface_skipped(self, config, device, caplog):
+ config["PORT"]["Ethernet0"] = {}
+ netbox_interfaces = {"Ethernet0": _nb_iface(netbox_name="Eth1/1")}
+ vlan_info = {
+ "vlan_members": {
+ 10: {"Eth9/9": "tagged"},
+ },
+ }
+
+ _add_tagged_vlans_to_ports(config, vlan_info, netbox_interfaces, device)
+
+ assert "tagged_vlans" not in config["PORT"]["Ethernet0"]
+
+ def test_port_in_mapping_but_absent_from_config(self, config, device):
</code_context>
<issue_to_address>
**nitpick (testing):** Either assert on the log message or drop the unused `caplog` fixture.
This test correctly verifies that unknown NetBox interface names are skipped, but `caplog` is never used. If `_add_tagged_vlans_to_ports` logs when skipping an unknown interface, consider asserting on that log entry; otherwise, remove `caplog` from the test signature to keep it minimal and clear.
</issue_to_address>
### Comment 3
<location path="tests/unit/tasks/conductor/sonic/test_config_generator_ports.py" line_range="1214-1215" />
<code_context>
+ def test_one_portchannel_with_two_members(self, config):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider an additional test for a port-channel with no members (non-empty `portchannels` but empty `members` list).
Right now we only cover a populated port-channel and the global no-op case where `portchannels` is empty. Please also add a case where a port-channel exists but its `members` list is empty, to clarify whether `PORTCHANNEL` and `PORTCHANNEL_INTERFACE` are still created while `PORTCHANNEL_MEMBER` stays empty, and to guard against future assumptions that `members` is always non-empty.
```suggestion
class TestAddPortchannelConfiguration:
def test_portchannel_with_no_members(self, config):
portchannel_info = {
"portchannels": {
"PortChannel1": {
"admin_status": "up",
"fast_rate": "false",
"min_links": "1",
"mtu": "9100",
"members": [],
},
},
}
_add_portchannel_configuration(config, portchannel_info)
# A port-channel with an explicitly empty members list should not
# populate PORTCHANNEL_MEMBER. This guards against assuming members
# is always non-empty.
assert "PORTCHANNEL_MEMBER" not in config or config["PORTCHANNEL_MEMBER"] == {}
def test_one_portchannel_with_two_members(self, config):
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @pytest.mark.parametrize( | ||
| "brkout_mode, expected_speed", | ||
| [ | ||
| ("4x10G", "10000"), | ||
| ("4x25G", "25000"), | ||
| ("4x50G", "50000"), | ||
| ("4x100G", "100000"), | ||
| ("4x200G", "200000"), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Add a test for _get_breakout_port_valid_speeds with an unsupported/non-standard speed value.
Current tests only cover known speeds; they don’t exercise or document behaviour for unexpected values (e.g. "12345", "abc"). Please add a case for an unsupported speed to assert the current behaviour (return value or exception), so future changes don’t accidentally alter this contract.
Suggested implementation:
class TestGetBreakoutPortValidSpeeds:
@pytest.mark.parametrize(
"port_speed, expected",
[
("10000", "10000,1000"),
("25000", "25000,10000,1000"),
("50000", "50000,25000,10000,1000"),
("100000", "100000,50000,25000,10000,1000"),
("200000", "200000,100000,50000,25000,10000,1000"),
("40000", "40000,10000,1000"),
],
)
def test_known_speeds(self, port_speed, expected):
assert _get_breakout_port_valid_speeds(port_speed) == expected
@pytest.mark.parametrize("port_speed", ["12345", "abc"])
def test_unsupported_speeds_return_input(self, port_speed):
"""Document behaviour for non-standard/unsupported speeds."""
assert _get_breakout_port_valid_speeds(port_speed) == port_speedThis test assumes that _get_breakout_port_valid_speeds currently returns the input value unchanged for unsupported speeds. If the actual behavior is different (e.g. raising an exception or returning None), adjust the assertion accordingly to match the real behavior before merging.
|
|
||
| _add_tagged_vlans_to_ports(config, vlan_info, netbox_interfaces, device) | ||
|
|
||
| assert "tagged_vlans" not in config["PORT"]["Ethernet0"] |
There was a problem hiding this comment.
nitpick (testing): Either assert on the log message or drop the unused caplog fixture.
This test correctly verifies that unknown NetBox interface names are skipped, but caplog is never used. If _add_tagged_vlans_to_ports logs when skipping an unknown interface, consider asserting on that log entry; otherwise, remove caplog from the test signature to keep it minimal and clear.
| class TestAddPortchannelConfiguration: | ||
| def test_one_portchannel_with_two_members(self, config): |
There was a problem hiding this comment.
suggestion (testing): Consider an additional test for a port-channel with no members (non-empty portchannels but empty members list).
Right now we only cover a populated port-channel and the global no-op case where portchannels is empty. Please also add a case where a port-channel exists but its members list is empty, to clarify whether PORTCHANNEL and PORTCHANNEL_INTERFACE are still created while PORTCHANNEL_MEMBER stays empty, and to guard against future assumptions that members is always non-empty.
| class TestAddPortchannelConfiguration: | |
| def test_one_portchannel_with_two_members(self, config): | |
| class TestAddPortchannelConfiguration: | |
| def test_portchannel_with_no_members(self, config): | |
| portchannel_info = { | |
| "portchannels": { | |
| "PortChannel1": { | |
| "admin_status": "up", | |
| "fast_rate": "false", | |
| "min_links": "1", | |
| "mtu": "9100", | |
| "members": [], | |
| }, | |
| }, | |
| } | |
| _add_portchannel_configuration(config, portchannel_info) | |
| # A port-channel with an explicitly empty members list should not | |
| # populate PORTCHANNEL_MEMBER. This guards against assuming members | |
| # is always non-empty. | |
| assert "PORTCHANNEL_MEMBER" not in config or config["PORTCHANNEL_MEMBER"] == {} | |
| def test_one_portchannel_with_two_members(self, config): |
| assert config["PORT"]["Ethernet1"] == {"sentinel": True} | ||
| alias.assert_not_called() | ||
|
|
||
| def test_speed_from_netbox_no_kbps_conversion(self, config, mocker): |
There was a problem hiding this comment.
Correctness bug — _add_missing_breakout_ports skips kbps→Mbps conversion
This test documents the inconsistency rather than catching it. _add_port_configurations divides NetBox speed by 1000 (kbps → Mbps) at config_generator.py:400; _add_missing_breakout_ports skips the conversion at config_generator.py:632 and writes the raw NetBox value. With realistic NetBox units (e.g. 25 G = 25 000 000 kbps), the missing-breakout path would emit "25000000" instead of "25000" — an obvious wrong value. The test value of 25 000 kbps (= 25 Mbps) happens to produce "25000", which looks plausible, masking the severity. The fix is to add the same // 1000 conversion in _add_missing_breakout_ports and update this test to use realistic units and expect the converted result.
| assert config["PORT"]["Ethernet1"]["speed"] == "25000" | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "brkout_mode, expected_speed", |
There was a problem hiding this comment.
Correctness bug + missing test — "4x10G" brkout_mode not handled in _add_missing_breakout_ports
_add_port_configurations handles "10G" at config_generator.py:432; _add_missing_breakout_ports starts its chain at "25G" (line 638) with no 10G branch, so a missing breakout port whose master has brkout_mode="4x10G" silently defaults to port_speed = "25000" instead of "10000".
This parametrize covers 4x25G / 4x50G / 4x100G / 4x200G / unknown but omits 4x10G. Adding ("4x10G", "10000") here would fail immediately and expose the production bug.
|
|
||
|
|
||
| class TestAddPortConfigurations: | ||
| def test_ports_processed_in_natural_sort_order( |
There was a problem hiding this comment.
Missing/weak test (low severity) — alias call signature not asserted in TestAddPortConfigurations
TestAddMissingBreakoutPorts.test_alias_called_with_breakout_flag verifies the exact arguments passed to convert_sonic_interface_to_alias. No equivalent check exists here: all mocks in this class use return_value or a catch-all lambda, so a regression in the convert_sonic_interface_to_alias call at config_generator.py:451 (wrong speed, wrong is_breakout_port flag, missing port_config) would not be caught.
Covers the port / interface / portchannel / breakout helpers in osism/tasks/conductor/sonic/config_generator.py: - _add_port_configurations: natural sort, master-port skip, admin_status, NetBox speed override (explicit vs derived, kbps→Mbps), breakout speed (NetBox + brkout_mode fallback), index propagation from master, default port_data shape, valid_speeds branches, and the post-loop _add_missing_breakout_ports / _add_tagged_vlans_to_ports invocations. - _get_breakout_port_valid_speeds: known speeds and falsy inputs. - _calculate_breakout_port_lane: 4-lane and 8-lane breakouts, range syntax, single-lane master, unexpected counts, out-of-bounds and regex no-match fallbacks. - _add_missing_breakout_ports: skip-existing, NetBox speed kbps→Mbps conversion, brkout_mode fallback (incl. default), admin state, master index copy, valid_speeds override, alias kwargs. - _add_tagged_vlans_to_ports: numeric VLAN ID sort, untagged ignored, unknown NetBox name skipped, port absent from PORT silently skipped. - _add_interface_configurations: IPv4 base + suffixed entry, IPv6 link-local fallback, port-channel-member skip, disconnected skip, missing netbox_interfaces entry. - _get_transfer_role_ipv4_addresses: happy path, dedup per interface, IPv6 ignored, mgmt/virtual filtered, invalid prefix and IP swallowed, IP without assigned_object_id, unknown id, top-level exception. - _has_direct_ipv4_address, _has_transfer_role_ipv4, _is_untagged_vlan_member: happy path and empty-input early returns. - _add_portchannel_configuration: populated and empty cases. Also fixes a kbps→Mbps conversion bug in _add_missing_breakout_ports (flagged in review): the helper previously wrote the raw NetBox kbps value, while _add_port_configurations divides by 1000. With realistic NetBox units (e.g. 25 G = 25 000 000 kbps) this would emit "25000000" instead of "25000". The conversion is now consistent across both code paths, and the test exercises realistic units. Closes #2222 AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
ideaship
left a comment
ideaship
left a comment
There was a problem hiding this comment.
One correctness bug and one weak test have not been addressed. I do not see any comments on this, so I am assuming this was an oversight.
3 participants
Covers the port / interface / portchannel / breakout helpers in osism/tasks/conductor/sonic/config_generator.py:
Also fixes a kbps→Mbps conversion bug in _add_missing_breakout_ports (flagged in review): the helper previously wrote the raw NetBox kbps value, while _add_port_configurations divides by 1000. With realistic NetBox units (e.g. 25 G = 25 000 000 kbps) this would emit "25000000" instead of "25000". The conversion is now consistent across both code paths, and the test exercises realistic units.
Closes #2222
AI-assisted: Claude Code