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

Marc Herbert
2025-11-05T02:42:47Z

"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.