Explicitly invoke start_test() in every test-case/*.sh #1187

marc-hb · 2024-05-01T21:10:30Z

2 commits, main one:

Stop unconditionally imposing start_test() to everything sourcing lib.sh

This makes it possible to source lib.sh without running anything which
can be useful for debugging.

For now this should be a no-op for all tests except two files in tools/ which
were never really "tests" in the first place:

tools/kmod/sof_remove.sh
tools/sof-kernel-log-check.sh

In these, start_test() never made sense it never really run thanks the
(awkward) "is_subtest()" escape.

There are other tests where start_test() should not be invoked either,
most notably test-case/verify-kernel-boot-log.sh in order to fix
#1112 but this is a huge
commit already; one change at a time.

Long overdue. Zero code change. Signed-off-by: Marc Herbert <[email protected]>

Stop unconditionally imposing start_test() to everything sourcing lib.sh This makes it possible to source lib.sh without running anything which can be useful for debugging. For now this should be a no-op for all tests except two files in `tools/` which were never really "tests" in the first place: - tools/kmod/sof_remove.sh - tools/sof-kernel-log-check.sh In these, start_test() never made sense it never really run thanks the (awkward) "is_subtest()" escape. There are other tests where start_test() should not be invoked either, most notably test-case/verify-kernel-boot-log.sh in order to fix thesofproject#1112 but this is a huge commit already; one change at a time. Signed-off-by: Marc Herbert <[email protected]>

marc-hb · 2024-05-02T00:09:46Z

This is the one-before-last, big step towards fixing:

[BUG] empty kernel logs on MAX_WAIT_FW_LOADING failures #1112

... and friends.

Known headset capture issue thesofproject/sof#9018 in LNL https://sof-ci.01.org/softestpr/PR1187/build383/devicetest/index.html, everything else green.

One single suspend/resume failure in CAVS https://sof-ci.01.org/softestpr/PR1187/build381/devicetest/index.html, everything else green.

MTL all green https://sof-ci.01.org/softestpr/PR1187/build382/devicetest/index.html

stable-v2.2 all green https://sof-ci.01.org/softestpr/PR1187/build384/devicetest/index.html

All the shellcheck warnings in https://github.com/thesofproject/sof-test/actions/runs/8915588945/job/24485498298?pr=1187 are unrelated

ujfalusi · 2024-05-02T12:05:41Z

case-lib/lib.sh

+# 3. Register the func_exit_handler() EXIT trap that collects logs, kills
+#    loggers and prints test results.
+#
+# 4. Waits for the FW to be successfully booted (can be disabled)


How can it be disabled?

NO_POLL_FW_LOADING 30 lines below, it's not that long of a function (as no function should be)

Also documented in config.sh

ujfalusi · 2024-05-02T12:06:19Z

case-lib/lib.sh

+#
+# 4. Waits for the FW to be successfully booted (can be disabled)
+#
+#


nitpick: one blank line should be enough?

agree it's nitpick

ujfalusi · 2024-05-02T12:09:37Z

test-case/check-alsabat.sh

@@ -73,6 +73,8 @@ frequency=${OPT_VAL['F']}
 sigmak=${OPT_VAL['k']}
 frames=${OPT_VAL['n']}

+start_test
+


In commit message, what is the meaning of the opening sentence in relation to moving the start_test from lib.sh to test cases?
Stop unconditionally imposing start_test() to everything sourcing lib.sh

I'm not sure I understand the question...
Now it's explicit, so each test has a choice to call start_test or not. That's all this PR is about.

When start_test was in lib.sh, tests had no choice but to run start_test

start_test started as a small thing so running it unconditionally was a "small bug" but it has kept growing.

ujfalusi · 2024-05-02T12:17:38Z

test-case/check-audio-equalizer.sh

@@ -41,6 +41,8 @@ loop_cnt=${OPT_VAL['l']}
 func_pipeline_export "$tplg" "eq:any"
 sofcard=${SOFCARD:-0}

+start_test


Isn't the start_test used to run at line 21?

Yes this is a minor change: start_test is now running after option parsing. It was confusing to "start" a test and run the exit handler, etc. when no testing had started yet because a wrong option was passed.

ujfalusi · 2024-05-02T12:21:06Z

test-case/check-capture.sh

@@ -57,6 +57,7 @@ loop_cnt=${OPT_VAL['l']}
 out_dir=${OPT_VAL['o']}
 file_prefix=${OPT_VAL['f']}

+start_test


All of this churn is to keep the systemctl_show_pulseaudio in tools/kmod/sof_remove.sh instead of just dropping it from the sof_remove.sh along with sourcing the case-lib/lib.sh and be done?

No, the main benefit for sof_remove.sh is the ability to run it more than once. It used to fail when the firmware wasn't running. Also avoids some confusing logs: sof_remove.sh is not a "test"

There will be a much bigger and much more important bug fix not related to sof_remove.sh, see commit message.

Also restores the ability to source lib.sh interactively, also mentioned in the commit message

instead of just dropping it from the sof_remove.sh along with sourcing the case-lib/lib.sh

The (non-mandatory) pulseaudio check is debatable. Including lib.sh is much less so. There's no reason to copy/paste routines when it can easily be avoided.

Anyway the most important and urgent benefit of this design fix is preparing the fix for #1036 and #1112. The same design fix just happens to help with some other scripts like sof_remove.sh.

fredoh9 · 2024-05-06T17:51:57Z

test-case/verify-kernel-boot-log.sh

@@ -18,6 +18,11 @@ set -e
 # shellcheck source=case-lib/lib.sh
 source "$(dirname "${BASH_SOURCE[0]}")"/../case-lib/lib.sh

+start_test
+# TODO: replace start_test with this:


This TODO will be next PR?

Yes immediately after this one. I want at least one run of daily tests between the two.

fredoh9

Initially start_test from lib.sh was making sense. but now it doesn't cover every case.

Explicit invoking start_test() is making sense now.

marc-hb · 2024-05-06T18:13:30Z

Thanks @fredoh9 !

marc-hb added 2 commits May 1, 2024 14:02

lib.sh: document more recent start_test() additions

8ab82ba

Long overdue. Zero code change. Signed-off-by: Marc Herbert <[email protected]>

marc-hb force-pushed the explicit-start-step branch from 9d02ef8 to a82935b Compare May 1, 2024 22:08

marc-hb marked this pull request as ready for review May 2, 2024 00:10

marc-hb requested a review from a team as a code owner May 2, 2024 00:10

marc-hb mentioned this pull request May 2, 2024

kmod: sof_remove: Drop Pulseaudio test and stop including case-lib/li… #1169

Closed

marc-hb requested review from ujfalusi, fredoh9 and kv2019i May 2, 2024 00:12

ujfalusi reviewed May 2, 2024

View reviewed changes

fredoh9 reviewed May 6, 2024

View reviewed changes

fredoh9 approved these changes May 6, 2024

View reviewed changes

marc-hb merged commit 7c2ede0 into thesofproject:main May 6, 2024
4 of 7 checks passed

marc-hb deleted the explicit-start-step branch May 6, 2024 18:13

marc-hb mentioned this pull request Jul 16, 2024

sof_remove.sh: make lib.sh optional #1220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicitly invoke start_test() in every test-case/*.sh #1187

Explicitly invoke start_test() in every test-case/*.sh #1187

marc-hb commented May 1, 2024

marc-hb commented May 2, 2024 •

edited

Loading

ujfalusi May 2, 2024

marc-hb May 2, 2024

ujfalusi May 2, 2024

fredoh9 May 6, 2024

ujfalusi May 2, 2024

marc-hb May 2, 2024

ujfalusi May 2, 2024

marc-hb May 2, 2024

ujfalusi May 2, 2024

marc-hb May 2, 2024

marc-hb May 3, 2024 •

edited

Loading

fredoh9 May 6, 2024

marc-hb May 6, 2024

fredoh9 left a comment •

edited

Loading

marc-hb commented May 6, 2024

Explicitly invoke start_test() in every test-case/*.sh #1187

Explicitly invoke start_test() in every test-case/*.sh #1187

Conversation

marc-hb commented May 1, 2024

marc-hb commented May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredoh9 left a comment • edited Loading

Choose a reason for hiding this comment

marc-hb commented May 6, 2024

marc-hb commented May 2, 2024 •

edited

Loading

marc-hb May 3, 2024 •

edited

Loading

fredoh9 left a comment •

edited

Loading