check-suspend-resume: check process id in each loop for audio test case #1223
Conversation
|
the main problem I'm trying to fix with this PR is... |
| @@ -134,6 +136,10 @@ do | |||
| ps --ppid $$ -f | |||
| exit 1 | |||
| } | |||
| # remove -p $process_id option, Need to unset twice | |||
| unset "opt_arr[${#opt_arr[@]}-1]" | |||
| unset "opt_arr[${#opt_arr[@]}-1]" | |||
There was a problem hiding this comment.
You don't need this if you simply use a copy, see suspend_opts above.
Every time you change a variable in place, consider making a copy instead. It's always better for correctness and concurrency (it's the default in functional languages)
There was a problem hiding this comment.
EDIT: just read your comment above.
This seems a bit complicated... why not just check the process status when the subtest returns a failure? I mean here:
"${TESTDIR}"/check-suspend-resume.sh "${opt_arr[@]}" || {
# new code to check process status
die "suspend resume failed"
}
| @@ -134,6 +136,10 @@ do | |||
| ps --ppid $$ -f | |||
| exit 1 | |||
| } | |||
| # remove -p $process_id option, Need to unset twice | |||
| unset "opt_arr[${#opt_arr[@]}-1]" | |||
| unset "opt_arr[${#opt_arr[@]}-1]" | |||
There was a problem hiding this comment.
You don't need this if you simply use a copy, see suspend_opts above.
Every time you change a variable in place, consider making a copy instead. It's always better for correctness and concurrency (it's the default in functional languages)
test-case/check-suspend-resume.sh
Outdated
| if [ "${OPT_VAL['p']}" ]; then | ||
| dlogi "Check for the process status, pid: ${OPT_VAL['p']}" | ||
| sof-process-state.sh "${OPT_VAL['p']}" || { | ||
| #func_lib_lsof_error_dump "$snd" |
There was a problem hiding this comment.
Why commented out? This is a failure and it's very likely very hard to reproduce. We want as much information as possible. This will be called only once (and usually: zero)
There was a problem hiding this comment.
$snd is only available in check-suspend-resume-with-audio.sh
I put a FIXME comment
There was a problem hiding this comment.
So if this can never work then why have this commented out line present at all?
fredoh9
force-pushed
the
fix/suspend-resume-pid-check
branch
from
9e7a07e
to
0d9ce9c
Compare
test-case/check-suspend-resume.sh
Outdated
| if [ "${OPT_VAL['p']}" ]; then | ||
| dlogi "Check for the process status, pid: ${OPT_VAL['p']}" | ||
| sof-process-state.sh "${OPT_VAL['p']}" || { | ||
| #func_lib_lsof_error_dump "$snd" |
There was a problem hiding this comment.
So if this can never work then why have this commented out line present at all?
No functionality change. indentation chagne for option variables. One double quote reduce shellcheck error. Loop/round terms are used for iteration in test cases, this is loop. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>
check-suspend-resume-with-audio.sh uses check-suspend-resume.sh. process check is in check-suspend-resume-with-audio.sh and check-suspend-resume.sh has the loop. Must check that the process id is available in each loop. TO do so, pass aplay/arecord pid to check-suspend-resume.sh. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>
fredoh9
force-pushed
the
fix/suspend-resume-pid-check
branch
from
0d9ce9c
to
2313bb0
Compare
|
split into two commits, first commit has no functionality change. |
|
A good few shellcheck warnings in https://github.com/thesofproject/sof-test/actions/runs/10084785339/job/27884307459?pr=1223 Most seem old and all seem harmless but it's a pity to include a cleanup commit in this PR and still have that many warnings :-( |
check-suspend-resume-with-audio.sh uses check-suspend-resume.sh. process check is in check-suspend-resume-with-audio.sh and check-suspend-resume.sh has the loop. Must check that the process id is available in each loop. TO do so, pass aplay/arecord pid to check-suspend-resume.sh.