utils/multipath: get_path_status returns None. #6260
Conversation
|
You are changing one of the avocado utils which has already been migrated to AAutils project https://github.com/avocado-framework/aautils and this utility will be removed after the LTS release. Please make sure that all your proposed changes are already in AAutils and this PR is only backport. For more information about AAutlis migration see https://avocado-framework.readthedocs.io/en/latest/blueprints/BP005.html For a list of migrated utilities see https://avocado-framework.github.io/aautils.html |
WalkthroughTwo inner functions in avocado/utils/multipath.py were changed to guard against a None return from get_path_status(path) before indexing. fail_path.is_failed now returns True only when path_stat is not None and path_stat[0] == "failed" and path_stat[2] == "faulty". reinstate_path.is_reinstated now returns True only when path_stat is not None and path_stat[0] == "active" and path_stat[2] == "ready". Both functions include an explicit final return False when checks do not pass. No public signatures were modified. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @cheng191, 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 critical runtime error where attempting to access elements of 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 correctly fixes a TypeError that occurs when get_path_status() returns None. The added checks prevent subscripting a NoneType object. I've suggested some minor refactoring in both modified functions to make the code more concise and Pythonic by combining conditional checks into a single boolean expression.
avocado/utils/multipath.py
Outdated
| path_stat = get_path_status(path) | ||
| if path_stat is None: | ||
| return False | ||
| if path_stat[0] == "failed" and path_stat[2] == "faulty": | ||
| return True | ||
| return False |
There was a problem hiding this comment.
While the fix is correct, this function can be made more concise. The series of if statements and returns can be combined into a single boolean expression.
path_stat = get_path_status(path)
return path_stat is not None and path_stat[0] == "failed" and path_stat[2] == "faulty"
avocado/utils/multipath.py
Outdated
| path_stat = get_path_status(path) | ||
| if path_stat is None: | ||
| return False | ||
| if path_stat[0] == "active" and path_stat[2] == "ready": | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Similar to the is_failed function, this can also be simplified into a single boolean expression for better readability and conciseness.
path_stat = get_path_status(path)
return path_stat is not None and path_stat[0] == "active" and path_stat[2] == "ready"
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6260 +/- ##
=======================================
Coverage 73.58% 73.59%
=======================================
Files 206 206
Lines 22497 22493 -4
=======================================
- Hits 16555 16554 -1
+ Misses 5942 5939 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
richtja
left a comment
richtja
left a comment
There was a problem hiding this comment.
Hi @cheng191, thank you for your contribution. The changes LGTM. Please just update your commit message to be aligned with our Commit style guide and run black . in the avocado root directory to fix the style issues. Thank you.
cheng191
changed the title
get_path_status() returns None (indicating that the path does not exist), directly accessing path_stat[0] and path_stat[2] will raise a TypeError: 'NoneType' object is not subscriptable.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @avocado/utils/multipath.py:
- Around line 276-280: The is_failed() helper is checking for the wrong state;
change its logic to detect a failed path by verifying path_stat is not None and
that path_stat[0] == "failed" and path_stat[2] == "faulty" (instead of
"active"/"ready"). Update the is_failed function to return True when those
failed/faulty values are present so fail_path() will correctly recognize a
successfully failed path. Ensure the function still guards against path_stat
being None before accessing indices.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/multipath.py
🔇 Additional comments (1)
avocado/utils/multipath.py (1)
298-302: LGTM: Correct logic with proper None guard.The None check correctly prevents an
IndexErrorwhenget_path_status()returnsNone, and the logic properly verifies that the path has been reinstated to an active and ready state.
cheng191
force-pushed
the
master
branch
2 times, most recently
from
86831ea to
35342d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @avocado/utils/multipath.py:
- Around line 274-280: The is_failed() helper currently checks the checker
status against "ready" which is inverted; update is_failed() to verify that
get_path_status(path) returns a checker status of "faulty" (i.e. change the
comparison in is_failed() from path_stat[2] == "ready" to path_stat[2] ==
"faulty") so fail_path() correctly detects a failed path; ensure the logic still
guards for path_stat is not None and still compares the primary status
(path_stat[0]) as "failed".
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/multipath.py
🔇 Additional comments (1)
avocado/utils/multipath.py (1)
296-302: LGTM: None check prevents IndexError.The None guard successfully addresses the PR objective by preventing
IndexErrorwhenget_path_status()returnsNone. The status checks ("active"dm_st and"ready"chk_st) correctly verify a reinstated path.
cheng191
changed the title
Signed-off-by: 成渭滨 <chengweibin@uniontech.com>
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.