Re: [ndctl PATCH v3] cxl: Add cxl-translate.sh unit test

[Marc Herbert <marc.herbert@linux.intel.com>]

"Usual" disclaimer: only sharing my shell experience. Unfortunately
still cannot review the actual test logic.

Alison Schofield <alison.schofield@intel.com> writes:
> --- /dev/null
> +++ b/test/cxl-translate.sh
> @@ -0,0 +1,320 @@

> +# shellcheck disable=SC2034
> +#
> +# Arrays in this script are passed by name into helper functions using Bash's
> +# nameref feature `declare -n`. This pattern supports writing generic test
> +# harnesses that can iterate over many different test vector arrays simply by
> +# passing the array name. ShellCheck doesn't track nameref indirection, so it
> +# incorrectly reports these arrays as unused (SC2034). At runtime they are
> +# fully used through the nameref, so these warnings are safe to ignore.

As mentioned before in Message-ID
<b64b9227-2406-440a-8cd8-95519f987b0e@linux.intel.com>, I still think
disabling this for the entire file is harmful and not necessary. I
tested the 4 lines below and they still work. "X is unused" is a useful
warning that has caught real bugs in other scripts before. For instance:
changing a variable name but not everywhere. Or just a typo. Due to the
nature of the language, such bugs can end up consuming significant
time. You do you.

# At the bottom of the script:
# shellcheck disable=SC2034
declare -a  Expect_Fail_Table XOR_Table_4R_4H XOR_Table_8R_4H XOR_Table_12R_12H
# shellcheck disable=SC2034
declare -A  Sample_4R_4H Sample_12R_12H

You would also need this one:

test_sample_sets() {
        local sample_name=$1
        # shellcheck disable=SC2034
        local -n sample_set=$1


> +check_dmesg_results() {
> +        local nr_entries=$1
> +        local expect_failures=${2:-false}  # Optional param, builtin true|false
> +        local log nr_pass nr_fail
> +
> +        log=$(journalctl --reverse --dmesg --since "$log_start_time")

In cxl-translate.sh line 297:
	local log=$(journalctl --reverse --dmesg --since "$log_start_time")
              ^-^ SC2155 (warning): Declare and assign separately to avoid masking return values.

Easily fixed with:
    local log; log=$(journalctl --reverse --dmesg --since "$log_start_time")

This matters for "set -e", see the SC2155 doc.


> +	nr_pass=$(echo "$log" | grep -c "CXL Translate Test.*PASS") || nr_pass=0
> +        nr_fail=$(echo "$log" | grep -c "CXL Translate Test.*FAIL") || nr_fail=0

Mix of tabs and spaces, here and elsewhere (I configured my editor to
show them in a different shade).

> +
> +        if ! $expect_failures; then
> +                # Expect all PASS and no FAIL
> +                [ "$nr_pass" -eq "$nr_entries" ] || err "$LINENO"
> +                [ "$nr_fail" -eq 0 ] || err "$LINENO"
> +        else
> +                # Expect no PASS and all FAIL
> +		[ "$nr_pass" -eq 0 ] || err "$LINENO"
> +                [ "$nr_fail" -eq "$nr_entries" ] || err "$LINENO"
> +        fi

Nit: I would flip the order to avoid the "double" negation (a failure is
generally considered "negative")


No other shell issue spotted.


>    [ 'cxl-qos-class.sh',       cxl_qos_class,      'cxl'   ],
>    [ 'cxl-poison.sh',          cxl_poison,         'cxl'   ],
> +  [ 'cxl-translate.sh',       cxl_translate,      'cxl'   ],
>  ]

Just FYI: this now conflicts with b26e9ae3b1dc. I got very confused
because I was missing a commit and kept looking for the correct
"pending" branch when in fact it's v3 missing a commit, not me.
Also, I forgot how clueless "git am" is. Even "patch" is better.

These lists are a regular source of conflicts when all the activity
always happens at the end. Defining some sort of order (alphabetical or
whatever) reduces the frequency of conflicts considerably.