When checking for Power Admin State, ignore FRU component slots that are empty#5254
When checking for Power Admin State, ignore FRU component slots that are empty#5254mnp-juniper wants to merge 2 commits intoopenconfig:mainfrom
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 addresses a test instability issue in the 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
|
Pull Request Functional Test Report for #5254 / a7dfcc9Virtual Devices
Hardware Devices
|
mnp-juniper
changed the title
![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 modifies telemetry_inventory_test.go to ensure that only non-empty Fabric, Linecard, and Supervisor components are appended to their respective slices. The review feedback suggests improving readability and adhering to Go's idiomatic style by simplifying the boolean expression c.GetEmpty() == false to !c.GetEmpty(), as recommended by the repository's style guide which references Effective Go and Go Code Review Comments.
| if c.GetEmpty() == false { | ||
| fabrics = append(fabrics, c) | ||
| } | ||
| case "Linecard": | ||
| linecards = append(linecards, c) | ||
| if c.GetEmpty() == false { | ||
| linecards = append(linecards, c) | ||
| } | ||
| case "Supervisor": | ||
| supervisors = append(supervisors, c) | ||
| if c.GetEmpty() == false { | ||
| supervisors = append(supervisors, c) | ||
| } |
There was a problem hiding this comment.
For better readability and to adhere to Go's idiomatic style, it's recommended to avoid explicit comparisons to false. The expression if c.GetEmpty() == false can be simplified to if !c.GetEmpty(). This applies to all three checks for Fabric, Linecard, and Supervisor components.
This follows the guidance from the repository style guide, which recommends following Effective Go and Go Code Review Comments.
if !c.GetEmpty() {
fabrics = append(fabrics, c)
}
case "Linecard":
if !c.GetEmpty() {
linecards = append(linecards, c)
}
case "Supervisor":
if !c.GetEmpty() {
supervisors = append(supervisors, c)
}References
- The repository style guide specifies following 'Effective Go' and 'Go Code Review Comments'. 'Effective Go' suggests simplifying boolean expressions by removing explicit comparisons to
trueorfalse. (link)
2 participants
Issue: In DefaultPowerAdminState test, the script gathers all Linecards/Fabrics/Supervisors based on Component Type and checks for the PowerAdminState leaf of the first entry in each of the three lists. If the slot in the first entry is empty, the PowerAdminState leaf may not be available causing the script to fail.
Fix: Add isEmpty() check and include only the non-empty slots only to the lists.