Skip to content

fix: tests: SRC tests catch aplay warnings and refactor#1351

Merged
KamilxPaszkiet merged 1 commit intothesofproject:mainfrom
ekurdybx:fix-src-tests
Mar 11, 2026
Merged

fix: tests: SRC tests catch aplay warnings and refactor#1351
KamilxPaszkiet merged 1 commit intothesofproject:mainfrom
ekurdybx:fix-src-tests

Conversation

@ekurdybx
Copy link
Contributor

@ekurdybx ekurdybx commented Mar 9, 2026

What's changed:

  • check for aplay warnings (ex. about wrong sample rate)
  • check aplay and arecord exit codes (these tests do not exit on errors automatically, because we want to go through all the tests in the loop, even if previous ones have failed)
  • refactor the check_soundfiles_for_glitches function
  • add logs for cleaner and more readable results
  • move generated audio files used for testing from logs dir to avoid "littering" in the test artifacts
  • add default parameters

@ekurdybx ekurdybx force-pushed the fix-src-tests branch 16 times, most recently from 4fe141b to 1411b51 Compare March 10, 2026 11:52
@ekurdybx ekurdybx marked this pull request as ready for review March 10, 2026 12:18
@ekurdybx ekurdybx requested review from a team, golowanow, lgirdwood and marc-hb as code owners March 10, 2026 12:18
case-lib/lib.sh Outdated
check_for_aplay_warnings()
{
dlogi "$1"
if echo "$1" | grep -q "Warning:"; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

case-lib/lib.sh Outdated

# Check aplay output for warnings
# Arguments: 1-aplay output
check_for_aplay_warnings()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
check_for_aplay_warnings()
check_for_warnings()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

case-lib/lib.sh Outdated
# shellcheck disable=SC2086
arecord $SOF_ALSA_OPTS $SOF_ARECORD_OPTS $1 & PID=$!
# shellcheck disable=SC2181
if [ $? -ne 0 ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

$? will never be useful when you background a command. To get the status of a backgrounded command you need to wait $PID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah you're right, I've missed that. Fixed :)

case-lib/lib.sh Outdated
# shellcheck disable=SC2086
aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2
aplay_output=$(aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 2>&1)
# shellcheck disable=SC2181
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just:

aplay_output=$(aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 2>&1) || errors=$((errors+1))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

case-lib/lib.sh Outdated
fi
# shellcheck disable=SC2086
aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2
aplay_output=$(aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 2>&1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Capturing output loses "real-time" feedback. You can (and should) have both with something like this:

set -o pipefail
aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 2>&1 | tee _.log || errors=$((errors+1))

check_warnings _.log

There are other ways:

https://stackoverflow.com/questions/1221833/pipe-output-and-capture-exit-status-in-bash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I've refactored the whole func a little bit again, now it prints log to the console and also captures it for further analysis

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 10, 2026

Forgot to mention this:

The "real" fix should be in aplay itself, not in sof-test. As in: aplay --fatal-errors --fatal-warnings. @kv2019i is it something that ALSA could implement in the (distant...) future?

@ekurdybx ekurdybx force-pushed the fix-src-tests branch 5 times, most recently from 35029cb to d58bfa4 Compare March 11, 2026 14:04
Catch aplay incorrect sample rate warnings and refactor tests

Signed-off-by: Emilia Kurdybelska <emiliax.kurdybelska@intel.com>
@KamilxPaszkiet KamilxPaszkiet merged commit 6cbcd70 into thesofproject:main Mar 11, 2026
3 checks passed
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.

4 participants