On Wed, Oct 18, 2023 at 12:33:29AM +0200, Thomas Haller wrote:
> Some tests cannot pass, for example due to missing kernel features or
> kernel bugs. The tests make an educated guess (feature detection),
> whether the failure is due to that, and marks the test as SKIP (exit
> 77). The problem is, the test might guess wrong and hide a real issue.
>
> Add support for a NFT_TEST_FAIL_ON_SKIP_EXCEPT= regex to help with this.
> Together with NFT_TEST_FAIL_ON_SKIP=y is enabled, test names that match
> the regex are allowed to be skipped. Unexpected skips are treated as
> fatal. This allows to maintain a list of tests that are known to be
> skipped.
>
> You can think of this as some sort of XFAIL/XPASS mechanism. The
> difference is that usually XFAIL is part of the test. Here the failure
> happens due to external conditions, as such you need to configure
> NFT_TEST_FAIL_ON_SKIP_EXCEPT= per kernel. Also, usually XFAIL is about
> failing tests, while this is about tests that are marked to be skipped.
> But we mark them as skipped due to a heuristic, and those we want to
> manually keep track off.
>
> Why is NFT_TEST_FAIL_ON_SKIP_EXCEPT= useful and why doesn't it just work
> automatically? It does work automatically (see use case 1 below). Trust
> the automatism to the right thing, and don't bother. This is when you
> don't trust the automatism and you curate a list of tests that are known
> to be be skipped, but no others (see use case 3 below). In particular,
> it's for running CI on a downstream kernel, where we expect a static
> list of skipped tests and where we want to find any changes.
>
> Consider this: there are three use case for running the tests.
>
> 1) The contributor, who wants to run the test suite. The tests make a
> best effort to pass and when the test detects that the failure is
> likely due to the kernel, then it will skip the test. This is the
> default.
This is not a good default, it is us that are running this tests
infrastructure, we are the target users.
This contributor should be running latest kernel either from nf.git
and nf-next.git
> 2) The maintainer, who runs latest kernel and expects that all tests are
> passing. Any SKIP is likely a bug. This is achieved by setting
> NFT_TEST_FAIL_ON_SKIP=y.
I don't want to remember to set this on, I am running this in my
test machines all the time.
> 3) The downstream maintainer, who test a distro kernel that is known to
> lack certain features. They know that a selected few tests should be
> skipped, but most tests should pass. By setting NFT_TEST_FAIL_ON_SKIP=y
> and NFT_TEST_FAIL_ON_SKIP_EXCEPT= they can specify which are expected to
> be skipped. The regex is kernel/environment specific, and must be
> actively curated.
Such downstream maintainer should be really concerned about the test
failure and track the issue to make sure the fix gets to their
downstream kernel.
> BONUS) actually, cases 2) and 3) optimally run automated CI tests.
> Then the test environment is defined with a particular kernel variant
> and changes slowly over time. NFT_TEST_FAIL_ON_SKIP_EXCEPT= allows
> to pick up any unexpected changes of the skipped/pass behavior of
> tests.
>
> If a test matches the regex but passes, this is also flagged as a
> failure ([XPASS]). The user is required to maintain an accurate list of
> XFAIL tests.
>
> Example:
>
> $ NFT_TEST_FAIL_ON_SKIP=y \
> NFT_TEST_FAIL_ON_SKIP_EXCEPT='^(tests/shell/testcases/nft-f/0017ct_timeout_obj_0|tests/shell/testcases/listing/0020flowtable_0)$' \
> ./tests/shell/run-tests.sh \
> tests/shell/testcases/nft-f/0017ct_timeout_obj_0 \
> tests/shell/testcases/cache/0006_cache_table_flush \
> tests/shell/testcases/listing/0013objects_0 \
> tests/shell/testcases/listing/0020flowtable_0
> ...
> I: [SKIPPED] 1/4 tests/shell/testcases/nft-f/0017ct_timeout_obj_0
> I: [OK] 2/4 tests/shell/testcases/cache/0006_cache_table_flush
> W: [FAIL-SKIP] 3/4 tests/shell/testcases/listing/0013objects_0
> W: [XPASS] 4/4 tests/shell/testcases/listing/0020flowtable_0
>
> This list of XFAIL tests is maintainable, because on a particular
> downstream kernel, the number of tests known to be skipped is small and
> relatively static. Also, you can generate the list easily (followed by
> manual verification!) via
>
> $ NFT_TEST_FAIL_ON_SKIP=n ./tests/shell/run-tests.sh -k
> $ export NFT_TEST_FAIL_ON_SKIP_EXCEPT="$(cat /tmp/nft-test.latest.*/skip-if-fail-except)"
> $ NFT_TEST_FAIL_ON_SKIP=y ./tests/shell/run-tests.sh
>
> Signed-off-by: Thomas Haller <thaller@xxxxxxxxxx>
> ---
> Sorry for the overly long commit message. I hope it can be useful and
> speak in favor of the patch (and not against it).
>
> This is related to the "eval-exit-code" patch, as it's about how to
> handle tests that are SKIPed. But it's relevant for any skipped test,
> and not tied to that work.
>
> tests/shell/helpers/test-wrapper.sh | 41 +++++++++++++++++++++
> tests/shell/run-tests.sh | 55 ++++++++++++++++++++++-------
> 2 files changed, 83 insertions(+), 13 deletions(-)
>
> diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
> index 13b918f8b8e1..e8edb7332d24 100755
> --- a/tests/shell/helpers/test-wrapper.sh
> +++ b/tests/shell/helpers/test-wrapper.sh
> @@ -176,10 +176,49 @@ if [ "$tainted_before" != "$tainted_after" ] ; then
> rc_tainted=1
> fi
>
> +rc_fail_on_skip=0
> +rc_fail_on_skip_xpass=0
> +if [ "$NFT_TEST_FAIL_ON_SKIP" = y ] ; then
> + if [ -n "${NFT_TEST_FAIL_ON_SKIP_EXCEPT+x}" ] ; then
> +
> + OLD_IFS="$IFS"
> + IFS=$'\n'
> + REGEXES=( $NFT_TEST_FAIL_ON_SKIP_EXCEPT )
> + IFS="$OLD_IFST"
> +
> + re_match=0
> + for re in "${REGEXES[@]}" ; do
> + if [[ "$TEST" =~ $re ]] ; then
> + re_match=1
> + break;
> + fi
> + done
> +
> + if [ "$re_match" -eq 1 ] ; then
> + if [ "$rc_test" -eq 0 ] ; then
> + echo "test passed although expected to be skipped (XPASS) by regex NFT_TEST_FAIL_ON_SKIP_EXCEPT /$NFT_TEST_FAIL_ON_SKIP_EXCEPT/" > "$NFT_TEST_TESTTMPDIR/rc-failed-skip"
> + rc_fail_on_skip_xpass=1
> + fi
> + else
> + if [ "$rc_test" -eq 77 ] ; then
> + echo "upgrade skip to failure due to NFT_TEST_FAIL_ON_SKIP. Regex NFT_TEST_FAIL_ON_SKIP_EXCEPT /$NFT_TEST_FAIL_ON_SKIP_EXCEPT/ does not match" > "$NFT_TEST_TESTTMPDIR/rc-failed-skip"
> + rc_fail_on_skip=1
> + fi
> + fi
> + else
> + if [ "$rc_test" -eq 77 ] ; then
> + echo "upgrade skip to failure due to NFT_TEST_FAIL_ON_SKIP" > "$NFT_TEST_TESTTMPDIR/rc-failed-skip"
> + rc_fail_on_skip=1
> + fi
> + fi
> +fi
> +
> if [ "$rc_valgrind" -ne 0 ] ; then
> rc_exit=122
> elif [ "$rc_tainted" -ne 0 ] ; then
> rc_exit=123
> +elif [ "$rc_fail_on_skip" -ne 0 ] ; then
> + rc_exit=120
> elif [ "$rc_test" -ge 118 -a "$rc_test" -le 124 ] ; then
> # Special exit codes are reserved. Coerce them.
> rc_exit=125
> @@ -189,6 +228,8 @@ elif [ "$rc_dump" -ne 0 ] ; then
> rc_exit=124
> elif [ "$rc_chkdump" -ne 0 ] ; then
> rc_exit=121
> +elif [ "$rc_fail_on_skip_xpass" -ne 0 ] ; then
> + rc_exit=119
> else
> rc_exit=0
> fi
> diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
> index 22105c2e90e2..8fceb2795113 100755
> --- a/tests/shell/run-tests.sh
> +++ b/tests/shell/run-tests.sh
> @@ -221,6 +221,10 @@ usage() {
> echo " kernel modules)."
> echo " Parallel jobs requires unshare and are disabled with NFT_TEST_UNSHARE_CMD=\"\"."
> echo " NFT_TEST_FAIL_ON_SKIP=*|y: if any jobs are skipped, exit with error."
> + echo " NFT_TEST_FAIL_ON_SKIP_EXCEPT=<regex>: only honored with NFT_TEST_FAIL_ON_SKIP=y. This is a regex for"
> + echo " tests for which a skip is expected and not fatal (XFAIL). Multiple regex can be separated by"
> + echo " newline. This allows that skip a selected set of known tests, while all other tests must pass."
> + echo " If a test passes but matches the regex, it is treated as failure too (XPASS)."
> echo " NFT_TEST_RANDOM_SEED=<SEED>: The test runner will export the environment variable NFT_TEST_RANDOM_SEED"
> echo " set to a random number. This can be used as a stable seed for tests to randomize behavior."
> echo " Set this to a fixed value to get reproducible behavior."
> @@ -305,6 +309,8 @@ export NFT_TEST_RANDOM_SEED
>
> TESTS=()
>
> +TESTS_SKIP_EXCEPT=()
> +
> SETUP_HOST=
> SETUP_HOST_OTHER=
>
> @@ -639,6 +645,11 @@ msg_info "conf: NFT_TEST_HAS_UNSHARED_MOUNT=$(printf '%q' "$NFT_TEST_HAS_UNSHARE
> msg_info "conf: NFT_TEST_KEEP_LOGS=$(printf '%q' "$NFT_TEST_KEEP_LOGS")"
> msg_info "conf: NFT_TEST_JOBS=$NFT_TEST_JOBS"
> msg_info "conf: NFT_TEST_FAIL_ON_SKIP=$NFT_TEST_FAIL_ON_SKIP"
> +if [ -z "${NFT_TEST_FAIL_ON_SKIP_EXCEPT+x}" ] ; then
> + msg_info "conf: # NFT_TEST_FAIL_ON_SKIP_EXCEPT unset"
> +else
> + msg_info "conf: NFT_TEST_FAIL_ON_SKIP_EXCEPT=$(printf '%q' "$NFT_TEST_FAIL_ON_SKIP_EXCEPT")"
> +fi
> msg_info "conf: NFT_TEST_RANDOM_SEED=$NFT_TEST_RANDOM_SEED"
> msg_info "conf: NFT_TEST_SHUFFLE_TESTS=$NFT_TEST_SHUFFLE_TESTS"
> msg_info "conf: TMPDIR=$(printf '%q' "$_TMPDIR")"
> @@ -707,6 +718,7 @@ kernel_cleanup() {
> echo ""
> ok=0
> skipped=0
> +skipped_as_fail=0
> failed=0
>
> kmem_runs=0
> @@ -767,7 +779,7 @@ print_test_header() {
> align_text text right "${#s_idx}" "$testidx_completed"
> s_idx="$text/${#TESTS[@]}"
>
> - align_text text left 12 "[$status]"
> + align_text text left 13 "[$status]"
> _msg "$msglevel" "$text $s_idx $testfile"
> }
>
> @@ -786,10 +798,19 @@ print_test_result() {
> elif [ "$rc_got" -eq 77 ] ; then
> ((skipped++))
> result_msg_status="${YELLOW}SKIPPED$RESET"
> + TESTS_SKIP_EXCEPT+=( "^$testfile$" )
> else
> ((failed++))
> result_msg_level="W"
> - if [ "$rc_got" -eq 121 ] ; then
> + if [ "$rc_got" -eq 119 ] ; then
> + # This means, the test passed, but it was matched by NFT_TEST_FAIL_ON_SKIP_EXCEPT regex.
> + # The regex is expected to match *exactly* the tests that are skipped. An unexpected PASS
> + # is also treated as a failure. Inspect NFT_TEST_FAIL_ON_SKIP_EXCEPT regex.
> + result_msg_status="XPASS"
> + elif [ "$rc_got" -eq 120 ] ; then
> + ((skipped_as_fail++))
> + result_msg_status="FAIL-SKIP"
> + elif [ "$rc_got" -eq 121 ] ; then
> result_msg_status="CHK DUMP"
> elif [ "$rc_got" -eq 122 ] ; then
> result_msg_status="VALGRIND"
> @@ -831,8 +852,14 @@ job_start() {
> print_test_header I "$testfile" "$testidx" "EXECUTING"
> fi
>
> + # NFT_TEST_FAIL_ON_SKIP_EXCEPT is already exported, if it happens to be set.
> NFT_TEST_TESTTMPDIR="${JOBS_TEMPDIR["$testfile"]}" \
> - NFT="$NFT" NFT_REAL="$NFT_REAL" DIFF="$DIFF" DUMPGEN="$DUMPGEN" $NFT_TEST_UNSHARE_CMD "$NFT_TEST_BASEDIR/helpers/test-wrapper.sh" "$testfile"
> + NFT="$NFT" \
> + NFT_REAL="$NFT_REAL" \
> + DIFF="$DIFF" \
> + DUMPGEN="$DUMPGEN" \
> + NFT_TEST_FAIL_ON_SKIP="$NFT_TEST_FAIL_ON_SKIP" \
> + $NFT_TEST_UNSHARE_CMD "$NFT_TEST_BASEDIR/helpers/test-wrapper.sh" "$testfile"
> local rc_got=$?
>
> if [ "$NFT_TEST_JOBS" -le 1 ] ; then
> @@ -896,12 +923,7 @@ echo ""
> kmemleak_found=0
> check_kmemleak_force
>
> -failed_total="$failed"
> -if [ "$NFT_TEST_FAIL_ON_SKIP" = y ] ; then
> - failed_total="$((failed_total + skipped))"
> -fi
> -
> -if [ "$failed_total" -gt 0 ] ; then
> +if [ "$failed" -gt 0 ] ; then
> RR="$RED"
> elif [ "$skipped" -gt 0 ] ; then
> RR="$YELLOW"
> @@ -926,17 +948,24 @@ END_TIME="$(cut -d ' ' -f1 /proc/uptime)"
> WALL_TIME="$(awk -v start="$START_TIME" -v end="$END_TIME" "BEGIN { print(end - start) }")"
> printf "%s\n" "$WALL_TIME" "$START_TIME" "$END_TIME" > "$NFT_TEST_TMPDIR/times"
>
> -if [ "$failed_total" -gt 0 -o "$NFT_TEST_KEEP_LOGS" = y ] ; then
> +if [ "${#TESTS_SKIP_EXCEPT[@]}" -gt 0 ] ; then
> + printf '%s\n' "${TESTS_SKIP_EXCEPT[@]}" | sort
> +fi > "$NFT_TEST_TMPDIR/skip-if-fail-except"
> +
> +if [ "$failed" -gt 0 -o "$NFT_TEST_KEEP_LOGS" = y ] ; then
> msg_info "check the temp directory \"$NFT_TEST_TMPDIR\" (\"$NFT_TEST_LATEST\")"
> msg_info " ls -lad \"$NFT_TEST_LATEST\"/*/*"
> msg_info " grep -R ^ \"$NFT_TEST_LATEST\"/"
> + if [ "${#TESTS_SKIP_EXCEPT[@]}" -gt 0 ] ; then
> + msg_info " generate XFAIL with NFT_TEST_FAIL_ON_SKIP_EXCEPT=\"\$(cat $NFT_TEST_LATEST/skip-if-fail-except)\""
> + fi
> NFT_TEST_TMPDIR=
> fi
>
> -if [ "$failed" -gt 0 ] ; then
> +if [ "$failed" -gt 0 -a "$skipped_as_fail" -eq "$failed" ] ; then
> + msg_info "Tests were skipped, but failed due to NFT_TEST_FAIL_ON_SKIP=y"
> exit 1
> -elif [ "$NFT_TEST_FAIL_ON_SKIP" = y -a "$skipped" -gt 0 ] ; then
> - msg_info "some tests were skipped. Fail due to NFT_TEST_FAIL_ON_SKIP=y"
> +elif [ "$failed" -gt 0 ] ; then
> exit 1
> elif [ "$ok" -eq 0 -a "$skipped" -gt 0 ] ; then
> exit 77
> --
> 2.41.0
>
