Modifications to gnmi1.27,gnmi_sample_mode_test.go#5390
Modifications to gnmi1.27,gnmi_sample_mode_test.go#5390divyarema wants to merge 1 commit intoopenconfig:mainfrom
Conversation
Pull Request Functional Test Report for #5390 / 1c96667Virtual Devices
Hardware Devices
|
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 adjusts the sample collection count in the gNMI sample mode test suite. The change is intended to prevent intermittent test failures caused by timing issues during protocol convergence on virtualized environments. 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
|
There was a problem hiding this comment.
Code Review
This pull request increases the sample count for the ISIS protocol stream from 5 to 10 to account for convergence time. Feedback was provided regarding inconsistencies this change introduces with the test documentation and error messages, as well as an opportunity to improve test efficiency by processing samples individually rather than blocking for the full sample set.
| // Starting ISIS protocol takes some time to converge. So we may not receive ISIS data | ||
| // in the first sample after configuration. | ||
| samples := niStream.Nexts(5) | ||
| samples := niStream.Nexts(10) |
There was a problem hiding this comment.
Increasing the sample count to 10 introduces inconsistencies with the test documentation and reporting:
- README Inconsistency: The
README.md(Test 3) specifies that telemetry should be received within 5 samples. Per the repository guidelines (line 29), the code must align with the test plan. Please update the README to reflect the new requirement. - Error Message Inconsistency: The error message on line 149 (not in this diff) mentions 3 samples. This should be updated to match the code to avoid confusion during failures.
- Efficiency:
niStream.Nexts(10)blocks for the full duration of 10 intervals (approx. 110s) before processing. Consider refactoring the loop to callniStream.Next()individually to allow the test to exit as soon as the first valid sample is received, rather than always waiting for the maximum count.
References
- Code should follow the steps and requirements documented in the test README.md. (link)
Increased the no of samples collected to avoid false failures in virtual platforms