check-pause-release-suspend-resume: timing fixes + switch to sof-tplgreader #885

keqiaozhang · 2022-03-25T04:49:51Z

No description provided.

plbossart

can this be added to run-all-tests.sh?

We need all developpers to replicate what CI does.

marc-hb

Since you're rewriting most of the test, please take the opportunity to move all the code to functions, see why in #740. At least one main() function, maybe others too.

keqiaozhang · 2022-03-29T04:09:52Z

can this be added to run-all-tests.sh?

We need all developpers to replicate what CI does.

Done.

keqiaozhang · 2022-03-29T04:10:19Z

Since you're rewriting most of the test, please take the opportunity to move all the code to functions, see why in #740. At least one main() function, maybe others too.

PR is updated.

marc-hb

The breakdown into multiple commits was a nice try but it didn't really work:

The first commit message talks about sof-tplgreader only but it has tons of other, unrelated changes.
The move to functions should have come first (but now it's too late).

You can still try to keep the "small" commits as independent commits if possible but please squash the 2 big commits into a single big commit, it's just simpler and more "honest".

Keep the run-all-tests.sh commit separate of course.

marc-hb · 2022-03-29T19:29:06Z

test-case/check-pause-release-suspend-resume.sh

-file_name=${OPT_VAL['F']}
+parse_param()
+{
+    func_opt_parse_option $@


shellcheck, easy

marc-hb · 2022-03-29T19:31:50Z

test-case/check-pause-release-suspend-resume.sh

-OPT_NAME['p']='pcm'          OPT_DESC['p']='audio pcm. Example: hw:0,0'
-OPT_HAS_ARG['p']=1             OPT_VAL['p']='hw:0,0'
+OPT_NAME['m']='mode'             OPT_DESC['m']='test mode. Example: playback; capture'
+OPT_HAS_ARG['m']=1               OPT_VAL['m']='playback'


Please remove all these whitespace changes in this area. They make it impossible to find the real changes.
https://wiki.openstack.org/wiki/GitCommitMessages#Things_to_avoid_when_creating_commits

Answering myself: reviewing commits individually avoids this problem.

marc-hb · 2022-03-29T19:34:35Z

test-case/check-pause-release-suspend-resume.sh

+
+run_test()
+{
+    for idx in $(seq 0 $((PIPELINE_COUNT - 1)))


You can save one indentation level by moving the for loop to the main() function.

Good suggestion. PR is updated.

marc-hb · 2022-03-29T19:41:16Z

test-case/check-pause-release-suspend-resume.sh

+    ret=$?
+    if [ $ret -ne 0 ]; then
+        sof-process-kill.sh || dlogw "sof-process-kill.sh failed"
+        exit $ret


Do we really want to preserve the exit code here? If it's 2 then it will be misinterpreted as a SKIP! We experienced this recently in some different place.

marc-hb · 2022-03-29T19:42:07Z

test-case/check-pause-release-suspend-resume.sh

+        sleep_opts="-l 1 -T ${OPT_VAL['T']}"
+    else
+        sleep_opts="-l 1"
+    fi


sleep_opts="-l 1" [ -z "${OPT_VAL['T']}" ] || sleep_opts+="-T ${OPT_VAL['T']}"

marc-hb

Shell script code looks good to me except for a couple shellcheck warnings easy to fix.

@plbossart or @ranj063 can you please review the test / expect logic? I think one of you requested this?

marc-hb · 2022-04-04T23:40:48Z

test-case/check-pause-release-suspend-resume.sh

+    do
+        run_test
+        if [ $? -ne 0 ]; then
+            sof-process-kill.sh || dlogw "Kill process catch error"


Suggested change

sof-process-kill.sh || dlogw "Kill process catch error"

sof-process-kill.sh || dlogw "sof-process-kill.sh cleanup failed"

marc-hb · 2022-04-04T23:45:11Z

test-case/check-pause-release-suspend-resume.sh

-OPT_NAME['p']='pcm'          OPT_DESC['p']='audio pcm. Example: hw:0,0'
-OPT_HAS_ARG['p']=1             OPT_VAL['p']='hw:0,0'
+OPT_NAME['m']='mode'             OPT_DESC['m']='test mode. Example: playback; capture'
+OPT_HAS_ARG['m']=1               OPT_VAL['m']='playback'


Answering myself: reviewing commits individually avoids this problem.

marc-hb · 2022-04-04T23:48:22Z

test-case/check-pause-release-suspend-resume.sh

+    for idx in $(seq 0 $((PIPELINE_COUNT - 1)))
+    do
+        run_test
+        if [ $? -ne 0 ]; then


As reported by shellcheck:

Suggested change

if [ $? -ne 0 ]; then

run_test || {

One possible reason to use ret=$? is when you want to exit $ret after cleanup but you're not doing that here.

marc-hb · 2022-04-04T23:49:18Z

test-case/check-pause-release-suspend-resume.sh

@@ -51,47 +51,50 @@ set -e
 ##  * No unexpected errors should be present in dmesg during or after test
 ##      completion.

+# shellcheck source=case-lib/lib.sh
 source $(dirname ${BASH_SOURCE[0]})/../case-lib/lib.sh


Last shellcheck warning / quoting issue.

Fixed.

No, not completely. Can you please run shellcheck locally before pushing? Especially after fixing shellcheck issues? It would have saved a lot of time.

sudo apt install shellcheck shellcheck -x test-case/check-pause-release-suspend-resume.sh

plbossart · 2022-04-05T00:06:43Z

can this be added to run-all-tests.sh?
We need all developpers to replicate what CI does.

Done.

I don't see any changes to run-all-tests.sh @keqiaozhang ?

marc-hb · 2022-04-05T00:11:41Z

I don't see any changes to run-all-tests.sh @keqiaozhang ?

That was moved to #886, not sure why. No big deal.

keqiaozhang · 2022-04-19T05:57:38Z

@plbossart @marc-hb @ranj063
Please help with the final review. If no objections, I will merge this PR by end of this week.

marc-hb · 2022-04-22T22:01:29Z

There's one shellcheck warning very easy to fix. We should not still be looking at trivial shellcheck warnings 1 month after the first version, this is a waste of time. Run shellcheck locally before submitting, it takes a split second.

warning and code cleanup fix one shellcheck warning and some code cleanup. Signed-off-by: Keqiao Zhang <[email protected]>

parse pipelines instead of specifying PCM to test Consistent with other test scripts to use sof-tplgreader to parse audio pipelines designed in tplg and use -m option to select part of them to test. Signed-off-by: Keqiao Zhang <[email protected]>

checking the aplay/arecord status after pause/release This interval is too short which may casue timing issue on some platforms. Extend the interval to 500ms to avoid such issues. Signed-off-by: Keqiao Zhang <[email protected]>

the sleep type for suspend/resuse There are 2 sleep types designed in /sys/power/mem_sleep add a -T option to specify which one you want to test. Signed-off-by: Keqiao Zhang <[email protected]>

pausing the audio stream There's a latency when starting audio stream, if pause the audio stream directly after it starts, it may lead to some unexpected errors. Add a delay to ensure that audio stream is in a normal state before pusing. Signed-off-by: Keqiao Zhang <[email protected]>

Move the code to a function to make the code easy to understand and maintain. Signed-off-by: Keqiao Zhang <[email protected]>

Add 2 new cases to run all test: 1. pause-release-suspend-resume-with-playback 2. pause-release-suspend-resume-with-capture Signed-off-by: Keqiao Zhang <[email protected]>

Addressed

marc-hb · 2022-04-27T21:03:02Z

@plbossart , @ranj063 , @lyakh could you please review the pause/release/suspend/resume logic?

plbossart · 2022-04-28T14:21:30Z

@plbossart , @ranj063 , @lyakh could you please review the pause/release/suspend/resume logic?

I don't speak shell fluently and I am unable to figure out what the 'logic' is in this heap of shell commands.

I will note however that I don't understand why the pause can only be done after a delay. What possible 'unexpected errors' could happen if we pause immediately after starting? It looks like we are masking a set of possible issues without a clear justification.

marc-hb · 2022-04-28T21:24:58Z

Note the vast majority of this test and PR is not written in shell script but in https://en.wikipedia.org/wiki/Expect

marc-hb · 2024-08-01T22:59:21Z

This test was finally rewritten in 006946c + follow-ups.

keqiaozhang requested a review from a team as a code owner March 25, 2022 04:49

plbossart requested changes Mar 25, 2022

View reviewed changes

marc-hb changed the title ~~refine test case: check-pause-release-suspend-resume~~ check-pause-release-suspend-resume: timing fixes + switch to sof-tplgreader Mar 27, 2022

marc-hb reviewed Mar 27, 2022

View reviewed changes

keqiaozhang force-pushed the s3ref branch from f4e0c64 to d10452e Compare March 29, 2022 04:08

marc-hb previously requested changes Mar 29, 2022

View reviewed changes

keqiaozhang force-pushed the s3ref branch from d10452e to eb15b5a Compare April 2, 2022 09:14

marc-hb mentioned this pull request Apr 4, 2022

run-all-tests: add pause-release-suspend-resume related cases #886

Closed

marc-hb reviewed Apr 5, 2022

View reviewed changes

keqiaozhang force-pushed the s3ref branch from eb15b5a to 3c36b11 Compare April 6, 2022 12:49

keqiaozhang added 7 commits April 26, 2022 19:13

check-pause-release-suspend-resume: fix the shellcheck

24de38f

warning and code cleanup fix one shellcheck warning and some code cleanup. Signed-off-by: Keqiao Zhang <[email protected]>

check-pause-release-suspend-resume: add a -T option to specify

8fb0258

the sleep type for suspend/resuse There are 2 sleep types designed in /sys/power/mem_sleep add a -T option to specify which one you want to test. Signed-off-by: Keqiao Zhang <[email protected]>

check-pause-release-suspend-resume: move the code to a function

909b76e

Move the code to a function to make the code easy to understand and maintain. Signed-off-by: Keqiao Zhang <[email protected]>

run-all-tests: add pause-release-suspend-resume related cases

61a1972

Add 2 new cases to run all test: 1. pause-release-suspend-resume-with-playback 2. pause-release-suspend-resume-with-capture Signed-off-by: Keqiao Zhang <[email protected]>

keqiaozhang force-pushed the s3ref branch from 3c36b11 to 61a1972 Compare April 26, 2022 12:06

keqiaozhang closed this Mar 15, 2024

marc-hb mentioned this pull request Mar 15, 2024

[BUG] multiple-pause-resume misses failure due to random wait times #694

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check-pause-release-suspend-resume: timing fixes + switch to sof-tplgreader #885

check-pause-release-suspend-resume: timing fixes + switch to sof-tplgreader #885

keqiaozhang commented Mar 25, 2022

plbossart left a comment

marc-hb left a comment

keqiaozhang commented Mar 29, 2022

keqiaozhang commented Mar 29, 2022

marc-hb left a comment

marc-hb Mar 29, 2022

keqiaozhang Apr 2, 2022

marc-hb Mar 29, 2022

marc-hb Apr 4, 2022

marc-hb Mar 29, 2022

keqiaozhang Apr 2, 2022

marc-hb Mar 29, 2022

marc-hb Mar 29, 2022

marc-hb left a comment

marc-hb Apr 4, 2022

marc-hb Apr 4, 2022

marc-hb Apr 4, 2022

keqiaozhang Apr 6, 2022

marc-hb Apr 4, 2022

keqiaozhang Apr 6, 2022

marc-hb Apr 6, 2022 •

edited

Loading

plbossart commented Apr 5, 2022

marc-hb commented Apr 5, 2022

keqiaozhang commented Apr 19, 2022

marc-hb commented Apr 22, 2022 •

edited

Loading

marc-hb commented Apr 27, 2022 •

edited

Loading

plbossart commented Apr 28, 2022

marc-hb commented Apr 28, 2022

marc-hb commented Aug 1, 2024

	sof-process-kill.sh \|\| dlogw "Kill process catch error"
	sof-process-kill.sh \|\| dlogw "sof-process-kill.sh cleanup failed"

check-pause-release-suspend-resume: timing fixes + switch to sof-tplgreader #885

check-pause-release-suspend-resume: timing fixes + switch to sof-tplgreader #885

Conversation

keqiaozhang commented Mar 25, 2022

plbossart left a comment

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

keqiaozhang commented Mar 29, 2022

keqiaozhang commented Mar 29, 2022

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

plbossart commented Apr 5, 2022

marc-hb commented Apr 5, 2022

keqiaozhang commented Apr 19, 2022

marc-hb commented Apr 22, 2022 • edited Loading

marc-hb commented Apr 27, 2022 • edited Loading

plbossart commented Apr 28, 2022

marc-hb commented Apr 28, 2022

marc-hb commented Aug 1, 2024

marc-hb Apr 6, 2022 •

edited

Loading

marc-hb commented Apr 22, 2022 •

edited

Loading

marc-hb commented Apr 27, 2022 •

edited

Loading