TRANSCEIVER-5.2: Restructured using functions from internal library#4908
TRANSCEIVER-5.2: Restructured using functions from internal library#4908snaragund wants to merge 11 commits intoopenconfig:mainfrom
Conversation
…library
-Defined `TunableParamsTest` which validates optical-channel telemetry.
-Added 3 more conditions in `isOpticalChannelStreamReady` func.
-Added required deviations in metadata.textproto.
-Used `tunableParamsTest` func to restructure zrp_tunable_parameters_test.
"This code is a Contribution to the OpenConfig Feature Profiles project ("Work") made under the Google Software Grant and Corporate Contributor License Agreement ("CLA") and governed by the Apache License 2.0. No other rights or licenses in or to any of Nokia's intellectual property are granted for any other purpose. This code is provided on an "as is" basis without any warranties of any kind."
snaragund
changed the title
Summary of ChangesHello @snaragund, 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 significantly refactors the optical transceiver test suite by introducing a new, reusable 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. 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 refactors the zrp_tunable_parameters_test.go by extracting common configuration and telemetry validation logic into a new helper function, TunableParamsTest, located in internal/telemetry/transceiver/zr_validation.go. The original test file now utilizes this new helper, leading to the removal of several unused imports and the Test400ZRPlusInterfaceFlap function. The metadata.textproto file was updated to include Nokia-specific platform exceptions for interface_enabled and explicit_dco_config. Additionally, optical_channel_validation.go received minor updates to add new telemetry checks for output power and chromatic dispersion. The review comments suggest further improvements to the TunableParamsTest helper function, recommending passing the dut object as a parameter for explicit dependency management and reducing code duplication by introducing a local helper for telemetry validation.
| func TunableParamsTest(t *testing.T, tp *TunableParams) { | ||
| t.Helper() | ||
|
|
||
| dut := ondatra.DUT(t, "dut") | ||
|
|
||
| fptest.ConfigureDefaultNetworkInstance(t, dut) | ||
|
|
||
| t.Logf("\n*** Configure interfaces with Operational Mode: %v, Optical Frequency: %v, Target Power: %v\n\n\n", tp.OpMode, tp.Freq, tp.OutputPower) | ||
| params := &cfgplugins.ConfigParameters{ | ||
| Enabled: true, | ||
| Frequency: tp.Freq, | ||
| TargetOpticalPower: tp.OutputPower, | ||
| OperationalMode: tp.OpMode, | ||
| } | ||
| batch := &gnmi.SetBatch{} | ||
| cfgplugins.NewInterfaceConfigAll(t, dut, batch, params) | ||
| batch.Set(t, dut) | ||
|
|
||
| // Create sample steams for each port. | ||
| ochStreams := make(map[string]*samplestream.SampleStream[*oc.Component]) | ||
| interfaceStreams := make(map[string]*samplestream.SampleStream[*oc.Interface]) | ||
| for _, p := range dut.Ports() { | ||
| ochStreams[p.Name()] = samplestream.New(t, dut, gnmi.OC().Component(params.OpticalChannelNames[p.Name()]).State(), samplingInterval) | ||
| interfaceStreams[p.Name()] = samplestream.New(t, dut, gnmi.OC().Interface(p.Name()).State(), samplingInterval) | ||
| defer ochStreams[p.Name()].Close() | ||
| defer interfaceStreams[p.Name()].Close() | ||
| } | ||
| for _, p := range dut.Ports() { | ||
| validateInterfaceTelemetry(t, dut, p, params, oc.Interface_OperStatus_UP, interfaceStreams[p.Name()]) | ||
| validateOpticalChannelTelemetry(t, p, params, oc.Interface_OperStatus_UP, ochStreams[p.Name()]) | ||
| } | ||
|
|
||
| t.Logf("\n*** Bringing DOWN all interfaces\n\n\n") | ||
| for _, p := range dut.Ports() { | ||
| params.Enabled = false | ||
| cfgplugins.ToggleInterfaceState(t, dut, p, params) | ||
| } | ||
| for _, p := range dut.Ports() { | ||
| validateInterfaceTelemetry(t, dut, p, params, oc.Interface_OperStatus_DOWN, interfaceStreams[p.Name()]) | ||
| validateOpticalChannelTelemetry(t, p, params, oc.Interface_OperStatus_DOWN, ochStreams[p.Name()]) | ||
| } | ||
|
|
||
| t.Logf("\n*** Bringing UP all interfaces\n\n\n") | ||
| for _, p := range dut.Ports() { | ||
| params.Enabled = true | ||
| cfgplugins.ToggleInterfaceState(t, dut, p, params) | ||
| } | ||
| for _, p := range dut.Ports() { | ||
| validateInterfaceTelemetry(t, dut, p, params, oc.Interface_OperStatus_UP, interfaceStreams[p.Name()]) | ||
| validateOpticalChannelTelemetry(t, p, params, oc.Interface_OperStatus_UP, ochStreams[p.Name()]) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a great helper function that encapsulates the tunable parameter testing logic. Here are a couple of suggestions to make it even more robust and maintainable:
- Pass
dutas a parameter: Instead of retrieving thedutobject internally, consider passing it as a parameter. This makes the function's dependencies explicit and avoids repeated lookups. - Reduce code duplication: The validation logic for all ports is repeated three times. This can be extracted into a local helper function to improve readability.
Here is a suggested implementation incorporating these changes. Note that you will also need to update the call sites in zrp_tunable_parameters_test.go to pass the dut object.
func TunableParamsTest(t *testing.T, dut *ondatra.DUTDevice, tp *TunableParams) {
t.Helper()
fptest.ConfigureDefaultNetworkInstance(t, dut)
t.Logf("\n*** Configure interfaces with Operational Mode: %v, Optical Frequency: %v, Target Power: %v\n\n\n", tp.OpMode, tp.Freq, tp.OutputPower)
params := &cfgplugins.ConfigParameters{
Enabled: true,
Frequency: tp.Freq,
TargetOpticalPower: tp.OutputPower,
OperationalMode: tp.OpMode,
}
batch := &gnmi.SetBatch{}
cfgplugins.NewInterfaceConfigAll(t, dut, batch, params)
batch.Set(t, dut)
// Create sample steams for each port.
ochStreams := make(map[string]*samplestream.SampleStream[*oc.Component])
interfaceStreams := make(map[string]*samplestream.SampleStream[*oc.Interface])
for _, p := range dut.Ports() {
ochStreams[p.Name()] = samplestream.New(t, dut, gnmi.OC().Component(params.OpticalChannelNames[p.Name()]).State(), samplingInterval)
interfaceStreams[p.Name()] = samplestream.New(t, dut, gnmi.OC().Interface(p.Name()).State(), samplingInterval)
defer ochStreams[p.Name()].Close()
defer interfaceStreams[p.Name()].Close()
}
// Helper function to validate telemetry for all ports.
validateAllPorts := func(wantStatus oc.E_Interface_OperStatus) {
t.Helper()
for _, p := range dut.Ports() {
validateInterfaceTelemetry(t, dut, p, params, wantStatus, interfaceStreams[p.Name()])
validateOpticalChannelTelemetry(t, p, params, wantStatus, ochStreams[p.Name()])
}
}
validateAllPorts(oc.Interface_OperStatus_UP)
t.Logf("\n*** Bringing DOWN all interfaces\n\n\n")
for _, p := range dut.Ports() {
params.Enabled = false
cfgplugins.ToggleInterfaceState(t, dut, p, params)
}
validateAllPorts(oc.Interface_OperStatus_DOWN)
t.Logf("\n*** Bringing UP all interfaces\n\n\n")
for _, p := range dut.Ports() {
params.Enabled = true
cfgplugins.ToggleInterfaceState(t, dut, p, params)
}
validateAllPorts(oc.Interface_OperStatus_UP)
}
Pull Request Test Coverage Report for Build 21852422973Details
💛 - Coveralls |
snaragund
changed the title
|
retriggering github checks |
Please fix static checks Please provide link to test results. Can either use a github gist or privately. |
Fix merge conflict
"This code is a Contribution to the OpenConfig Feature Profiles project ("Work") made under the Google Software Grant and Corporate Contributor License Agreement ("CLA") and governed by the Apache License 2.0. No other rights or licenses in or to any of Nokia's intellectual property are granted for any other purpose. This code is provided on an "as is" basis without any warranties of any kind."
6 participants
TunableParamsTestwhich validates optical-channel telemetry.isOpticalChannelStreamReadyfunc.tunableParamsTestfunc to restructure zrp_tunable_parameters_test."This code is a Contribution to the OpenConfig Feature Profiles project ("Work") made under the Google Software Grant and Corporate Contributor License Agreement ("CLA") and governed by the Apache License 2.0. No other rights or licenses in or to any of Nokia's intellectual property are granted for any other purpose. This code is provided on an "as is" basis without any warranties of any kind."