Otg rt 5.14 - adding Cisco vendor as well#5510
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new test suite for validating aggregate subinterface behavior across different network instances. It enhances existing test infrastructure to support Cisco hardware by incorporating vendor-specific timing and configuration adjustments, while also refining metadata for Arista platform compatibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new aggregate subinterface test suite under feature/interface/aggregate/otg_tests/aggregate_subinterface_test/. The test validates aggregate interface flapping using min-links, as well as aggregate subinterfaces in both default and non-default network instances. The reviewer's feedback correctly points out several instances where dut.Vendor() is used directly for vendor-specific logic instead of abstracting these behaviors behind the deviations package, which violates the repository's style guidelines.
| if dut.Vendor() != ondatra.CISCO { | ||
| ipv4.Enabled = ygot.Bool(true) | ||
| } |
There was a problem hiding this comment.
Avoid using dut.Vendor() for vendor-specific logic. Instead, use the deviations package to maintain test abstraction and portability across different vendors. Since Arista has interface_enabled: true in its platform exceptions and Cisco does not, you can use deviations.InterfaceEnabled(dut) here to decide whether to explicitly set ipv4.Enabled.
| if dut.Vendor() != ondatra.CISCO { | |
| ipv4.Enabled = ygot.Bool(true) | |
| } | |
| if deviations.InterfaceEnabled(dut) { | |
| ipv4.Enabled = ygot.Bool(true) | |
| } |
References
- Avoid "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| if dut.Vendor() != ondatra.CISCO { | ||
| ipv6.Enabled = ygot.Bool(true) | ||
| } |
There was a problem hiding this comment.
Avoid using dut.Vendor() for vendor-specific logic. Use deviations.InterfaceEnabled(dut) here as well to determine if ipv6.Enabled should be explicitly set.
| if dut.Vendor() != ondatra.CISCO { | |
| ipv6.Enabled = ygot.Bool(true) | |
| } | |
| if deviations.InterfaceEnabled(dut) { | |
| ipv6.Enabled = ygot.Bool(true) | |
| } |
References
- Avoid "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| if dut.Vendor() == ondatra.CISCO { | ||
| gnmi.BatchReplace(mtuBatch, gnmi.OC().Interface(lagName).Mtu().Config(), uint16(mtu)) | ||
| } else { | ||
| gnmi.BatchReplace(mtuBatch, gnmi.OC().Interface(lagName).Subinterface(0).Ipv4().Mtu().Config(), mtu) | ||
| gnmi.BatchReplace(mtuBatch, gnmi.OC().Interface(lagName).Subinterface(0).Ipv6().Mtu().Config(), mtu) | ||
| } |
There was a problem hiding this comment.
Avoid using dut.Vendor() for vendor-specific configuration logic. Please define a deviation (e.g., MtuOnInterfaceLevel) to abstract whether MTU should be configured on the main interface level versus the subinterface level.
References
- Avoid "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| } | ||
| ateLACP, _ := ateVal.Val() | ||
|
|
||
| if dut.Vendor() != ondatra.CISCO { |
There was a problem hiding this comment.
Avoid using dut.Vendor() directly in the test code. Please define a new deviation in internal/deviations/deviations.go (e.g., LacpPartnerSystemIdUnsupported) and use it here to maintain test abstraction and portability across different vendors.
References
- Avoid "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
2 participants
https://github.com/openconfig/featureprofiles/blob/main/feature/interface/aggregate/otg_tests/aggregate_subinterface_test/README.md