Adding 50 extra seconds to account for the following variables:#5252
Adding 50 extra seconds to account for the following variables:#5252fmolinar wants to merge 1 commit intoopenconfig:mainfrom
Conversation
timeout ≥ hold_timer + keepalive_interval + 30s_margin = 90 + 30 + 30 = 150s.
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 enhances the reliability of BGP session parameter tests by extending a critical timeout duration. This adjustment ensures that the test environment can adequately account for network propagation delays and other variables, preventing premature test failures and improving the accuracy of BGP session state verification. 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 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 counter productive. 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
|
Pull Request Functional Test Report for #5252 / cf7aa8bVirtual Devices
Hardware Devices
|
![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 increases the timeout for a BGP session state check in TestPassword to prevent test flakiness. The change increases the margin from 10 to 60 seconds over the BGP hold time. While the new timeout value is correct, the implementation uses a magic number. My feedback suggests replacing this with an expression using existing constants to improve code clarity and maintainability, aligning with the logic provided in the pull request description.
| } | ||
| t.Log("Wait till hold time expires: BGP should not be in ESTABLISHED state when passwords do not match.") | ||
| _, ok := gnmi.Watch(t, dut, nbrPath.SessionState().State(), (dutHoldTime+10)*time.Second, func(val *ygnmi.Value[oc.E_Bgp_Neighbor_SessionState]) bool { | ||
| _, ok := gnmi.Watch(t, dut, nbrPath.SessionState().State(), (dutHoldTime+60)*time.Second, func(val *ygnmi.Value[oc.E_Bgp_Neighbor_SessionState]) bool { |
There was a problem hiding this comment.
While the increased timeout of 150 seconds (dutHoldTime of 90s + 60s) is correct according to the pull request description, using the magic number 60 makes the code harder to understand and maintain. The description explains this margin as keepalive_interval + 30s_margin. Since dutKeepaliveTime is 30s, this margin is equivalent to 2 * dutKeepaliveTime. Using this expression makes the intent clearer and ties the timeout to the relevant BGP parameters.
| _, ok := gnmi.Watch(t, dut, nbrPath.SessionState().State(), (dutHoldTime+60)*time.Second, func(val *ygnmi.Value[oc.E_Bgp_Neighbor_SessionState]) bool { | |
| _, ok := gnmi.Watch(t, dut, nbrPath.SessionState().State(), (dutHoldTime+2*dutKeepaliveTime)*time.Second, func(val *ygnmi.Value[oc.E_Bgp_Neighbor_SessionState]) bool { |
2 participants
This provides a 60-second margin over the hold timer, easily accommodating the propagation pipeline. Formula: timeout ≥ hold_timer + keepalive_interval + 30s_margin = 90 + 30 + 30 = 150s.