Optimize this case to support all the PCM #758
Conversation
…end-resume-release.sh. And optimize this case to support all the PCM. Signed-off-by: VickyX_Intel_Ubuntu <[email protected]>
|
|
||
| rm -rf /tmp/sof-test.lock | ||
| rm -rf /tmp/sof-test.lock |
There was a problem hiding this comment.
who will create /tmp/sof-test.lock?
There was a problem hiding this comment.
After verification, this code is useless, I will delete it.
| @@ -146,7 +145,7 @@ expect { | |||
| } | |||
|
|
|||
| #enter suspend-resume cycle once per pause instance | |||
| set retval [catch { exec bash check-suspend-resume.sh -l 1 } msg] | |||
| set retval [catch { exec bash /home/ubuntu/sof-test/test-case/check-suspend-resume.sh -l 1 } msg] | |||
There was a problem hiding this comment.
Instead of hard coded path, can we use BASH_SOURCE[0] in expect?
There was a problem hiding this comment.
Agreed, /home/ubuntu/ is not going to work for most people. It worked with a relative path before, what changed?
There was a problem hiding this comment.
I will change the code and use the BASH_SOURCE[0] to get the dirname.
|
Since you are touching this file, some of shellcheck errors/warnings are easy to fix. |
There was a problem hiding this comment.
Rename the case check-pause-suspend-resume-release.sh to check-pause-suspend-resume-release.sh.
(same name)
File renames are very painful because they can cause endless version control issues. Is this rename really necessary? I see no justification for it in the commit message.
If the rename is absolutely necessary, do the rename in a separate commit that only renames the file, this helps git identify the rename and reduces version control issues a little bit.
Optimize this case to support all the PCM #758
This is not an optimization. An optimization does the same thing faster or better in some way. This is adding a new feature: iterating no all PCMs (the same way as before).
| fi | ||
| sof-kernel-log-check.sh "$KERNEL_CHECKPOINT" | ||
| exit $? | ||
| ret=$? |
There was a problem hiding this comment.
Either you don't change that unrelated part of the code to minimize the size of the commit, or you change it and then you go all the way and fix the most obvious shellcheck warnings too (in a separate commit). Don't stop in the middle of the bridge.
There was a problem hiding this comment.
About the rename, because the original name is check-pause-release-suspend-resume.sh, it does not represent the test that our case will be done, so I changed it to check-pause-suspend-resume-release.sh. I will raise a PR to change the case name.
| exit $ret | ||
| fi | ||
| sof-kernel-log-check.sh "$KERNEL_CHECKPOINT" || die "Caught error in kernel log" | ||
| #exit $? |
There was a problem hiding this comment.
If you think it's really useful to leave code commented out, add a comment explaining why.
| @@ -146,7 +145,7 @@ expect { | |||
| } | |||
|
|
|||
| #enter suspend-resume cycle once per pause instance | |||
| set retval [catch { exec bash check-suspend-resume.sh -l 1 } msg] | |||
| set retval [catch { exec bash /home/ubuntu/sof-test/test-case/check-suspend-resume.sh -l 1 } msg] | |||
There was a problem hiding this comment.
Agreed, /home/ubuntu/ is not going to work for most people. It worked with a relative path before, what changed?
| # expect is tcl language script | ||
| # catch: Evaluate script and trap exceptional returns | ||
| # after ms: Ms must be an integer giving a time in milliseconds. | ||
| # The command sleeps for ms milliseconds and then returns. |
There was a problem hiding this comment.
If you change the indentation then take that opportunity to use more shell functions, see why in #740 . One of the function should be something like test_single_pcm()
Signed-off-by: VickyX_Intel_Ubuntu <[email protected]>
… be done. Rename the case name to check-pause-suspend-resume-release.sh. Signed-off-by: VickyX_Intel_Ubuntu <[email protected]>
There was a problem hiding this comment.
This PR is completely broken now, abandon this PR and start a new one.
The first commit still has a rename and code change in a single commit.
The second commit has a bit of code but does NOT add the for loop contrary to its commit message
The third commit does a second rename.
This is actually a good demonstration that renames always make version control much harder. A rename will also break any other script or CI engine that relies on the old name. I bet the new name is better but it's not so much better that it's worth these hassles, so forget about the rename for now.
Please also fix your git config to have your real name in the Signed-off field.
Rename the case check-pause-suspend-resume-release.sh to check-pause-suspend-resume-release.sh.
And optimize this case to support all the PCM.
Signed-off-by: VickyX_Intel_Ubuntu [email protected]