Update traffic validation to use otgutils.ExpectedTrafficLoss - #5887
Conversation
5c81c2a to
d016763
Compare
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 refactors a wide range of BGP tests to standardize traffic validation. By replacing custom, repetitive validation logic with the centralized otgutils.ExpectedTrafficLoss function, the codebase becomes cleaner and easier to maintain. This change ensures consistent behavior across different test scenarios while reducing the surface area for potential bugs in validation logic. 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 refactors multiple BGP and MPLS/SR OTG tests by replacing custom traffic loss validation logic with the standardized helper function otgutils.ExpectedTrafficLoss. This significantly simplifies the test files and improves code maintainability. The feedback identifies temporary merge artifact files (.rej and .orig) that were accidentally committed and should be deleted before merging. Additionally, a simplification is suggested for an unnecessarily complex type conversion in bgp_always_compare_med_test.go.
I am having trouble creating individual review comments. Click here to see my feedback.
feature/bgp/policybase/otg_tests/aspath_test/aspath_test.go.rej (1)
This .rej file (along with the corresponding .orig file) is a temporary artifact from a patch or merge conflict resolution. These files should not be committed to the repository. Please delete aspath_test.go.rej and aspath_test.go.orig before merging.
feature/bgp/policybase/otg_tests/community_test/community_test.go.rej (1)
This .rej file (along with the corresponding .orig file) is a temporary artifact from a patch or merge conflict resolution. These files should not be committed to the repository. Please delete community_test.go.rej and community_test.go.orig before merging.
feature/bgp/otg_tests/bgp_always_compare_med/bgp_always_compare_med_test.go (375-379)
The conversion float64(100-float32(tolerancePct)) is unnecessarily complex because tolerancePct is an untyped constant. It can be simplified directly to float64(100-tolerancePct).
if wantLoss {
otgutils.ExpectedTrafficLoss(t, otg, flowName, float64(100-tolerancePct), 100)
} else {
otgutils.ExpectedTrafficLoss(t, otg, flowName, 0, float64(tolerancePct))
}
References
- In Go test assertions, avoid over-complicating arithmetic operations with defensive type casting (e.g., casting uint32 to uint64 to prevent overflow) if the values involved (such as packet mismatch counters) are guaranteed to be extremely small in practice.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors traffic validation across multiple BGP, MPLS, and ISIS OTG tests by replacing duplicated gnmi.Watch validation blocks with the helper function otgutils.ExpectedTrafficLoss. While this significantly simplifies the codebase, the refactoring left behind unused imports of github.com/openconfig/ygnmi/ygnmi in aspath_and_community_test.go and isis_node_sid_forward_test.go, which will cause Go compilation errors. Additionally, the type conversions in bgp_always_compare_med_test.go can be simplified, and the packet counter query in isis_node_sid_forward_test.go should use gnmi.Watch to prevent flaky test failures.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors multiple OTG test files to use the helper function otgutils.ExpectedTrafficLoss instead of manually implementing gnmi.Watch loops and calculating traffic loss percentages, standardizing traffic loss verification across BGP, gRIBI, and MPLS tests. The review feedback correctly points out that some outer gnmi.Watch blocks in exrr_test.go are now redundant and should be removed, as otgutils.ExpectedTrafficLoss internally performs the same watch and validation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors multiple BGP, gRIBI, and MPLS/SR OTG tests by replacing verbose, custom traffic loss verification blocks with the standardized otgutils.ExpectedTrafficLoss helper function. This significantly reduces boilerplate code and improves test consistency. A review comment was kept for isis_node_sid_forward_test.go which identifies a potential test flakiness issue where flow counters are queried once using gnmi.Get immediately after traffic stops; the reviewer recommends wrapping this query in a gnmi.Watch loop to allow sufficient time for the counters to populate.
…nfig#5887) * Update traffic validation to use otgutils.ExpectedTrafficLoss * Update bgp_prefix_limit_test.go * Update aspath_and_community_test.go * Update isis_node_sid_forward_test.go * Update aspath_and_community_test.go * Update isis_node_sid_forward_test.go * Update bgp_always_compare_med_test.go * Standardize all traffic validation on otgutils.ExpectedTrafficLoss * Remove unused imports * Fix float32 to float64 cast in bgp_prefix_limit_test.go * Update exrr_test.go
3 participants
int(lossPct)were used, the function is updated tootgutils.ExpectedTrafficLoss(t, otg, flowName, 0, 0.99)to account for the previously truncated decimal values.