Update route_propagation_test.go #4976
Conversation
Pull Request Functional Test Report for #4976 / 007f52dVirtual Devices
Hardware Devices
|
Summary of ChangesHello @AmrNJ, 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 addresses flakiness in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 addresses test flakiness by replacing gnmi.WatchAll with a polling loop to wait for BGP prefixes. While this fixes the immediate issue, the polling implementation is inefficient and goes against the spirit of the repository's testing guidelines, which recommend using gnmi.Watch for waiting on conditions.
My review includes suggestions to refactor the prefix checking functions (checkOTGBGP4Prefix and checkOTGBGP6Prefix) to use gnmi.Watch on a specific prefix path. This approach is more efficient as it avoids continuous polling and is more idiomatic with the Ondatra testing framework.
feature/bgp/addpath/otg_tests/route_propagation_test/route_propagation_test.go
Show resolved
Hide resolved
feature/bgp/addpath/otg_tests/route_propagation_test/route_propagation_test.go
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 20781664684Details
💛 - Coveralls |
| found := false | ||
| if ok { | ||
| start := time.Now() | ||
| for time.Since(start) < 2*time.Minute { |
There was a problem hiding this comment.
May I request to know how we arrive at 2 minutes for all data routes to comes in ?
There was a problem hiding this comment.
We noticed test being flaky with 1 minute. The 2 minute wait time gives consistent passes. The await loop would also terminate (before 2 minutes) when all prefixes are received.
5 participants
Address code flakiness by removing gnmi.WatchAll which terminates the wait time as soon as just one entry is received instead of waiting for all the prefixes to be learnt.