-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test-speaker: add argument for loop count #677
base: main
Are you sure you want to change the base?
Conversation
The existed '-l' is for nloops of speaker-test, which is used to configure the loop number of left/right .wav files in each playback, it is not suitable for the loop count for the speaker-test stress. Change the old speaker-test one to '-L' and add new '-l' for stress test purpose, to make it consistent with stress test of other cases. Signed-off-by: Keyon Jie <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch and improvement. Thank you for bring back '-l' option for the test! (and still keeping speaker-test's -l option)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't add new shellcheck warnings. You don't have to fix all warnings if you want to keep your PR small and focused but please don't increase the number of warnings.
|
||
dlogc "speaker-test -D $dev -r $rate -c $channel -f $fmt -l $tcnt -t wav -P 8" | ||
speaker-test -D $dev -r $rate -c $channel -f $fmt -l $tcnt -t wav -P 8 2>&1 |tee $LOG_ROOT/result_$loop_$idx.txt | ||
resultRet=$? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize the code was already like this before this PR but here resultRet=$?
is not the exit code of speaker-test
, it's the exit code of the tee
command/ which is useless - more green failures and untested test code yet again.
Please make sure this test fails when speaker test fails and fix it first if it does not; there's no point enhancing a test that does not even report failures in the first place. A very simple way to test the test is to temporarily pass a broken parameter.
Isn't a simple speaker-test || die
enough? Do we really need these log files? Does speaker-test not use the exit code to report errors?
OPT_NAME['l']='loop' OPT_DESC['l']='stress loops count' | ||
OPT_HAS_ARG['l']=1 OPT_VAL['l']=1 | ||
|
||
OPT_NAME['L']='nloops' OPT_DESC['L']='option of speaker-test, loop number of .wav files, 0 = infinite' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What .wav files?
@@ -20,8 +20,11 @@ source $(dirname ${BASH_SOURCE[0]})/../case-lib/lib.sh | |||
OPT_NAME['t']='tplg' OPT_DESC['t']='tplg file, default value is env TPLG: $TPLG' | |||
OPT_HAS_ARG['t']=1 OPT_VAL['t']="$TPLG" | |||
|
|||
OPT_NAME['l']='loop' OPT_DESC['l']='option of speaker-test' | |||
OPT_HAS_ARG['l']=1 OPT_VAL['l']=3 | |||
OPT_NAME['l']='loop' OPT_DESC['l']='stress loops count' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"stress loops count" would apply to either option, please use a description that makes a difference with the other option. Maybe something like "How many times speaker-test is run" versus "speaker-test --nloops option"
Can one of the admins verify this patch?
|
The existed '-l' is for nloops of speaker-test, which is used to
configure the loop number of left/right .wav files in each playback, it
is not suitable for the loop count for the speaker-test stress.
Change the old speaker-test one to '-L' and add new '-l' for stress test
purpose, to make it consistent with stress test of other cases.
Signed-off-by: Keyon Jie [email protected]