tests: fix bug: lack of option sof-logger in tests#1251
tests: fix bug: lack of option sof-logger in tests#1251arikgreen wants to merge 1 commit intothesofproject:mainfrom
Conversation
|
Duplicating the same Have you considered using Also, maybe that code could be moved to
This is missing quotes BTW, it should be:
|
|
@arikgreen any update for review comments ? |
Yes, you right duplicate same code isn't good technic. Maybe it is a right time to fix it and move the code to the lib.sh. |
sorry for late but I was sick for last week. |
arikgreen
force-pushed
the
arikgreen/fix-sof-logger-opt-val-issue
branch
from
3256720 to
2cd5085
Compare
|
@marc-hb I decided implement a better quick fix |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
LGTM but I think there's a less subtle way.
Also, the PR description is now completely out of date with the commit message (that's because of the unusual "force-push" way SOF uses GitHub zephyrproject-rtos/zephyr#39194 - I digress)
case-lib/lib.sh
Outdated
| { | ||
| # Disable logging when available... | ||
| if [ ${OPT_VAL['s']} -eq 0 ]; then | ||
| if [[ ${OPT_VAL['s']} -eq 0 ]]; then |
There was a problem hiding this comment.
That difference between [ ] and [[ ]] is too subtle IMHO, I think most people won't know it. You could just do this instead: Wrong suggestion, see below.
| if [[ ${OPT_VAL['s']} -eq 0 ]]; then | |
| if [ "${OPT_VAL['s']}" = 0 ]; then |
There was a problem hiding this comment.
This should be correct but please test it:
| if [[ ${OPT_VAL['s']} -eq 0 ]]; then | |
| if [ -n "${OPT_VAL['s']}" ] && [ "${OPT_VAL['s']}" -eq 0 ]; then |
There was a problem hiding this comment.
your both suggestions are wrong, both return
-bash: 's': syntax error: operand expected (error token is "'s'")
[[ ... ]] - it fixed current error in this script. But you are right an undefined and a default value should enable sof_logger, it's easy to fixed some tests have set default value and for other we can check if ['s'] is set if not we can set to 1.
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
Actually, your [[ ]] solution and my = solution are both wrong.
-
All tests that support the
-sflag haveOPT_VAL['s']equals to1by default. Because the default is to collect logs when you don't pass-s(sorry for the double and triple negations, not all of them sof-test's fault) -
Tests that do NOT support
-sflag and haveOPT_VAL['s']undefined must of course consistently default to collecting logs too.
So, undefined OPT_VAL['s'] must be equivalent to OPT_VAL['s'] = 1: both must collect logs by default. The current code emits an ugly -eq warning but it is functionally correct.
Both the [[ ]] solution and my = solution make undefined equivalent to 0, which stops collecting logs and hides errors in tests that do not support -s. It would unfortunately not be the first time:
- #373
- #297
- https://github.com/thesofproject/sof-test/issues?q=label%3A%22False%20Pass%20%2F%20green%20failure%22
How did you test this?
arikgreen
force-pushed
the
arikgreen/fix-sof-logger-opt-val-issue
branch
from
2cd5085 to
497ab86
Compare
| return 1 | ||
| } | ||
|
|
||
| set_default_param_for_sof_logger() |
There was a problem hiding this comment.
I don't understand what problem this function solves. Is there some other code looking at OPT_VAL['s']? There shouldn't be.
There was a problem hiding this comment.
as I know at this moment no. But I didn't want add setting options in logger_disabled function. IMO it is more readable.
There was a problem hiding this comment.
For sure the OPT_NAME and OPT_DESC definitions here are never going to be used, so it's quite confusing to find them defined here. OPT_NAME and OPT_DESC are only used by the option parser and the option parser is never going to be run twice.
case-lib/lib.sh
Outdated
| # Disable logging when available... | ||
| if [ ${OPT_VAL['s']} -eq 0 ]; then | ||
| return 0 | ||
| if [ ${OPT_VAL['s']} ]; then |
There was a problem hiding this comment.
| if [ ${OPT_VAL['s']} ]; then | |
| if [ -n "${OPT_VAL['s']}" ]; then |
Not necessary in this particular case but more readable and safer in general. Variable content can start with some -x option or some other special stuff.
There was a problem hiding this comment.
yes, you may be right
|
Github "drafts" have mostly obsoleted "DNM" labels. You physically cannot merge a draft and there are some other benefits. |
Fix for: ``` /home/ubuntu/sof-test/test-case/../case-lib/lib.sh: line 955: [: -eq: unary operator expected ``` Also set default value for an arg -s for the tests which not set it. Signed-off-by: Artur Wilczak <arturx.wilczak@intel.com>
arikgreen
force-pushed
the
arikgreen/fix-sof-logger-opt-val-issue
branch
from
497ab86 to
619257b
Compare
3 participants
fix the issue:
sof-test/case-lib/lib.sh: line 955: [: -eq: unary operator expected