"Usual" disclaimer: only sharing my shell experience. Unfortunately still cannot review the actual test logic.
Alison Schofield <[email protected]> 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 <[email protected]>, 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.
