[SKIP SOF-TEST] test-case: Add ALSA conformance tests by golowanow · Pull Request #1281 · thesofproject/sof-test

golowanow · 2025-07-07T21:50:23Z

Add ALSA conformance tests from ChromeOS Audio Test package.

The new test case check-alsa-conformance.sh executes alsa_conformnance_test and compose its results into a JSON file.

golowanow · 2025-07-11T07:36:15Z

SOFCI TEST

lyakh

nice! Just a couple of cosmetic remarks / questions

test-case/check-alsa-conformance.sh

golowanow · 2025-07-11T10:32:57Z

@lyakh, thank you, your comments are addressed, please check.

test-case/check-alsa-conformance.sh

marc-hb · 2025-07-11T16:52:01Z

test-case/check-alsa-conformance.sh

+      skip_test "Neither playback, nor capture ALSA PCM is specified."
+  else
+    PLAYBACK_OPTIONS=$([ -n "${pcm_p}" ] && echo "-P ${pcm_p}" || echo "")
+    CAPTURE_OPTIONS=$([ -n "${pcm_c}" ] && echo "-C ${pcm_c}" || echo "")


I think this is OK but it's not good "role-modelling" because a && b || c is NOT equivalent to if a; then b; else c; fi

See difference explained in #312

EDIT: also, echo "" does nothing so why use it?

marc-hb · 2025-07-11T16:54:28Z

test-case/check-alsa-conformance.sh

+  local run_cmd="${ALSA_CONFORMANCE_TEST} ${PLAYBACK_OPTIONS} ${CAPTURE_OPTIONS} --dev_info_only"
+  dlogc "${run_cmd}"
+  local rc
+  bash -c "${run_cmd}" ; rc=$?


This looks convoluted, why not just:

${run_cmd} || die ...

Is this done to expand OPTIONS? Then run_cmd should really be a bash array.

Long and complex strings are an absolute quoting and debugging nightmare in bash and must always be replaced by arrays.

https://mywiki.wooledge.org/BashGuide/Arrays

@marc-hb, thank you for your comments, I think all are addressed, except this one.

I'd keep it this way (calling from another bash) for these reasons:

in case we later need to call the test command with timeout.

to compose OPTIONS with multiple values (-T, -r, -F)

I don't see why timeout ... "${run_cmd[@]}" would not work

"composing" sounds even more risky = needing arrays even more

Whatever the reasons are, they definitely deserve a comment in the source.

Well, I've managed to change it to arrays. Nice exercise.

test-case/check-alsa-conformance.sh

marc-hb · 2025-07-11T17:03:09Z

test-case/check-alsa-conformance.sh

+pcm_p=${OPT_VAL['p']}
+pcm_c=${OPT_VAL['c']}
+rates=$([ -z "${OPT_VAL['r']}" ] && echo '' || echo "--allow-rates ${OPT_VAL['r']}")
+formats=$([ -z "${OPT_VAL['F']}" ] && echo '' || echo "--allow-formats ${OPT_VAL['F']}")


The echo '' is not needed.

rates=$([ -z "${OPT_VAL['r']}" ] || echo "--allow-rates ${OPT_VAL['r']}")

Also, this repetition is calling for some new prefix_if_not_empty() and prefix_if_1() functions. I know they would be useful outside this new script.

formats=$(prefix_if_not_empty --allow-formats "${OPT_VAL['F']}")

test-case/check-alsa-conformance.sh

golowanow · 2025-07-13T10:25:50Z

13/Jul/2025 PCMs selection for the test run is extended - now it is possible to

choose several PCMs;
run for all devices on a card;
run for all PCMs on the DUT.

marc-hb · 2025-07-14T08:45:03Z

test-case/check-alsa-conformance.sh

+  do
+    mapfile -t p_dev_expanded < <(get_card_devices 'playback' "${p_dev}")
+    # shellcheck disable=SC2206
+    PLAYBACK_DEVICES+=( ${p_dev_expanded[@]} )


It would be nice to have an example of why this is not:

Suggested change

PLAYBACK_DEVICES+=( ${p_dev_expanded[@]} )

PLAYBACK_DEVICES+=( "${p_dev_expanded[@]}" )

(that's a LOT of disable=SC2206...)

right, here it is not needed

test-case/check-alsa-conformance.sh

marc-hb

Sorry I keep finding minor problems but that's because the code keep changing :-)

marc-hb · 2025-07-14T18:27:44Z

test-case/check-alsa-conformance.sh

+  elif [ "${mode}" == 'capture' ]; then
+    alsa_list=('arecord' '-l')
+  else
+    return


Not a failure? A dlogi maybe?

it will be logged at the caller side as empty devices whereas any dlogi will mess the expected list of devices

sorry I read too fast and missed the output was on stdout. A comment at the top of the function would not hurt ;-)

You could still:

log to stderr

return 1

both

None of these would disrupt the caller - and it would give it more information. That it can choose to use or ignore.

test-case/check-alsa-conformance.sh

marc-hb · 2025-07-14T18:38:57Z

test-case/check-alsa-conformance.sh

+  run_cmd+=("--json-file" "${AUDIOTEST_OUT}_${mode}.json")
+  dlogc "${run_cmd[@]}"
+  local rc=0
+  "${run_prefix[@]}" && "${run_cmd[@]}" || rc=$?


I think this is correct but it's really tricky; not for the faint of heart.

Too many people think A && B || CC is the same as if A; then B; else C; fi and that they can save typing 15 characters that way. But it's not the same! (#312)

# Generally NOT what you want! true && false || echo 'else_is_also_run!!'

HOWEVER, I think this example is correct but it is in any case way too subtle and bad role-modelling IMHO.

There are (at least) two better ways:

Suggested change

"${run_prefix[@]}" && "${run_cmd[@]}" || rc=$?

"${run_prefix[@]}" # export can hardly fail

"${run_cmd[@]}" || rc=$?

Maybe simpler and better IMHO: you don't need run_prefix at all if the current process does not need to be affected:

Suggested change

"${run_prefix[@]}" && "${run_cmd[@]}" || rc=$?

"PATH=${ALSA_CONFORMANCE_PATH}:${PATH}" "${run_cmd[@]}" || rc=?

I've tried it with different combinations. And the prefix part is to make clear/shorter separation of what is the preparational part (eg. other env. variables).

If the name matters, then you can likely just do something like this:

run_prefix="PATH=${ALSA_CONFORMANCE_PATH}:${PATH}" "${run_prefix}" "${run_cmd[@]}" || rc=$?

or

run_prefix=(PATH="${ALSA_CONFORMANCE_PATH}:${PATH}" FOO=bar) "${run_prefix[@]}" "${run_cmd[@]}" || rc=$?

golowanow · 2025-07-14T20:58:36Z

Sorry I keep finding minor problems but that's because the code keep changing :-)

yeah, I need to use this test already, so it evolves while in review ..

marc-hb · 2025-07-14T21:25:41Z

test-case/check-alsa-conformance.sh

+      local gawk_script='match($0, /^card [0-9]+: ('"${arg_pcm}"') .+ device ([0-9]+): /, arr) { print "hw:" arr[1] "," arr[2] }'
+      mapfile -t res_devs < <( "${alsa_list[@]}" | gawk "${gawk_script}" )
+    fi
+    echo "${res_devs[@]}"


For anything non trivial I would always prefer printf

https://unix.stackexchange.com/questions/65803/why-is-printf-better-than-echo

marc-hb · 2025-07-14T21:28:23Z

yeah, I need to use this test already, so it evolves while in review ..

Not ideal but typical for brand new tests and other big changes.

Actually, I had my own requests in mind and not this but it's probably both :-)

Sorry I keep finding minor problems but that's because the code keep changing :-)

To be clear: this is IMHO good enough to merge. As a brand new test, it cannot regress anything :-) On other hand, "fix later" == "fix never" more often than not....

Add ALSA conformance tests from ChromeOS Audio Test package. The new test case `check-alsa-conformance.sh` executes `alsa_conformnance_test` and compose its results into a JSON file. Signed-off-by: Dmitrii Golovanov <dmitrii.golovanov@intel.com>

golowanow · 2025-07-15T20:30:25Z

15/Jul/2025 with some comments addressed, and 3 new optional parameters (--allow-channels, --skip-channels, --timeout) which require the extended version of alsa_conformance_test.py not yet up-streamed, but deployed on our CI.
Hopefully, no more additions to this PR.

golowanow · 2025-07-16T06:56:56Z

merged as no objections and to enable this new test case on CI

golowanow added DNM Do not merge type:enhancement New framework feature or request labels Jul 7, 2025

golowanow force-pushed the alsa_conformance_20250707 branch 2 times, most recently from 2768492 to 3585cb9 Compare July 8, 2025 17:32

golowanow marked this pull request as ready for review July 8, 2025 19:00

golowanow requested review from a team, lgirdwood and marc-hb as code owners July 8, 2025 19:00

golowanow requested a review from ujfalusi July 8, 2025 19:01

golowanow removed the DNM Do not merge label Jul 8, 2025

ujfalusi approved these changes Jul 10, 2025

View reviewed changes

golowanow changed the title ~~[SKIP SOF-TEST] test-case: Add ALSA conformance tests~~ test-case: Add ALSA conformance tests Jul 11, 2025

lyakh reviewed Jul 11, 2025

View reviewed changes

test-case/check-alsa-conformance.sh Outdated Show resolved Hide resolved

test-case/check-alsa-conformance.sh Outdated Show resolved Hide resolved

test-case/check-alsa-conformance.sh Outdated Show resolved Hide resolved

test-case/check-alsa-conformance.sh Outdated Show resolved Hide resolved

golowanow force-pushed the alsa_conformance_20250707 branch from 3585cb9 to cfa3b3d Compare July 11, 2025 10:31

golowanow requested a review from lyakh July 11, 2025 10:33

golowanow force-pushed the alsa_conformance_20250707 branch from cfa3b3d to e189697 Compare July 11, 2025 11:50

lyakh reviewed Jul 11, 2025

View reviewed changes

test-case/check-alsa-conformance.sh Outdated Show resolved Hide resolved

lyakh approved these changes Jul 11, 2025

View reviewed changes

marc-hb reviewed Jul 11, 2025

View reviewed changes

golowanow force-pushed the alsa_conformance_20250707 branch from e189697 to 91bdb4c Compare July 11, 2025 18:12

marc-hb reviewed Jul 11, 2025

View reviewed changes

test-case/check-alsa-conformance.sh Outdated Show resolved Hide resolved

marc-hb reviewed Jul 11, 2025

View reviewed changes

test-case/check-alsa-conformance.sh Outdated Show resolved Hide resolved

golowanow force-pushed the alsa_conformance_20250707 branch 2 times, most recently from cf89c0c to 9fecb79 Compare July 12, 2025 13:55

golowanow requested a review from marc-hb July 12, 2025 14:00

golowanow force-pushed the alsa_conformance_20250707 branch from 9fecb79 to 34b33eb Compare July 13, 2025 09:53

marc-hb reviewed Jul 14, 2025

View reviewed changes

golowanow force-pushed the alsa_conformance_20250707 branch from 34b33eb to 11602d5 Compare July 14, 2025 15:46

golowanow requested a review from marc-hb July 14, 2025 15:50

marc-hb reviewed Jul 14, 2025

View reviewed changes

golowanow force-pushed the alsa_conformance_20250707 branch from 11602d5 to c08c26c Compare July 15, 2025 16:46

golowanow changed the title ~~test-case: Add ALSA conformance tests~~ [SKIP SOF-TEST] test-case: Add ALSA conformance tests Jul 15, 2025

golowanow force-pushed the alsa_conformance_20250707 branch from c08c26c to 6543ac6 Compare July 15, 2025 18:47

test-case: Add ALSA conformance tests

3565cbc

Add ALSA conformance tests from ChromeOS Audio Test package. The new test case `check-alsa-conformance.sh` executes `alsa_conformnance_test` and compose its results into a JSON file. Signed-off-by: Dmitrii Golovanov <dmitrii.golovanov@intel.com>

golowanow force-pushed the alsa_conformance_20250707 branch from 6543ac6 to 3565cbc Compare July 15, 2025 20:20

golowanow merged commit 34b6e0a into thesofproject:main Jul 16, 2025
3 checks passed

	PLAYBACK_DEVICES+=( ${p_dev_expanded[@]} )
	PLAYBACK_DEVICES+=( "${p_dev_expanded[@]}" )

	"${run_prefix[@]}" && "${run_cmd[@]}" \|\| rc=$?
	"${run_prefix[@]}" # export can hardly fail
	"${run_cmd[@]}" \|\| rc=$?

	"${run_prefix[@]}" && "${run_cmd[@]}" \|\| rc=$?
	"PATH=${ALSA_CONFORMANCE_PATH}:${PATH}" "${run_cmd[@]}" \|\| rc=?

Conversation

golowanow commented Jul 7, 2025

Uh oh!

golowanow commented Jul 11, 2025

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

golowanow commented Jul 11, 2025

Uh oh!

Uh oh!

marc-hb Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

golowanow Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

golowanow commented Jul 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

golowanow commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Jul 14, 2025

Uh oh!

golowanow commented Jul 15, 2025

Uh oh!

Uh oh!

golowanow commented Jul 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marc-hb Jul 11, 2025 •

edited

Loading

golowanow Jul 11, 2025 •

edited

Loading

marc-hb Jul 14, 2025 •

edited

Loading