Fix list accumulation in get_all_disk_paths#6284
Fix list accumulation in get_all_disk_paths#6284Poppy-mt wants to merge 1 commit intoavocado-framework:masterfrom
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 resolves a subtle bug in the Highlights
Changelog
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
|
There was a problem hiding this comment.
Code Review
The pull request modifies avocado/utils/disk.py to correct list initialization within the get_all_disk_paths function. It separates the initialization of disk_list and abs_path to prevent them from aliasing the same list object. Furthermore, abs_path is now re-initialized as an empty list inside the loop for each directory path, ensuring it correctly accumulates devices for the current path before extending the main disk_list, thereby fixing a potential bug related to incorrect path aggregation.
|
@richtja Hi, I submitted this PR on March 10th, and it's been a week since then. I haven't received any feedback yet, so I'm reaching out to kindly ask if you might have some time to review it at your convenience. |
|
@harvey0100 Kindly have it checked |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6284 +/- ##
==========================================
- Coverage 73.70% 73.70% -0.01%
==========================================
Files 206 206
Lines 22620 22621 +1
==========================================
- Hits 16673 16672 -1
- Misses 5947 5949 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
harvey0100
left a comment
There was a problem hiding this comment.
Hi @Poppy-mt, the fix is correct -- good catch on the list aliasing bug.
However, the commit is missing Signed-off-by and a commit body. Please amend it to follow the project's commit style (see styleguides).
For example:
avocado/utils/disk: fix list aliasing in get_all_disk_paths
The assignment disk_list = abs_path = [] creates a single list referenced by both names, causing entries to accumulate across loop iterations and duplicate on each extend call. Initialise disk_list separately and reset abs_path inside the loop.
Signed-off-by: Your Name your@email.com
Once that's done and CI is green, it LGTM.
The assignment disk_list = abs_path = [] creates a single list referenced by both names, causing entries to accumulate across loop iterations and duplicate on each extend call. Initialise disk_list separately and reset abs_path inside the loop. Signed-off-by: Poppt-mt 935295021@qq.com
|
Hi @harvey0100, The failing CI test selftests/functional/job_timeout.py::JobTimeOutTest::test_sleep_short_timeout is unrelated to this change. The test expects exit code 8 (AVOCADO_JOB_INTERRUPTED), but in CI the job may exit with 9 (8 | 1) when a timeout interruption is also recorded as ERROR, so both AVOCADO_JOB_INTERRUPTED and AVOCADO_TESTS_FAIL are set. This appears to be an existing test/environment behavior, not a regression from this patch. |
Avoid reusing the same list across paths, which caused repeated
entries to be appended when iterating over multiple directories.