Adding RT-1.36: AIGP feature support test#5375
Adding RT-1.36: AIGP feature support test#5375ASHNA-AGGARWAL-KEYSIGHT wants to merge 1 commit intoopenconfig:mainfrom
Conversation
Pull Request Functional Test Report for #5375 / 3c7ca13Virtual 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 introduces comprehensive test coverage for the BGP AIGP (Accumulated IGP Metric) feature, ensuring correct attribute propagation and policy application across different network instances. Additionally, it refactors and extends internal test infrastructure to support new configuration requirements and device-specific deviations, while also refining existing ISIS graceful restart tests for better reliability. 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
|
![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 a new test suite for the BGP Accumulated IGP Metric (AIGP) feature and updates ISIS Graceful Restart tests with improved telemetry checks. It also extends internal configuration plugins and deviations to support Arista-specific CLI fallbacks for BGP policies, interface encapsulation, and sub-interface formatting. The review feedback identifies a misleading error message in the RIB validation logic, a bug in a test case log format that repeats the test name instead of the description, and the presence of leftover debug logs that should be removed.
| gotAIGP := attrSet.GetAigp() | ||
| if gotAIGP != wantAIGP { | ||
| t.Errorf("fail: aigp for neighbor %s in NI %s: want %d, got %d", nbrAddr, ni, wantAIGP, gotAIGP) | ||
| t.Errorf("OC not supported for AIGP attribute in BGP RIB") |
There was a problem hiding this comment.
This error message is misleading. Since gotAIGP was successfully retrieved using OpenConfig on line 1023, the claim that OpenConfig is not supported is contradictory. If the metric comparison fails, the preceding t.Errorf on line 1026 already provides the necessary failure details. Additionally, consider using t.Fatalf instead of t.Errorf if a failure here makes subsequent test steps meaningless.
References
- In tests, t.Fatalf is preferred over t.Errorf when a failure makes subsequent test steps meaningless, as this fails fast and reduces overall test execution time.
| // Run the test cases. | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Logf("Description: %s - %s", tc.name, tc.name) |
There was a problem hiding this comment.
The log message incorrectly repeats tc.name. It should use tc.description for the second format specifier to provide the intended test case description.
| t.Logf("Description: %s - %s", tc.name, tc.name) | |
| t.Logf("Description: %s - %s", tc.name, tc.description) |
| t.Log(dut1loopback10.loopbackIntfName) | ||
| t.Log(dut2loopback10.loopbackIntfName) |
There was a problem hiding this comment.
These t.Log statements appear to be leftover debug logs and should be removed from the production test code.
ASHNA-AGGARWAL-KEYSIGHT
force-pushed
the
fpt_otg_RT_1_36
branch
from
bdf443e to
3c7ca13
Compare
2 participants
Readme: https://github.com/toadeniy-g/featureprofiles/blob/main/feature/bgp/otg_tests/aigp_test/README.md
Raised an issue for the test not passing completely: https://partnerissuetracker.corp.google.com/issues/505782544