add User space test cases #363

libinyang · 2020-09-07T06:31:31Z

This patches add the test cases for user space.
It includes:

verify-ucm-config (already merged)
check-user-packages: test installing user space packages, restart pulseaudio
check-userspace-pa-status: check the pulseaudio can be run successfully
check-userspace-cardinfo: check pulseaudio can parse the ucm correctly and exports the correct card
check-userspace-paplay: go through all the active sinks and play on it if port is available (almost finished, not included in this patchset)
check-userspace-parecord: go through all the active sources and record on it if port is available (almost finished, not included in this patchset)

marc-hb

To save a lot of review time, please run shellcheck -x locally before submitting and start all scripts with set -e (see #312)

test-case/check-userspace-pa-status.sh

test-case/check-userspace-packages.sh

xiulipan

Please remove the install code part and update other checks.

test-case/check-userspace-cardinfo.sh

test-case/check-userspace-pa-status.sh

test-case/check-userspace-packages.sh

libinyang · 2020-09-09T02:14:59Z

I do not think this function should not be a test case. This should be some ENV check for our CI instead of a shared test case. Please use dpkg or apt for check only. Install should be in other part.

This is an installation script, it does not belong to the test-case directory. Can you submit this in a different Pull Request?

@xiulipan @marc-hb Thanks for raise this discussion. When I developed the check-userspace-packages test case at first, I did put it in sof-sh repo. From logic view, this should be put in sof-sh repo. However, I met some issues when I was trying to install the packages in sof-sh.

install pulseaudio, libasounds serial packages need a lot of dependency. This means we need more check whether they are installed successfully or not. From this point view, it's like a test case to check the dependency issue than just installation.
My final solution of this test case is a little complex logic, such as: Install libasound2 in Monday and test, install pulseaudio in Tuesday and test, install UCM in Wednesday and test. With this we can easily tell the failure is because of libasound2 update, pulseaudio update or UCM updated. It seems this logic is too complex for sof-sh.
I also want to test 'pulseaudio --kill' 'pulseaudio --start (or as Marc suggested use systemctl)' after install the new pulseaudio package. Of course, this should be not a big problem, we can split it a very simple test case which dedicates to test 'start' and 'stop'.

xiulipan · 2020-09-09T03:11:24Z

@libinyang Please split set up test device and test scripts.

Setup scripts should be CI stuff, please add those into our CI scripts.
For the test case dependency, we have env-check.sh to check and give advice. Also if the case is optional, we should add check before the test run.
Please do not install anything silently. We can add anything into our CI deploy scripts.
This should be OK. If the pulseaudio can be controlled now. Otherwise we will have conflict with other test cases.

Please try to set up a sync up meeting for this at any time you prefer

marc-hb · 2020-09-09T03:40:57Z

Packages dependencies issues should be tested when new packages are built. Based on discussions in Add document to clarify BKM of user space packages: alsa-lib, alsa-ucm-conf, PulseAudio, GNOME etc linux#2211 It doesn't look like you want to move the debian code to build user space packages to sof-test, correct?

My final solution of this test case is a little complex logic, such as: Install libasound2 in Monday and test, install pulseaudio in Tuesday and test, install UCM in Wednesday and test.

Again based on Add document to clarify BKM of user space packages: alsa-lib, alsa-ucm-conf, PulseAudio, GNOME etc linux#2211, we don't want to upgrade CI to newer user space package versions every week, do we? If correct then each CI upgrade will be manual, ideally performed in a PR. These PRs can be staggered as needed.
We should test pulseaudio continuously, not just when upgrading it and independently from it. pulseaudio could break because of a kernel or firmware change.

libinyang · 2020-09-09T05:30:15Z

It seems 2 vs 1 to prefer to put it into the sof-sh. OK. I will move it into sof-sh.

This should be OK. If the pulseaudio can be controlled now. Otherwise we will have conflict with other test cases.

No, pulseaudio will conflict with other driver test cases. So I setup 2 CI DUT dedicated for user space test.

Packages dependencies issues should be tested when new packages are built. Based on discussions in thesofproject/linux#2211 It doesn't look like you want to move the debian code to build user space packages to sof-test, correct?

Build machine is different from the DUT. So we won't test the dependencies when building the packages on DUT. I use the debian code to build the alsa lib based on your suggestion.

Again based on thesofproject/linux#2211, we don't want to upgrade CI to newer user space package versions every week, do we? If correct then each CI upgrade will be manual, ideally performed in a PR. These PRs can be staggered as needed.

Based on Mengdong's requirement, this is used to test the upstreamed code. We will use this to make sure the latest alsa lib, pulseaudio and UCM won't break our Intel platform.
In thesofproject/linux#2211, we provide the stable packages, which are for products release. Only with many tests (including automation test and manual test), these packages can be the stable packages.

We should test pulseaudio continuously, not just when upgrading it and independently from it. pulseaudio could break because of a kernel or firmware change.

Yes, this is why we add the user space test case. Actually, the kernel change may cause UCM error, which will cause user space totally not work. I met such issue before. The user space test cases are not finished. I will submit another 2 test cases today of paplay and parecord.

marc-hb · 2020-09-09T06:37:21Z

OK. I will move it into sof-sh. [...] I use the debian code to build the alsa lib based on your suggestion.

Is this closed source location where the code to build the packages is right now? In thesofproject/linux#2211 I shared a (pretty small), open-source prototype that anyone can use to rebuild alsa-lib packages.

No, pulseaudio will conflict with other driver test cases

I spent some more time trying the systemctl commands suggested in other pulseaudio isssues and it looks like it shouldn't be too hard to control pulseaudio like this. I believe no one surprisingly tried to use systemctl yet so maybe it's just the matter of using the right tools.

Build machine is different from the DUT. So we won't test the dependencies when building the packages on DUT.

Both systems should be Ubuntu 20.04 configured similarly so I don't understand which dependencies could be different, can you share an example? SOF developers shouldn't have to use more than one system. They can but they shouldn't have too. thesofproject/linux#2402 (comment)

Based on Mengdong's requirement,... We will use this to make sure the latest alsa lib, pulseaudio and UCM won't break our Intel platform. In thesofproject/linux#2211, we provide the stable packages, which are for products release

Please write down and submit for review these test cycle and release requirements before submitting the corresponding code. I understand we don't want to wait for official ALSA releases but don't think we want to test every single ALSA commit either, do we?

libinyang · 2020-09-09T07:52:57Z

Is this closed source location where the code to build the packages is right now? In thesofproject/linux#2211 I shared a (pretty small), open-source prototype that anyone can use to rebuild alsa-lib packages.

It is closed source location, in sof-sh repo. Not sure to attach the code address here is OK?

No, pulseaudio will conflict with other driver test cases

I spent some more time trying the systemctl commands suggested in other pulseaudio isssues and it looks like it shouldn't be too hard to control pulseaudio like this. I believe no one surprisingly tried to use systemctl yet so maybe it's just the matter of using the right tools.

I didn't mean the conflict of using systemclt. I was answering Xiuli, that pulseaudio will conflict with aplay test cases.

Build machine is different from the DUT. So we won't test the dependencies when building the packages on DUT.

Both systems should be Ubuntu 20.04 configured similarly so I don't understand which dependencies could be different, can you share an example? SOF developers shouldn't have to use more than one system. They can but they shouldn't have too. thesofproject/linux#2402 (comment)

I don't have example here. But no example doesn't mean there will never be dependency issues, right? With the pulseaudio updated, it may require the related application's upgrade. If you try to install the latest pulseaudio on ubuntu 18.04, I believe you will meet such issue. However, we don't need to take too much effort on it now, this should not be our priority task. And if we really meet such issue, I think we can manually update the system.

Based on Mengdong's requirement,... We will use this to make sure the latest alsa lib, pulseaudio and UCM won't break our Intel platform. In thesofproject/linux#2211, we provide the stable packages, which are for products release

Please write down and submit for review these test cycle and release requirements before submitting the corresponding code. I understand we don't want to wait for official ALSA releases but don't think we want to test every single ALSA commit either, do we?

My plan is to test the user space package once every week. Do you think this test frequency is OK? Xiuli talked to me before that we can change to test every single commit.

libinyang · 2020-09-09T08:58:35Z

update:

refine the patches based on Xiuli and Marc suggestion
remove the check of packages installation as Xiuli and Marc are strongly against it.
add the test case paplay and parecord

marc-hb · 2020-09-09T22:52:42Z

I was answering Xiuli, that pulseaudio will conflict with aplay test cases.

Hopefully not when disabled with systemctl --user. Then we can have a unique DUT configuration for everything - which is what developers need anyway. See previous discussions in https://github.com/thesofproject/sof-test/issues?q=+label%3Apulseaudio+

I don't have example here. But no example doesn't mean there will never be dependency issues, right? With the pulseaudio updated, it may require the related application's upgrade

So a separate build system is solving a problem we don't know we have and that we haven't tried to solve yet...

My plan is to test the user space package once every week. Do you think this test frequency is OK?

That seems reasonable to me but it should be discussed with more people. To keep that particular discussion focused it shouldn't be diluted in this PR or in 2211. Instead it should be submitted as focused, documentation PR. You could also start a new topic at https://github.com/orgs/thesofproject/teams/sof-developers

libinyang · 2020-09-15T08:33:59Z

a separate patch to deploy the user space packages patch has been submitted in sof-sh repo as Marc and Xiuli's feedback.

libinyang · 2020-09-15T08:35:43Z

Hopefully not when disabled with systemctl --user. Then we can have a unique DUT configuration for everything - which is what developers need anyway. See previous discussions in https://github.com/thesofproject/sof-test/issues?q=+label%3Apulseaudio+

I checked the sof-sh. Currently we will remove pulseaudio. So it will not impact on our other DUTs in CI.

test-case/check-userspace-cardinfo.sh

test-case/check-userspace-parecord.sh

tools/pactlinfo.py

plbossart

let's be careful about licenses please?

tools/pactlinfo.py

libinyang · 2020-10-14T05:10:45Z

patch updated:

Move pactl stat check into lib.sh as a helper function
Not use jq. Add parameters in pactlinfo.py and make pactlinfo.py return specified information

xiulipan

Could we merge the playback and capture test later? I think we only need some parameter to split playback and catpure.

libinyang · 2020-10-15T07:48:37Z

Could we merge the playback and capture test later? I think we only need some parameter to split playback and catpure.

Sure. We can talk about it in detail.

marc-hb · 2020-10-15T07:56:40Z

case-lib/lib.sh

+        return 1
+    fi
+
+    return 0


If something goes wrong&>/dev/null will hide it. This is test code, never, ever redirect stderr and hide errors. Redirecting stdout to a log file is useful when it's big. This is not big.

The fewer negations and the more readable. Negations with ! are especially annoying because they cannot be copied to the interactive shell.

pactl stat || { dloge ... return 1 }

return 0 is useless most of the time. It is useless here, remove it.

In the test cases, why do we care the interactive shell?

In my new patch, I will use :
pactl stat &>dev/null || {
dloge ...
return 1
}

I still use "&>dev/null" because error message pactl stat doesn't show explicit information that pactl stat fails. It doesn't help for developer to identity which fails quickly. Instead, I will use dloge to export the useful information for sof-test developer.

marc-hb · 2020-10-15T07:57:08Z

test-case/check-userspace-cardinfo.sh

+
+# check pulseaudio runs properly or not
+if ! func_lib_check_pa; then
+    die "Please check whether pulseaudio runs correctly or not"


See above, no need for a negation.

marc-hb · 2020-10-15T07:58:35Z

test-case/check-userspace-cardinfo.sh

+## Expect result:
+##    There is at least one available card for test
+##
+


Use set -e in every new test (old tests are being converted too)

grep will fail if set -e.

try grep ABC || true

See detailed set -e guidelines in #312

marc-hb · 2020-10-15T08:04:39Z

test-case/check-userspace-cardinfo.sh

+OLDIFS=$IFS
+dlogc "pactl list cards short"
+cardlist=$(pactl list cards short)
+((available_card=0))


This bashism not compatible with set -e. Change to the POSIX standard

: $((available_card=0))

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_04

marc-hb · 2020-10-15T08:05:16Z

test-case/check-userspace-cardinfo.sh

+    dlogi "found card: $(echo "$card" | awk '{print $2}')"
+    usbcard=$(echo "$card" |grep "[Uu][Ss][Bb]")
+    if [ -z "$usbcard" ]; then
+        ((available_card++))


marc-hb · 2020-10-15T08:17:38Z

test-case/check-userspace-paplay.sh

+rate=${OPT_VALUE_lst['R']}
+channel=${OPT_VALUE_lst['C']}
+
+if [[ ! -e $file ]]; then


Avoid negations

Suggested change

if [[ ! -e $file ]]; then

[[ -e $file ]] || (dlogw "$file does not exist, use /dev/zero as dummy playback source" && file=/dev/zero )

marc-hb · 2020-10-15T08:18:03Z

test-case/check-userspace-paplay.sh

+sinkkeys=$(pactlinfo.py --showsinks)
+for round in $(seq 1 $round_cnt); do
+    for i in $sinkkeys; do
+        if ! sinkcard=$(pactlinfo.py --getsinkcardname "$i"); then


marc-hb · 2020-10-15T08:19:16Z

test-case/check-userspace-cardinfo.sh

+##    1. run pactl to get the cards info
+##    2. check the card name
+## Expect result:
+##    There is at least one available card for test


I don't see what this test achieves. Other tests also check that pulseaudio is functional and they should also fail if zero card is found so I think this test can be removed.

there are several cards and we only care sof cards. Other user space test cases won't check it and return correctly.

Other user space test cases won't check it and return correctly.

This is wrong, all tests that need an sof card to test should either FAIL or SKIP (exit 2) when there is no SOF card. A test should never return PASS after testing nothing.

test-case/check-userspace-paplay.sh

tools/pactlinfo.py

xiulipan

Please apply some suggestion from marc and run spellcheck for advice.
PS: we may take tare about scripts from other repo
If it is just some string handler and conversion, maybe write a version by our self is better.

xiulipan · 2020-10-16T07:22:15Z

test-case/check-userspace-paplay.sh

+rate=${OPT_VALUE_lst['R']}
+channel=${OPT_VALUE_lst['C']}
+
+if [[ ! -e $file ]]; then


Suggested change

if [[ ! -e $file ]]; then

[[ -e $file ]] || (dlogw "$file does not exist, use /dev/zero as dummy playback source" && file=/dev/zero )

libinyang · 2020-10-20T02:28:05Z

patch updated:

fix the license issue
switch if ! ... to other code style
refine the code based on other review comments.

set -e is still not used because if it is used, the case will fail to run.

xiulipan · 2020-10-20T03:48:44Z

test-case/check-userspace-cardinfo.sh

+## Expect result:
+##    There is at least one available card for test
+##
+


try grep ABC || true

xiulipan · 2020-10-20T03:54:50Z

test-case/check-userspace-cardinfo.sh

+    # pactl list cards short format should be like:
+    # 0	alsa_card.pci-0000_00_1f.3-platform-sof_sdw	module-alsa-card.c
+    dlogi "found card: $(echo "$card" | awk '{print $2}')"
+    echo "$card" |grep "[Uu][Ss][Bb]" &>/dev/null || ((available_card++))


Suggested change

echo "$card" |grep "[Uu][Ss][Bb]" &>/dev/null || ((available_card++))

echo "$card" |grep -i usb &>/dev/null || ((available_card++))

So we will ignore the USB card here?

yes. USB is not our scope

xiulipan · 2020-10-20T05:38:07Z

test-case/check-userspace-cardinfo.sh

+dlogc "pactl list cards short"
+cardlist=$(pactl list cards short)
+((available_card=0))
+IFS=$'\n'


https://unix.stackexchange.com/questions/275794/iterating-over-multiple-line-string-stored-in-variable
echo "$variable" | while IFS= read -r line ; do echo $line; done

xiulipan · 2020-10-20T06:17:41Z

test-case/check-userspace-parecord.sh

+rate=${OPT_VALUE_lst['R']}
+channel=${OPT_VALUE_lst['C']}
+
+[[ -e $file ]] || { dlogw "$file does not exist, use /dev/zero as dummy playback source" && file=/dev/zero; }


capture should use dev/null as dummy file to write into.

good catch. Thanks. Fixed in the latest update

https://unix.stackexchange.com/questions/275794/iterating-over-multiple-line-string-stored-in-variable
echo "$variable" | while IFS= read -r line ; do echo $line; done

I don't why: if I use this such while xxx, the 'available_card' seems to be changed locally and it will remain to 0 out of the loop. So I still keep the original code.

xiulipan · 2020-10-20T06:17:59Z

test-case/check-userspace-parecord.sh

+        }
+
+        # Let's skip testing on USB card
+        if echo "$sourcecard" |grep "[Uu][Ss][Bb]" &>/dev/null; then


grep -i usb is enough
PS: do we need a ignore list or filter list? Three will also be legacy HDA card and other graphic HDMI card.

I added a "TODO" for the filter list. Currently we don't meet such platform. How about we implement it when we meet such platforms?

Add the helper function to check whether pulseaudio runs properly or not. Signed-off-by: Libin Yang <libin.yang@intel.com>

libinyang · 2020-10-22T07:39:53Z

patch updated to add set -e

Check the pulseaudio card info to make sure the card is ready. Signed-off-by: Libin Yang <libin.yang@intel.com>

Add the tool pactlinfo.py to parse the pactl list information Signed-off-by: Libin Yang <libin.yang@intel.com>

This test case will go through all the sinks on the tested cards, and try to paplay it if the active port is not "not available" Signed-off-by: Libin Yang <libin.yang@intel.com>

This test case will go through all the sources on the tested cards, and try to parecord it if the active port is not "not available" Signed-off-by: Libin Yang <libin.yang@intel.com>

marc-hb

In the test cases, why do we care the interactive shell?

It's often useful to copy/paste to the shell for interactive testing. Anyway that is not the main point: it's just good practice to avoid negations because they require a little bit more reading/review effort. Avoid negations as a good practice also tends to avoid double negations (e.g: if ! nocodec_cmode ) which require even more reading effort for no value. I don't think I would not approve a PR just because of negations (see what I just did?)

I don't why: if I use this such while xxx, the 'available_card' seems to be changed locally and it will remain to 0 out of the loop. So I still keep the original code.

Please try to debug this, "I don't why it does not work" is not acceptable. If you can isolate and reproduce in a small function then share that function and I will debug it.

marc-hb · 2020-10-27T00:21:33Z

test-case/check-userspace-cardinfo.sh

+## Expect result:
+##    There is at least one available card for test
+##
+


See detailed set -e guidelines in #312

marc-hb · 2020-10-27T00:25:00Z

test-case/check-userspace-cardinfo.sh

+##    1. run pactl to get the cards info
+##    2. check the card name
+## Expect result:
+##    There is at least one available card for test


Other user space test cases won't check it and return correctly.

This is wrong, all tests that need an sof card to test should either FAIL or SKIP (exit 2) when there is no SOF card. A test should never return PASS after testing nothing.

marc-hb

I still use "&>dev/null" because error message pactl stat doesn't show explicit information that pactl stat fails. It doesn't help for developer to identity which fails quickly. Instead, I will use dloge to export the useful information for sof-test developer.

pactl error messages can be googled. So even when they're really really bad (which I suspect is rare), they are still useful because the reader can very quickly see what command failed: a pactl command.
If you think some pactl error message is really bad then feel free to ADD your own message but keep the original pact message too.
Last but not least: how can you even know in advance all the possible error messages that this pactl command will produce? Did you read the entire pactl source code? People look at test logs only when something unexpected happened, do you have a complete list of all the unexpected things that can happen?

Never, ever discard stderr, period.

Don't discard stdout either before it has become an actual annoyance. Even in that case it's best to redirect it to a file.

This is test code here, we're not trying to build slick iPhone interfaces. No one looks at test logs unless the tests fail. When tests fail people need and want as much original and googlable information as possible, they absolutely do not want alternative error messages hiding the original messages. No one care about "beautiful" test logs.

marc-hb · 2020-10-27T00:58:59Z

Can you please start a new PR? This one is overloaded with many comments that don't apply any more.

Please also run pylint on the new Python script. It doesn't have to be completely warning-free but it should have a low enough number.

We don't have a defined policy for formatting python code but I recommend black

libinyang · 2020-10-27T05:20:24Z

try to debug this, "I don't why it does not work" is not

Answer from Google shows: When you pipe into a while loop in Bash, it creates a subshell.
https://stackoverflow.com/questions/4667509/shell-variables-set-inside-while-loop-not-visible-outside-of-it

libinyang · 2020-10-27T05:40:18Z

pactl error messages can be googled. So even when they're really really bad (which I suspect is rare), they are still useful because the reader can very quickly see what command failed: a pactl command.

If you think some pactl error message is really bad then feel free to ADD your own message but keep the original pact message too.

Last but not least: how can you even know in advance all the possible error messages that this pactl command will produce? Did you read the entire pactl source code? People look at test logs only when something unexpected happened, do you have a complete list of all the unexpected things that can happen?

Do we need to understand entire source code when we use the tool? Sorry, I'm surprised to know that. What I think is that we and the user don't need to debug the pactl, pulseaudio tools. So I don't export all the error message to users. When I hide the error message, I don't say the error message is useless. The error message is useful to root cause why pactl stat fails. But we don't want to debug why pactl stat fails, do we? My opinion is to decide whether to show the error message case by case. It's not a good idea to mandate to always show the error messages or never show the error messages.

libinyang · 2020-10-27T06:03:35Z

Can you please start a new PR? This one is overloaded with many comments that don't apply any more.

Sure, I will create a new PR later.

Please also run pylint on the new Python script. It doesn't have to be completely warning-free but it should have a low enough number.

Good suggestion. I will try this tool.

We don't have a defined policy for formatting python code but I recommend black

Sorry, I don't understand black means here. Could you please give a little more hints?

libinyang · 2020-10-27T08:14:56Z

It's often useful to copy/paste to the shell for interactive testing. Anyway that is not the main point: it's just good practice to avoid negations because they require a little bit more reading/review effort. Avoid negations as a good practice also tends to avoid double negations (e.g: if ! nocodec_cmode ) which require even more reading effort for no value. I don't think I would not approve a PR just because of negations (see what I just did?)

Just find if ! mycommand; then ... is a recommend format in https://github.com/koalaman/shellcheck/wiki/SC2181

libinyang · 2020-11-02T06:40:29Z

open a new thread #479 and close this one

marc-hb · 2020-11-05T02:23:59Z

Answer from Google shows: When you pipe into a while loop in Bash, it creates a subshell.
https://stackoverflow.com/questions/4667509/shell-variables-set-inside-while-loop-not-visible-outside-of-it

There are a number of ways to solve this: https://mywiki.wooledge.org/BashFAQ/024

See process substitution example in tools/CI/shellcheck-gitrange.bash (commit faadb20)

Sorry, I don't understand black means here. Could you please give a little more hints?

apt-get install black
black something.py # re-formats in place

Just find if ! mycommand; then ... is a recommend format in https://github.com/koalaman/shellcheck/wiki/SC2181

No, that page does not "recommend" this format because that page is about $? which is a different issue. This page uses if ! command because it focuses between the difference between the "bad" and "good" code for the $?
issue and it does not want to distract the page from a much more minor and unrelated codestyle issue.

if ! assert_something; then fail works fine but assert_something || fail is more readable for a number of reasons, most which I already explained a couple times here and there. The main reason is that it reads like plain English ("do or die") and looks more like a C assert. It's also one of the most common shell idioms, try grep -R -F ' || ' /usr/bin/ and see other examples in

https://github.com/koalaman/shellcheck/wiki/SC2164
https://github.com/koalaman/shellcheck/wiki/SC2065
https://github.com/koalaman/shellcheck/wiki/SC2103

https://mywiki.wooledge.org/BashPitfalls#cd_.2Ffoo.3B_bar

Code reviews include both required changed and not required changes, "avoid" negations is not required change, it's general advice to make your submissions easier and faster to review.

set -e is still not used because if it is used, the case will fail to run.

I added detailed guidelines and some tips in #312. If that's not enough for some reason please ask me for help. I think this is very important so I will prioritize it.

marc-hb · 2020-11-05T05:21:52Z

Do we need to understand entire source code when we use the tool?

Only if you discard stderr. Don't discard stderr if you don't want to have to understand the whole pactl source code.

So I don't export all the error message to users. When I hide the error message, I don't say the error message is useless.

You've seen ONE error message in one situation and you're wrongly assuming pactl has no other error message. The entire purpose of test code is to find as many errors as possible in as many places and situations as possible so considering only "the" one error message you've seen so far makes no sense.

But we don't want to debug why pactl stat fails, do we?

This PR was named "add user space test cases" yet you would do not want to observe and try to understand some unexpected pactl failure?!

It's not a good idea to mandate to always show the error messages or never show the error messages.

Especially for test code yes: it is a "good idea" to never discard stderr except in some extremely rare (and well justified) cases. In fact it's absolutely critical for test code.

The user interface of test code is the exact opposite of the user interface of an iPhone: it has to be 100% transparent so users can totally and immediately see through absolutely everything it does. Users look at test logs ONLY when something unexpected happened and then they want to minimize guessing time as much as possible.

Such complete transparency is even more important when running in CI because it's often difficult to reproduce issues in different environment with no ability to interact directly.

marc-hb · 2020-11-05T18:23:03Z

The user interface of test code is the exact opposite of the user interface of an iPhone: it has to be 100% transparent so users can totally and immediately see through absolutely everything it does. Users look at test logs ONLY when something unexpected happened and then they want to minimize guessing time as much as possible.

Such complete transparency is even more important when running in CI because it's often difficult to reproduce issues in different environment with no ability to interact directly.

Only one day later here's one example why sof-test should be as close as possible to interactive commands : #495 (comment)

libinyang requested a review from a team as a code owner September 7, 2020 06:31

marc-hb requested changes Sep 8, 2020

View reviewed changes

marc-hb added the pulseaudio label Sep 8, 2020

xiulipan suggested changes Sep 8, 2020

View reviewed changes

test-case/check-userspace-cardinfo.sh Show resolved Hide resolved

test-case/check-userspace-cardinfo.sh Outdated Show resolved Hide resolved

test-case/check-userspace-pa-status.sh Outdated Show resolved Hide resolved

test-case/check-userspace-packages.sh Outdated Show resolved Hide resolved

marc-hb requested a review from plbossart September 9, 2020 06:37

libinyang force-pushed the user_space branch from 0e67009 to e8bdc53 Compare September 9, 2020 08:57

xiulipan suggested changes Sep 18, 2020

View reviewed changes

test-case/check-userspace-cardinfo.sh Show resolved Hide resolved

test-case/check-userspace-parecord.sh Outdated Show resolved Hide resolved

tools/pactlinfo.py Outdated Show resolved Hide resolved

plbossart reviewed Sep 18, 2020

View reviewed changes

tools/pactlinfo.py Outdated Show resolved Hide resolved

libinyang force-pushed the user_space branch from e8bdc53 to 2128553 Compare October 14, 2020 05:06

xiulipan approved these changes Oct 15, 2020

View reviewed changes

marc-hb requested changes Oct 15, 2020

View reviewed changes

xiulipan suggested changes Oct 16, 2020

View reviewed changes

libinyang force-pushed the user_space branch from 2128553 to 273f7e3 Compare October 20, 2020 02:23

xiulipan suggested changes Oct 20, 2020

View reviewed changes

case-lib: add helper function to check pulseaudio status

258470c

Add the helper function to check whether pulseaudio runs properly or not. Signed-off-by: Libin Yang <libin.yang@intel.com>

libinyang force-pushed the user_space branch from 273f7e3 to 9147aef Compare October 22, 2020 07:39

libinyang added 4 commits October 22, 2020 15:41

userspace: add check-userspace-cardinfo

0667c56

Check the pulseaudio card info to make sure the card is ready. Signed-off-by: Libin Yang <libin.yang@intel.com>

tools: add pactlinfo

92e63ee

Add the tool pactlinfo.py to parse the pactl list information Signed-off-by: Libin Yang <libin.yang@intel.com>

userspace: add check-userspace-paplay

c96535c

This test case will go through all the sinks on the tested cards, and try to paplay it if the active port is not "not available" Signed-off-by: Libin Yang <libin.yang@intel.com>

userspace: add check-userspace-parecord

8977528

This test case will go through all the sources on the tested cards, and try to parecord it if the active port is not "not available" Signed-off-by: Libin Yang <libin.yang@intel.com>

libinyang force-pushed the user_space branch from 9147aef to 8977528 Compare October 22, 2020 08:39

marc-hb reviewed Oct 27, 2020

View reviewed changes

marc-hb requested changes Oct 27, 2020

View reviewed changes

libinyang mentioned this pull request Nov 2, 2020

add User space test cases (version2) #479

Merged

libinyang closed this Nov 2, 2020

marc-hb added the False Pass / green failure label Jul 3, 2021

marc-hb mentioned this pull request May 5, 2025

[SKIP SOFCI-TEST] topology: build using a local ALSA install thesofproject/sof#9870

Merged

	if [[ ! -e $file ]]; then
	[[ -e $file ]] \|\| (dlogw "$file does not exist, use /dev/zero as dummy playback source" && file=/dev/zero )

	echo "$card" \|grep "[Uu][Ss][Bb]" &>/dev/null \|\| ((available_card++))
	echo "$card" \|grep -i usb &>/dev/null \|\| ((available_card++))

add User space test cases #363

add User space test cases #363

Uh oh!

Conversation

libinyang commented Sep 7, 2020

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiulipan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

libinyang commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiulipan commented Sep 9, 2020

Uh oh!

marc-hb commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

libinyang commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Sep 9, 2020

Uh oh!

libinyang commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

libinyang commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Sep 9, 2020

Uh oh!

libinyang commented Sep 15, 2020

Uh oh!

libinyang commented Sep 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

libinyang commented Oct 14, 2020

Uh oh!

xiulipan left a comment

Choose a reason for hiding this comment

Uh oh!

libinyang commented Oct 15, 2020

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!

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!

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

libinyang commented Sep 9, 2020 •

edited

Loading

marc-hb commented Sep 9, 2020 •

edited

Loading

libinyang commented Sep 9, 2020 •

edited

Loading

libinyang commented Sep 9, 2020 •

edited

Loading

libinyang commented Sep 9, 2020 •

edited

Loading