check-pause-release-suspend-resume: timing fixes + switch to sof-tplgreader by keqiaozhang · Pull Request #885 · thesofproject/sof-test

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

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


shellcheck, easy

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

-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

+
+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

+    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

+        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

+    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

-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

+    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

 ##      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 <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

Move the code to a function to make the code easy to understand and maintain. Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

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 <keqiao.zhang@intel.com>

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

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

Conversation

keqiaozhang commented Mar 25, 2022

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

keqiaozhang commented Mar 29, 2022

Uh oh!

keqiaozhang commented Mar 29, 2022

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!

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!

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!

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!

marc-hb Apr 6, 2022 • 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!

plbossart commented Apr 5, 2022

Uh oh!

marc-hb commented Apr 5, 2022

Uh oh!

keqiaozhang commented Apr 19, 2022

Uh oh!

marc-hb commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Apr 28, 2022

Uh oh!

marc-hb commented Apr 28, 2022

Uh oh!

marc-hb commented Aug 1, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marc-hb Apr 6, 2022 •

edited

Loading

marc-hb commented Apr 22, 2022 •

edited

Loading

marc-hb commented Apr 27, 2022 •

edited

Loading