test-case: rename pause-resume to pause-release #1025

fredoh9 · 2023-04-14T21:01:03Z

resume term leads to confusion with system resume. Nothing to do with system suspend/resume test. pause-release is more clear.

fixes: #792

test plan in SOF CI system also need to rename the command also when this is merged.

marc-hb

I disagree sorry.

The only way to confuse "pause/resume" with suspend/resume is to ignore the first word! I've never seen anyone confusing "pause" with "suspend". So this is a whole lot of code churn and potential git conflicts for solving a minor issue.

The official man aplay calls it "pause/resume"

"release" is not the opposite of "pause", "release" is the opposite of "acquire". Morever, playing typically acquires resources while pausing releases them.

If other people think a change is really necessary then pause-unpause or pause-start would be better

kv2019i

A bit concerned with the transition time with existing scripts running check-pause-resume.sh getting broken with this -- see inline comment. Otherwise seems good.

test-case/check-pause-release.sh

plbossart · 2023-04-17T15:45:46Z

I support this change 100%, but the only want to avoid naming issues is to use the ALSA terminology:

check_pause_push_pause_release is a bit longer but will avoid any confusions.

And yes a symlink is required until all references to the old script are removed.

marc-hb · 2023-04-17T18:10:11Z

I support this change 100%, but the only want to avoid naming issues is to use the ALSA terminology:

Can you please share a link to the official ALSA terminology? I did a quick search but I only found "pause/resume" in man aplay.

check_pause_push_pause_release is a bit longer but will avoid any confusions.

What could the much shorter check-pause-unpause be confused with?

Now shellcheck clean before thesofproject#1025 Signed-off-by: Marc Herbert <[email protected]>

plbossart · 2023-04-17T18:28:31Z

https://elixir.bootlin.com/linux/latest/source/include/sound/pcm.h#L100

this is a kernel test, let's use kernel terminology.

We have similar confusions in SoundWire between clock_pause, stretch, stop, resume, restart.

Use one set of words clearly linked to a well-defined concept is key. Unpause is not such a word, sorry.

overruled

marc-hb · 2023-04-17T23:01:07Z

https://sof-ci.01.org/softestpr/PR1025/build285/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=multiple-pause-resume-50 (internal 23679) failed with a strange warning (that did not cause the failure)

=== PAUSE ===                                                            
(50/50) pcm'Deepbuffer Port0' cmd'aplay' id'8': Wait for 49 ms before pause

(50/50) pcm'Deepbuffer Port0' cmd'aplay' id'8': Wait for 33 ms before release

2023-04-17 22:31:56 UTC [REMOTE_INFO] pipeline: Port2 with arecord
2023-04-17 22:31:56 UTC [REMOTE_INFO] pipeline: Deepbuffer Port0 with aplay
2023-04-17 22:31:56 UTC [REMOTE_INFO] Check expect exit status
2023-04-17 22:31:56 UTC [REMOTE_WARNING] SOF_TEST_TOP_PID=17709 already defined, multiple lib.sh inclusions?
declare -- cmd="journalctl_cmd --since=@1681770512"
2023-04-17 22:31:56 UTC [REMOTE_ERROR] pause release PID 19337 had non-zero exit status
2023-04-17 22:31:56 UTC [REMOTE_ERROR] Starting func_exit_handler(), exit status=1, FUNCNAME stack:
2023-04-17 22:31:56 UTC [REMOTE_ERROR]  die()  @  /home/ubuntu/sof-test/test-case/../case-lib/lib.sh
2023-04-17 22:31:56 UTC [REMOTE_ERROR]  main()  @  /home/ubuntu/sof-test/test-case/multiple-pause-resume.sh:191
2023-04-17 22:31:57 UTC [REMOTE_INFO] pkill -TERM -f mtrace-reader.py
2023-04-17 22:31:57 UTC [REMOTE_INFO] ktime=1755 sof-test PID=17709: ending
2023-04-17 22:31:57 UTC [REMOTE_INFO] Test Result: FAIL!

test-case/check-pause-release.sh

Now shellcheck clean before #1025 Signed-off-by: Marc Herbert <[email protected]>

marc-hb · 2023-04-18T00:04:14Z

shellcheck warnings fixed in #1026, minor conflict now, needs rebase.

fredoh9 · 2023-04-19T17:38:23Z

rebased top of main

resume term leads to confusion with system resume. Nothing to do with system suspend/resume test. pause-release is more clear. Signed-off-by: Fred Oh <[email protected]>

To support backward compatibility, keep the symbolic link for the time being. check-pause-resume.sh -> check-pause-release.sh multiple-pause-resume.sh -> multiple-pause-release.sh Signed-off-by: Fred Oh <[email protected]>

marc-hb

Instead of the symbolic links you could use an exec like in test-case/multiple-pipeline-capture.sh which would give you the possibility of printing a deprecation warning.

fredoh9 · 2023-04-20T21:14:53Z

@marc-hb I take adding symbolic links is really short term solution. I could update the test case database in same time with the merge without the symbolic link. as soon as this merge.
I think adding a wrapper is over kill for real short time. I will submit a revert PR for the commit as soon as this merge.

marc-hb · 2023-04-20T21:24:53Z

There are always more users than you think. In fact there is already one in this repo that you missed, looks like you didn't git grep.

Renamed pause-resume to pause-release. Ref link: thesofproject#1025 Signed-off-by: Fred Oh <[email protected]>

fredoh9 · 2023-04-20T22:05:22Z

missed run-all-test.sh

fredoh9 · 2023-05-01T22:08:04Z

@marc-hb @plbossart @ranj063 do you have any comment for this PR?

fredoh9 requested a review from a team as a code owner April 14, 2023 21:01

marc-hb previously requested changes Apr 14, 2023

View reviewed changes

fredoh9 requested review from plbossart, ranj063, mengdonglin, ujfalusi, keqiaozhang, kv2019i and aiChaoSONG April 14, 2023 23:09

kv2019i reviewed Apr 17, 2023

View reviewed changes

test-case/check-pause-release.sh Outdated Show resolved Hide resolved

marc-hb added a commit to marc-hb/sof-test that referenced this pull request Apr 17, 2023

multiple-pause-resume.sh: fix two minor quote warnings

04165b7

Now shellcheck clean before thesofproject#1025 Signed-off-by: Marc Herbert <[email protected]>

marc-hb mentioned this pull request Apr 17, 2023

multiple-pause-resume.sh: fix two minor quote warnings #1026

Merged

ranj063 reviewed Apr 17, 2023

View reviewed changes

test-case/check-pause-release.sh Outdated Show resolved Hide resolved

marc-hb mentioned this pull request Apr 17, 2023

Tools: Add more robustness to check_volume_levels.m measure #985

Merged

marc-hb added a commit that referenced this pull request Apr 17, 2023

multiple-pause-resume.sh: fix two minor quote warnings

7a74c40

Now shellcheck clean before #1025 Signed-off-by: Marc Herbert <[email protected]>

fredoh9 force-pushed the fix/pause-release-rename branch from cb4d75e to aa43e38 Compare April 19, 2023 17:37

fredoh9 added 2 commits April 19, 2023 10:43

test-case: rename pause-resume to pause-release

c743658

resume term leads to confusion with system resume. Nothing to do with system suspend/resume test. pause-release is more clear. Signed-off-by: Fred Oh <[email protected]>

test-case: add symbolic link for pause-resume tests

e5f7ac1

To support backward compatibility, keep the symbolic link for the time being. check-pause-resume.sh -> check-pause-release.sh multiple-pause-resume.sh -> multiple-pause-release.sh Signed-off-by: Fred Oh <[email protected]>

plbossart previously approved these changes Apr 19, 2023

View reviewed changes

fredoh9 dismissed plbossart’s stale review via e5f7ac1 April 19, 2023 17:45

fredoh9 force-pushed the fix/pause-release-rename branch from aa43e38 to e5f7ac1 Compare April 19, 2023 17:45

fredoh9 requested review from ranj063 and kv2019i April 19, 2023 17:47

fredoh9 requested a review from marc-hb April 19, 2023 17:47

marc-hb reviewed Apr 19, 2023

View reviewed changes

run-all-test: rename pause-resume to pause-release

e577e28

Renamed pause-resume to pause-release. Ref link: thesofproject#1025 Signed-off-by: Fred Oh <[email protected]>

fredoh9 requested review from marc-hb and plbossart May 1, 2023 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test-case: rename pause-resume to pause-release #1025

test-case: rename pause-resume to pause-release #1025

fredoh9 commented Apr 14, 2023

marc-hb left a comment •

edited

Loading

kv2019i left a comment

plbossart commented Apr 17, 2023

marc-hb commented Apr 17, 2023

plbossart commented Apr 17, 2023

marc-hb commented Apr 17, 2023

marc-hb commented Apr 18, 2023 •

edited

Loading

fredoh9 commented Apr 19, 2023

marc-hb left a comment •

edited

Loading

fredoh9 commented Apr 20, 2023

marc-hb commented Apr 20, 2023

fredoh9 commented Apr 20, 2023

fredoh9 commented May 1, 2023

test-case: rename pause-resume to pause-release #1025

Are you sure you want to change the base?

test-case: rename pause-resume to pause-release #1025

Conversation

fredoh9 commented Apr 14, 2023

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

plbossart commented Apr 17, 2023

marc-hb commented Apr 17, 2023

plbossart commented Apr 17, 2023

marc-hb commented Apr 17, 2023

marc-hb commented Apr 18, 2023 • edited Loading

fredoh9 commented Apr 19, 2023

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

fredoh9 commented Apr 20, 2023

marc-hb commented Apr 20, 2023

fredoh9 commented Apr 20, 2023

fredoh9 commented May 1, 2023

marc-hb left a comment •

edited

Loading

marc-hb commented Apr 18, 2023 •

edited

Loading

marc-hb left a comment •

edited

Loading