Skip to content

Conversation

@heywji
Copy link
Contributor

@heywji heywji commented Oct 29, 2025

Remove process.TimeoutError from exception handling, as it doesn't exist in avocado.utils.process module.

ID: 4640

Signed-off-by: Wenkang Ji [email protected]

Summary by CodeRabbit

Release Notes

  • Chores
    • Updated internal test exception handling logic for improved error propagation in specific scenarios.

Remove process.TimeoutError from exception handling, as it doesn't
exist in avocado.utils.process module.

Signed-off-by: Wenkang Ji <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Oct 29, 2025

Walkthrough

The exception handling in the host-debuginfo installation step has been narrowed to catch only CmdError instead of both CmdError and TimeoutError. Timeout scenarios will now propagate rather than be treated as test failures.

Changes

Cohort / File(s) Change Summary
Exception Handling Narrowing
qemu/tests/dump_guest_core.py
Removed TimeoutError from the except clause in host-debuginfo installation step; now catches only CmdError, allowing timeouts to propagate

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Exception handling logic: Verify the rationale for removing TimeoutError handling—this changes error propagation behavior and could affect test reliability across multiple runs
  • Impact analysis: Confirm whether timeout scenarios should indeed propagate uncaught or if this represents a behavioral regression
  • Test implications: Consider whether existing tests expect timeouts to be handled gracefully or if exposing them is intentional

Poem

🐰 A narrower net now catches the flow,
TimeoutError left to steal the show,
Exception paths shift left and right,
Testing's dance in dimmer light.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "dump_guest_core: Fix AttributeError in dump_guest_core test" directly addresses the main objective of the changeset. According to the PR description and objectives, the primary change is removing process.TimeoutError from exception handling because this attribute does not exist in the avocado.utils.process module, which prevents an AttributeError from being raised. The title accurately identifies that the PR fixes an AttributeError in the dump_guest_core test, which is the core issue being addressed. The title is concise, specific, and clearly conveys the purpose of the change to someone reviewing the commit history.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1eb1d9a and a0ffe53.

📒 Files selected for processing (1)
  • qemu/tests/dump_guest_core.py (1 hunks)
🔇 Additional comments (1)
qemu/tests/dump_guest_core.py (1)

83-86: Code change is correct—no action needed.

Verification confirms that avocado.utils.process.run() raises avocado.utils.process.CmdError on timeout, not a separate TimeoutError. The fix correctly removes the non-existent process.TimeoutError reference while the existing CmdError handler at line 85 properly catches and converts timeout exceptions to descriptive test failures.


Comment @coderabbitai help to get the list of available commands and usage tips.

@heywji
Copy link
Contributor Author

heywji commented Oct 29, 2025

Test Result: PASS

FROM:

 (1/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.unattended_install.cdrom.ex
tra_cdrom_ks.default_install.aio_threads.q35: STARTED                                                                                  
 (1/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.unattended_install.cdrom.ex
tra_cdrom_ks.default_install.aio_threads.q35:  PASS (576.05 s)                                                                         
 (2/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.dump_guest_core.on.q35: STA
RTED                                                                                                                                   
 (2/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.dump_guest_core.on.q35:  ER
ROR: module 'avocado.utils.process' has no attribute 'TimeoutError' (323.43 s)                                                         
RESULTS    : PASS 1 | ERROR 1 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0                                                      

TO:

 (1/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu
.unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads.q35: STARTED
 (1/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu
.unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads.q35:  PASS (564.69 s)
 (2/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu
.dump_guest_core.on.q35: STARTED
 (2/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-q
emu.dump_guest_core.on.q35:  PASS (425.28 s)
RESULTS    : PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

@heywji
Copy link
Contributor Author

heywji commented Oct 30, 2025

@leidwang @PaulYuuu Hi Leidong and Yihuang, I am not sure that I am AT-ing the right person for this qemu feature review purpose. Could you please review and merge this patch?

Copy link
Contributor

@PaulYuuu PaulYuuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a separate commit in #4391, they are all related to same feature, additional PR will only be burdensome.

@heywji
Copy link
Contributor Author

heywji commented Nov 4, 2025

Closed this PR because I have moved the commit to the new location, as Yihuang suggested.
b846806

@heywji heywji closed this Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants