CI: add automated sound playback integration test#117
Conversation
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".ci/test-sound.sh">
<violation number="1" location=".ci/test-sound.sh:27">
P1: Unreachable error handling code. The `ASSERT` function from `common.sh` exits the script directly on failure, so `ret="$?"` and the subsequent `MESSAGES` array logic will never execute for error cases. The descriptive error messages ('Fail to boot', 'Fail to login', etc.) will never be displayed.
Consider capturing the exit code without using ASSERT, or modify `test_sound` to not use ASSERT for this case.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
jserv
left a comment
There was a problem hiding this comment.
Improve descriptions about this pull request.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".ci/test-sound.sh">
<violation number="1" location=".ci/test-sound.sh:43">
P1: `ret="$?"` now captures the exit code of `echo` (always 0) instead of `test_sound`. This breaks the error message handling logic below. The success message should be printed after capturing the return code.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".ci/test-sound.sh">
<violation number="1" location=".ci/test-sound.sh:27">
P2: Using `ASSERT` here makes the error handling code below unreachable. When `expect` returns a non-zero exit code (1-4), `ASSERT` will immediately exit the script with a generic "Assert failed: expect" message, bypassing the specific error messages in the MESSAGES array. Either remove `ASSERT` to preserve the detailed error messages, or remove the now-dead MESSAGES handling code.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".ci/test-sound.sh">
<violation number="1" location=".ci/test-sound.sh:27">
P1: Removing `ASSERT` breaks error handling. Since the script uses `set -euo pipefail`, when `expect` fails, the script will exit immediately before `ret="$?"` can capture the exit status. The error message logic (`MESSAGES` array) becomes dead code and users won't see helpful failure diagnostics. Other CI scripts (`test-netdev.sh`, `autorun.sh`) correctly use `ASSERT expect`.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
69e4e1d to
11ca7c3
Compare
jserv
left a comment
There was a problem hiding this comment.
Improve git commit messages.
11ca7c3 to
7bd7b24
Compare
jserv
left a comment
There was a problem hiding this comment.
Improve git commit messages!
- Create '.ci/test-sound.sh' to validate playback on host OS via expect. - Integrate sound test into Ubuntu and macOS GitHub workflows. - Set higher timeout for macOS (900s) to account for slower CI runners.
7bd7b24 to
1ba71a0
Compare
|
Thank @Cuda-Chen for contributing! |
.ci/test-sound.shto validate playback on host OS via expect.Summary by cubic
Add virtio-snd CI to automatically validate sound playback in semu on Linux and macOS, catching audio regressions early.
Written for commit 1ba71a0. Summary will update automatically on new commits.