New aplay_opts() and arecord_opts() to change flags across all tests

[marc-hb]

"All problems in computer science can be solved by another level of
indirection"

First step towards #489

Default SOF_ALSA_OPTS to --fatal-errors, can be overriden with:

 SOF_ALSA_OPTS="" ./scripts/run-all-tests.sh

Signed-off-by: Marc Herbert marc.herbert@intel.com

[marc-hb]

https://sof-ci.01.org/softestpr/PR498/build369/devicetest was 100% successful even with the addition of --fatal-errors in 3 basic tests.

One new quoting issue caught by shellcheck in https://travis-ci.org/github/thesofproject/sof-test/builds/741801847, my bad Ijust force-pushed a fix.

[xiulipan]

Nice patch for a generic wrapper for aplay/arecord call. But I think we may need more README and a correct file for the overridable ENV

[xiulipan]

@marc-hb maybe consider to put this into the case-lib/config.sh file? That config file is designed for overridable ENV values.

[marc-hb]

I like config files but I see a number of issues with the case-lib/config.sh design. I'll remove the default value here and resubmit so we can postpone the potentially complex discussion about config files and focus on these wrappers first. One change at a time!

Some issues with case-lib/config.sh:

• Except for two actual values, all the rest is made of no-op AAAA=${AAAA:-} statements. That's very misleading from a coding perspective.
• What would be more useful is a local config file that is not in git so it doesn't pollute git diff and git status (and even git add * - never do that)
• I'm not even sure a single file collecting all default values is a good idea. Most software seems to have good defaults even when run with empty config files.

[xiulipan]

I create a new issue #502 for the later discussion. Nice point here

[xiulipan]

I think opts means options? maybe a more good name for the wrapper function

[marc-hb]

I've been looking for a better but still short name but I didn't find any, suggestions anyone? It has to be kept relatively short because it will be used a lot.

[xiulipan]

Same feeling here. If I had a better name, I should type it already.

[xiulipan]

same for SOF_ARECORD_OPTS like SOF_APLAY_OPTS

[plbossart]

we also want to test the return status since it contains an error code when it fails. If you just stop I think the rest of the script will probably think nothing bad happened.

[marc-hb]

The vast majority of the time set -e (errexit #312) will immediately abort the script, demonstration:

$ SOF_ALSA_OPTS=--bogus ./test-case/check-playback.sh

2020-11-06 19:05:09 UTC [COMMAND] sof-tplgreader.py /lib/firmware/intel/sof-tplg/sof-apl-pcm512x.tplg -f 'type:playback & id:any' -b ' pcm:HDA Digital' -s 0 -e
2020-11-06 19:05:09 UTC [INFO] ===== Testing: (Round: 1/1) (PCM: Port5 [hw:0,0]<playback>) (Loop: 1/3) =====
2020-11-06 19:05:09 UTC [COMMAND] aplay --bogus  -Dhw:0,0 -r 48000 -c 2 -f S16_LE -d 10 /dev/zero -v -q
aplay: unrecognized option '--bogus'
Try `aplay --help' for more information.
2020-11-06 19:05:09 UTC [ERROR] aplay on PCM hw:0,0 failed at 1/3.
2020-11-06 19:05:09 UTC [ERROR] Starting func_exit_handler(), exit status=1, FUNCNAME stack:
2020-11-06 19:05:09 UTC [ERROR]  die()  @  ./test-case/../case-lib/lib.sh
2020-11-06 19:05:09 UTC [ERROR]  main()  @  ./test-case/check-playback.sh:99

Same with ./test-case/check-capture.sh

The other test SOF_ALSA_OPTS=--bogus ./test-case/multiple-pipeline-capture.sh doesn't stop immediately because multiple-pipeline-capture.sh backgrounds the aplay and arecord commands. Testing the error code inside this function would add complexity and not make any difference because the entire function is backgrounded.

What makes a difference with backgrounded commands is to collect ASAP their exit status with wait $aplayPID. This is something sof-test rarely does for now (cause we like our green failures...) but that I systematically requested in every code review involving backgrounded command(s). wait $aplay_optsPID will collect the exit status of the last command in aplay_opts() function which is of course aplay. set -e applies to wait exactly the same and if desired it can be overriden with a better message (#312) the same.

Even without considering backgrounding issues, the caller of aplay_opts caller can generally provide more useful and relevant context if a custom error message is desired.

[marc-hb]

But I think we may need more README

Good point I'll add something to the README.md

[marc-hb]

I've been looking for a better but still short name but I didn't find any, suggestions anyone? It has to be kept relatively short because it will be used a lot.

Done, please review

[plbossart]

I have zero understanding of the bash/shellcheck considerations below, if that detects issues and does not add false-positives that sounds good to me.

[marc-hb]

@xiulipan , others I think I addressed or answered all your comments, can you please check?

This change is a lot of English to explain the direction and very little code change for the moment :-)

[xiulipan]

Thanks for the update, let's update the usage once we find it is needed for other test cases.