TRANSCEIVER-7.2 & 9.2 fix #4772
Conversation
- Deviation `explicit_dco_config`, `transceiver_config_enable_unsupported` & `missing_zr_optical_channel_tunable_parameters_telemetry` added.
- TransceiveronOff sub-test is been skipped as Nokia dont support transceiver/config/enable leaf.
"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."
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 enhances the robustness of transceiver-related tests by introducing platform-specific deviations for Nokia devices. It ensures that tests gracefully handle features not supported by Nokia hardware, preventing unnecessary failures and improving test suite reliability. Additionally, the changes include a refactoring of test setup procedures to make them more efficient and consistent across different test cases. 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 introduces Nokia-specific deviations for transceiver tests and refactors the test setup logic. While the changes correctly use deviations to handle vendor-specific behavior, there are several areas for improvement. A critical bug has been introduced in zrp_inventory_test.go where logic for the second port is commented out, leaving the test incomplete. Additionally, a hardcoded time.Sleep in zrp_laser_bias_current_test.go should be replaced with a more robust gnmi.Await. There is also significant code duplication for interface setup across multiple test functions in both modified Go files, which should be refactored into a helper function to improve maintainability. Lastly, one test case silently passes when skipped via deviation, whereas it should explicitly be marked as skipped for clarity.
| cfgplugins.InterfaceConfig(t, dut, dp1) | ||
| cfgplugins.InterfaceConfig(t, dut, dp2) | ||
| tr1 := gnmi.Get(t, dut, gnmi.OC().Interface(dp1.Name()).Transceiver().State()) | ||
| //tr2 := gnmi.Get(t, dut, gnmi.OC().Interface(dp2.Name()).Transceiver().State()) |
There was a problem hiding this comment.
The definition for tr2 has been commented out. This variable is needed to re-enable the transceiver on the second port later in the test (line 204). Please uncomment this line to fix the test logic.
| //tr2 := gnmi.Get(t, dut, gnmi.OC().Interface(dp2.Name()).Transceiver().State()) | |
| tr2 := gnmi.Get(t, dut, gnmi.OC().Interface(dp2.Name()).Transceiver().State()) |
| gnmi.Update(t, dut, gnmi.OC().Component(dp1.Name()).Name().Config(), dp1.Name()) | ||
| gnmi.Update(t, dut, gnmi.OC().Component(tr1).Transceiver().Enabled().Config(), true) | ||
| gnmi.Update(t, dut, gnmi.OC().Component(dp2.Name()).Name().Config(), dp2.Name()) | ||
| //gnmi.Update(t, dut, gnmi.OC().Component(tr2).Transceiver().Enabled().Config(), true) |
There was a problem hiding this comment.
The logic to re-enable the transceiver for the second port (dp2) has been commented out. This prevents the second interface from coming back up, which contradicts the test's intent and the log message on line 209. Please uncomment this line to restore the correct behavior.
| //gnmi.Update(t, dut, gnmi.OC().Component(tr2).Transceiver().Enabled().Config(), true) | |
| gnmi.Update(t, dut, gnmi.OC().Component(tr2).Transceiver().Enabled().Config(), true) |
| gnmi.Await(t, dut1, gnmi.OC().Interface(dp1.Name()).OperStatus().State(), intUpdateTime, oc.Interface_OperStatus_DOWN) | ||
| t.Logf("%v operational status is: %v", dp1.Name(), gnmi.Get(t, dut1, gnmi.OC().Interface(dp1.Name()).OperStatus().State())) | ||
| t.Log("Wait to update telemetry") | ||
| time.Sleep(80 * time.Second) |
There was a problem hiding this comment.
Using a fixed time.Sleep makes the test brittle and unnecessarily slow. The comment "Wait to update telemetry" indicates a wait for a state change. It is much better practice to use gnmi.Await to poll for the specific condition you are waiting for. This makes the test more robust and efficient.
| dut := ondatra.DUT(t, "dut") | ||
| dp1 := dut.Port(t, "port1") | ||
| dp2 := dut.Port(t, "port2") | ||
| fptest.ConfigureDefaultNetworkInstance(t, dut) |
There was a problem hiding this comment.
This call to fptest.ConfigureDefaultNetworkInstance(t, dut) is redundant, as the same function is called again on line 82 within this test. Please remove this duplicate call to improve code clarity.
| dp1 := dut.Port(t, "port1") | ||
| dp2 := dut.Port(t, "port2") | ||
| fptest.ConfigureDefaultNetworkInstance(t, dut) | ||
| operationalMode = uint16(*operationalModeFlag) | ||
| cfgplugins.InterfaceInitialize(t, dut, operationalMode) | ||
| cfgplugins.InterfaceConfig(t, dut, dp1) | ||
| cfgplugins.InterfaceConfig(t, dut, dp2) |
There was a problem hiding this comment.
There is significant code duplication for interface and port setup between TestInventoryInterfaceFlap (lines 70-77) and TestInventoryTransceiverOnOff (lines 142-148). Consider creating a helper function to encapsulate this common setup logic. This would improve maintainability and reduce redundancy.
| func TestZRLaserBiasCurrentStateTransceiverOnOff(t *testing.T) { | ||
| if operationalModeFlag != nil { | ||
| dut1 := ondatra.DUT(t, "dut") | ||
| if !deviations.TransceiverConfigEnableUnsupported(dut1) { | ||
| dp1 := dut1.Port(t, "port1") | ||
| dp2 := dut1.Port(t, "port2") | ||
| t.Logf("dut1: %v", dut1) | ||
| t.Logf("dut1 dp1 name: %v", dp1.Name()) | ||
| operationalMode = uint16(*operationalModeFlag) | ||
| } else { | ||
| t.Fatalf("Please specify the vendor-specific operational-mode flag") | ||
| operationalMode = cfgplugins.InterfaceInitialize(t, dut1, operationalMode) | ||
| cfgplugins.InterfaceConfig(t, dut1, dp1) | ||
| cfgplugins.InterfaceConfig(t, dut1, dp2) | ||
| gnmi.Await(t, dut1, gnmi.OC().Interface(dp1.Name()).OperStatus().State(), intUpdateTime, oc.Interface_OperStatus_UP) | ||
| transceiverState := gnmi.Get(t, dut1, gnmi.OC().Interface(dp1.Name()).Transceiver().State()) | ||
| // Check if TRANSCEIVER is of type 400ZR_PLUS | ||
| // Uncomment once the Ondatra OC release version is fixed. | ||
| // if dp1.PMD() != ondatra.PMD400GBASEZRP { | ||
| // t.Fatalf("%s Transceiver is not 400ZR_PLUS its of type: %v", transceiverState, dp1.PMD()) | ||
| // } | ||
| componentName := components.OpticalChannelComponentFromPort(t, dut1, dp1) | ||
| component := gnmi.OC().Component(componentName) | ||
| p1Stream := samplestream.New(t, dut1, component.OpticalChannel().LaserBiasCurrent().State(), 10*time.Second) | ||
| defer p1Stream.Close() | ||
| verifyLaserBiasCurrentAll(t, p1Stream, dut1) | ||
| // power off interface transceiver | ||
| gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Name().Config(), transceiverState) | ||
| // for transceiver disable, the input needs to be the transceiver name instead of the interface name | ||
| gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Transceiver().Enabled().Config(), false) | ||
| // Cannot use Interface_OperStatus_DOWN here as the interface is "Not Present" | ||
| verifyLaserBiasCurrentAll(t, p1Stream, dut1) | ||
| // power on interface transceiver | ||
| gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Transceiver().Enabled().Config(), true) | ||
| gnmi.Await(t, dut1, gnmi.OC().Interface(dp1.Name()).OperStatus().State(), intUpdateTime, oc.Interface_OperStatus_UP) | ||
| verifyLaserBiasCurrentAll(t, p1Stream, dut1) | ||
| } | ||
| dut1 := ondatra.DUT(t, "dut") | ||
| dp1 := dut1.Port(t, "port1") | ||
| dp2 := dut1.Port(t, "port2") | ||
| t.Logf("dut1: %v", dut1) | ||
| t.Logf("dut1 dp1 name: %v", dp1.Name()) | ||
| och1 := components.OpticalChannelComponentFromPort(t, dut1, dp1) | ||
| och2 := components.OpticalChannelComponentFromPort(t, dut1, dp2) | ||
| cfgplugins.ConfigOpticalChannel(t, dut1, och1, frequency, targetOutputPower, operationalMode) | ||
| cfgplugins.ConfigOpticalChannel(t, dut1, och2, frequency, targetOutputPower, operationalMode) | ||
| gnmi.Await(t, dut1, gnmi.OC().Interface(dp1.Name()).OperStatus().State(), intUpdateTime, oc.Interface_OperStatus_UP) | ||
| transceiverState := gnmi.Get(t, dut1, gnmi.OC().Interface(dp1.Name()).Transceiver().State()) | ||
| // Check if TRANSCEIVER is of type 400ZR_PLUS | ||
| // Uncomment once the Ondatra OC release version is fixed. | ||
| // if dp1.PMD() != ondatra.PMD400GBASEZRP { | ||
| // t.Fatalf("%s Transceiver is not 400ZR_PLUS its of type: %v", transceiverState, dp1.PMD()) | ||
| // } | ||
| componentName := components.OpticalChannelComponentFromPort(t, dut1, dp1) | ||
| component := gnmi.OC().Component(componentName) | ||
| p1Stream := samplestream.New(t, dut1, component.OpticalChannel().LaserBiasCurrent().State(), 10*time.Second) | ||
| defer p1Stream.Close() | ||
| verifyLaserBiasCurrentAll(t, p1Stream, dut1) | ||
| // power off interface transceiver | ||
| gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Name().Config(), transceiverState) | ||
| // for transceiver disable, the input needs to be the transceiver name instead of the interface name | ||
| gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Transceiver().Enabled().Config(), false) | ||
| // Cannot use Interface_OperStatus_DOWN here as the interface is "Not Present" | ||
| verifyLaserBiasCurrentAll(t, p1Stream, dut1) | ||
| // power on interface transceiver | ||
| gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Transceiver().Enabled().Config(), true) | ||
| gnmi.Await(t, dut1, gnmi.OC().Interface(dp1.Name()).OperStatus().State(), intUpdateTime, oc.Interface_OperStatus_UP) | ||
| verifyLaserBiasCurrentAll(t, p1Stream, dut1) | ||
| } |
There was a problem hiding this comment.
When deviations.TransceiverConfigEnableUnsupported(dut1) is true, this test function completes without running any checks or indicating that it was skipped. This can be misleading. It's better to explicitly skip the test in an else block using t.Skip() to make the test results clearer, as is done in zrp_inventory_test.go.
func TestZRLaserBiasCurrentStateTransceiverOnOff(t *testing.T) {
dut1 := ondatra.DUT(t, "dut")
if !deviations.TransceiverConfigEnableUnsupported(dut1) {
dp1 := dut1.Port(t, "port1")
dp2 := dut1.Port(t, "port2")
t.Logf("dut1: %v", dut1)
t.Logf("dut1 dp1 name: %v", dp1.Name())
operationalMode = uint16(*operationalModeFlag)
operationalMode = cfgplugins.InterfaceInitialize(t, dut1, operationalMode)
cfgplugins.InterfaceConfig(t, dut1, dp1)
cfgplugins.InterfaceConfig(t, dut1, dp2)
gnmi.Await(t, dut1, gnmi.OC().Interface(dp1.Name()).OperStatus().State(), intUpdateTime, oc.Interface_OperStatus_UP)
transceiverState := gnmi.Get(t, dut1, gnmi.OC().Interface(dp1.Name()).Transceiver().State())
// Check if TRANSCEIVER is of type 400ZR_PLUS
// Uncomment once the Ondatra OC release version is fixed.
// if dp1.PMD() != ondatra.PMD400GBASEZRP {
// t.Fatalf("%s Transceiver is not 400ZR_PLUS its of type: %v", transceiverState, dp1.PMD())
// }
componentName := components.OpticalChannelComponentFromPort(t, dut1, dp1)
component := gnmi.OC().Component(componentName)
p1Stream := samplestream.New(t, dut1, component.OpticalChannel().LaserBiasCurrent().State(), 10*time.Second)
defer p1Stream.Close()
verifyLaserBiasCurrentAll(t, p1Stream, dut1)
// power off interface transceiver
gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Name().Config(), transceiverState)
// for transceiver disable, the input needs to be the transceiver name instead of the interface name
gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Transceiver().Enabled().Config(), false)
// Cannot use Interface_OperStatus_DOWN here as the interface is "Not Present"
verifyLaserBiasCurrentAll(t, p1Stream, dut1)
// power on interface transceiver
gnmi.Update(t, dut1, gnmi.OC().Component(transceiverState).Transceiver().Enabled().Config(), true)
gnmi.Await(t, dut1, gnmi.OC().Interface(dp1.Name()).OperStatus().State(), intUpdateTime, oc.Interface_OperStatus_UP)
verifyLaserBiasCurrentAll(t, p1Stream, dut1)
} else {
t.Skip("Skipping sub-test as transceiver Enable/Disable leaf not supported")
}
}
Pull Request Test Coverage Report for Build 19044056506Details
💛 - Coveralls |
- Deviation `explicit_dco_config`, `transceiver_config_enable_unsupported` & `missing_zr_optical_channel_tunable_parameters_telemetry` added.
- TransceiveronOff sub-test is been skipped as Nokia dont support transceiver/config/enable leaf.
"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."
- Deviation `explicit_dco_config`, `transceiver_config_enable_unsupported` & `missing_zr_optical_channel_tunable_parameters_telemetry` added.
- TransceiveronOff sub-test is been skipped as Nokia dont support transceiver/config/enable leaf.
"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."
- Deviation `explicit_dco_config`, `transceiver_config_enable_unsupported` & `missing_zr_optical_channel_tunable_parameters_telemetry` added.
- TransceiveronOff sub-test is been skipped as Nokia dont support transceiver/config/enable leaf.
"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
explicit_dco_config,transceiver_config_enable_unsupported&missing_zr_optical_channel_tunable_parameters_telemetryadded."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."