Tr13newfix#5469
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 improves the robustness and accuracy of the low power mode test suite. It introduces necessary configuration validation, refactors the optical channel setup for better clarity, and corrects a timing calculation error that previously incorrectly scaled the sampling interval. 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 updates the TestLowPowerMode function to configure optical channels directly and modifies the sampling interval logic. However, several issues were identified: removing the * time.Second multiplier introduces a timing bug as the OpenConfig value is in seconds, the new nil check for the operational mode flag is redundant as the flag pointer is never nil, and the implementation of cfgplugins.ConfigOpticalChannel violates the repository's style guide regarding function signatures and the use of gnmi.SetBatch.
| currentSamplingInterval := samplingInterval | ||
| if !deviations.SkipOpticalChannelOutputPowerInterval(dut) { | ||
| currentSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) * time.Second | ||
| currentSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) |
There was a problem hiding this comment.
Removing the * time.Second multiplier introduces a bug. In Go, time.Duration is an integer count of nanoseconds. Since the OpenConfig leaf output-power/interval typically returns a value in seconds, time.Duration(val) will result in a duration of only a few nanoseconds (e.g., 30ns instead of 30s). This will cause the samplestream to attempt sampling at an extremely high frequency, which is incorrect for transceiver power telemetry and may lead to performance issues or test failures.
| currentSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) | |
| currentSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) * time.Second |
| if operationalModeFlag != nil { | ||
| operationalMode = uint16(*operationalModeFlag) | ||
| } else { | ||
| t.Fatalf("Please specify the vendor-specific operational-mode flag") | ||
| } |
There was a problem hiding this comment.
The check if operationalModeFlag != nil is redundant. The flag.Int function returns a pointer to an integer that is allocated during package initialization and will never be nil. If the goal is to ensure that the user explicitly provided a value for this flag, consider checking if the value is non-zero (if 0 is not a valid mode) or using flag.Visit to detect if the flag was set on the command line. As currently implemented, the else block is unreachable dead code.
| cfgplugins.ConfigOpticalChannel(t, dut, och1, targetFrequencyMHz, targetOutputPowerdBm, operationalMode) | ||
| cfgplugins.ConfigOpticalChannel(t, dut, och2, targetFrequencyMHz, targetOutputPowerdBm, operationalMode) |
There was a problem hiding this comment.
The usage of cfgplugins.ConfigOpticalChannel does not adhere to the repository's style guide for configuration plugins. According to the guidelines, plugin functions should use a *gnmi.SetBatch for configuration and accept parameters via a configuration struct. The signature should follow the pattern: (t *testing.T, dut *ondatra.DUTDevice, sb *gnmi.SetBatch, cfg MyConfigStruct) *gnmi.SetBatch. Refactoring to parameterized versions helps reduce code duplication.
References
- Configuration plugin functions must follow a specific signature: (t *testing.T, dut *ondatra.DUTDevice, sb *gnmi.SetBatch, cfg MyConfigStruct) *gnmi.SetBatch. (link)
- Refactor functions to use parameterized versions to reduce code duplication.
2 participants
5207 change revert