From 4824ac3f5c4a845d373c85ee37e00a0d6f109608 Mon Sep 17 00:00:00 2001 From: Omar Sandoval Date: Wed, 4 Mar 2020 16:27:38 -0800 Subject: [PATCH] Skip tests based on SKIP_REASON, not return value Currently, {,group_,device_}requires are required to both set SKIP_REASON and return non-zero. This is somewhat redundant, and will be inconsistent with skipping from test{,_device} that is about to be implemented. Let's ignore the return value and always skip the test if SKIP_REASON is set. The _have_foo and _test_dev_foo helpers should still return so that they can be chained together. --- check | 55 ++++++++++++++++++++++++++++++++-------------- common/rc | 2 +- new | 46 ++++++++++++++++++-------------------- tests/meta/007 | 1 - tests/meta/008 | 1 - tests/meta/rc | 4 ---- tests/nvmeof-mp/rc | 18 +++++++-------- tests/srp/015 | 7 +++--- tests/srp/rc | 20 ++++++++--------- tests/zbd/rc | 8 +++---- 10 files changed, 86 insertions(+), 76 deletions(-) diff --git a/check b/check index 5153dcd..5aeedd2 100755 --- a/check +++ b/check @@ -423,6 +423,15 @@ _test_dev_is_zoned() { return 0 } +_test_dev_is_not_zoned() { + if _test_dev_is_zoned; then + SKIP_REASON="${TEST_DEV} is a zoned block device" + return 1 + fi + unset SKIP_REASON + return 0 +} + _run_test() { TEST_NAME="$1" CAN_BE_ZONED=0 @@ -435,9 +444,12 @@ _run_test() { . "tests/${TEST_NAME}" if declare -fF test >/dev/null; then - if declare -fF requires >/dev/null && ! requires; then - _output_notrun "$TEST_NAME" - return 0 + if declare -fF requires >/dev/null; then + requires + if [[ -v SKIP_REASON ]]; then + _output_notrun "$TEST_NAME" + return 0 + fi fi RESULTS_DIR="$OUTPUT/nodev" @@ -471,24 +483,30 @@ _run_test() { return 0 fi - if declare -fF requires >/dev/null && ! requires; then - _output_notrun "$TEST_NAME" - return 0 + if declare -fF requires >/dev/null; then + requires + if [[ -v SKIP_REASON ]]; then + _output_notrun "$TEST_NAME" + return 0 + fi fi local ret=0 for TEST_DEV in "${TEST_DEVS[@]}"; do TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}" TEST_DEV_PART_SYSFS="${TEST_DEV_PART_SYSFS_DIRS["$TEST_DEV"]}" - if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then - SKIP_REASON="${TEST_DEV} is a zoned block device" + if (( !CAN_BE_ZONED )) && ! _test_dev_is_not_zoned; then _output_notrun "$TEST_NAME => $(basename "$TEST_DEV")" + unset SKIP_REASON continue fi - unset SKIP_REASON - if declare -fF device_requires >/dev/null && ! device_requires; then - _output_notrun "$TEST_NAME => $(basename "$TEST_DEV")" - continue + if declare -fF device_requires >/dev/null; then + device_requires + if [[ -v SKIP_REASON ]]; then + _output_notrun "$TEST_NAME => $(basename "$TEST_DEV")" + unset SKIP_REASON + continue + fi fi RESULTS_DIR="$OUTPUT/$(basename "$TEST_DEV")" if ! _call_test test_device; then @@ -514,9 +532,12 @@ _run_group() { # shellcheck disable=SC1090 . "tests/${group}/rc" - if declare -fF group_requires >/dev/null && ! group_requires; then - _output_notrun "${group}/***" - return 0 + if declare -fF group_requires >/dev/null; then + group_requires + if [[ -v SKIP_REASON ]]; then + _output_notrun "${group}/***" + return 0 + fi fi if declare -fF group_device_requires >/dev/null; then @@ -526,9 +547,11 @@ _run_group() { TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}" # shellcheck disable=SC2034 TEST_DEV_PART_SYSFS="${TEST_DEV_PART_SYSFS_DIRS["$TEST_DEV"]}" - if ! group_device_requires; then + group_device_requires + if [[ -v SKIP_REASON ]]; then _output_notrun "${group}/*** => $(basename "$TEST_DEV")" unset TEST_DEVS["$i"] + unset SKIP_REASON fi done # Fix the array indices. diff --git a/common/rc b/common/rc index 63b29be..1893dda 100644 --- a/common/rc +++ b/common/rc @@ -139,7 +139,7 @@ _have_kernel_option() { for f in /proc/config.gz /boot/config-$(uname -r); do [ -e "$f" ] || continue if zgrep -q "^CONFIG_${opt}=[my]$" "$f"; then - SKIP_REASON="" + unset SKIP_REASON return 0 else SKIP_REASON="kernel option $opt has not been enabled" diff --git a/new b/new index 5f6e1c0..9a2d255 100755 --- a/new +++ b/new @@ -63,32 +63,30 @@ if [[ ! -e tests/${group} ]]; then # . common/foo # TODO: if this test group has any extra requirements, it should define a -# group_requires() function. If tests in this group can be run, -# group_requires() should return 0. Otherwise, it should return non-zero and -# set the \$SKIP_REASON variable. +# group_requires() function. If tests in this group cannot be run, +# group_requires() should set the \$SKIP_REASON variable. # # Usually, group_requires() just needs to check that any necessary programs and # kernel features are available using the _have_foo helpers. If -# group_requires() returns non-zero, all tests in this group will be skipped. +# group_requires() sets \$SKIP_REASON, all tests in this group will be skipped. group_requires() { _have_root } # TODO: if this test group has extra requirements for what devices it can be # run on, it should define a group_device_requires() function. If tests in this -# group can be run on the test device, it should return zero. Otherwise, it -# should return non-zero and set the \$SKIP_REASON variable. \$TEST_DEV is the -# full path of the block device (e.g., /dev/nvme0n1 or /dev/sda1), and -# \$TEST_DEV_SYSFS is the sysfs path of the disk (not the partition, e.g., -# /sys/block/nvme0n1 or /sys/block/sda). If the target device is a partition -# device, \$TEST_DEV_PART_SYSFS is the sysfs path of the partition device -# (e.g., /sys/block/nvme0n1/nvme0n1p1 or /sys/block/sda/sda1). Otherwise, -# \$TEST_DEV_PART_SYSFS is an empty string. +# group cannot be run on the test device, it should set the \$SKIP_REASON +# variable. \$TEST_DEV is the full path of the block device (e.g., /dev/nvme0n1 +# or /dev/sda1), and \$TEST_DEV_SYSFS is the sysfs path of the disk (not the +# partition, e.g., /sys/block/nvme0n1 or /sys/block/sda). If the target device +# is a partition device, \$TEST_DEV_PART_SYSFS is the sysfs path of the +# partition device (e.g., /sys/block/nvme0n1/nvme0n1p1 or /sys/block/sda/sda1). +# Otherwise, \$TEST_DEV_PART_SYSFS is an empty string. # # Usually, group_device_requires() just needs to check that the test device is # the right type of hardware or supports any necessary features using the -# _test_dev_foo helpers. If group_device_requires() returns non-zero, all tests -# in this group will be skipped on that device. +# _test_dev_foo helpers. If group_device_requires() sets \$SKIP_REASON, all +# tests in this group will be skipped on that device. # group_device_requires() { # _test_dev_is_foo && _test_dev_supports_bar # } @@ -153,21 +151,19 @@ DESCRIPTION="" # CAN_BE_ZONED=1 # TODO: if this test has any extra requirements, it should define a requires() -# function. If the test can be run, requires() should return 0. Otherwise, it -# should return non-zero and set the \$SKIP_REASON variable. Usually, -# requires() just needs to check that any necessary programs and kernel -# features are available using the _have_foo helpers. If requires() returns -# non-zero, the test is skipped. +# function. If the test cannot be run, requires() should set the \$SKIP_REASON +# variable. Usually, requires() just needs to check that any necessary programs +# and kernel features are available using the _have_foo helpers. If requires() +# sets \$SKIP_REASON, the test is skipped. # requires() { # _have_foo # } # TODO: if this test has extra requirements for what devices it can be run on, -# it should define a device_requires() function. If this test can be run on the -# test device, it should return zero. Otherwise, it should return non-zero and -# set the \$SKIP_REASON variable. \$TEST_DEV is the full path of the block -# device (e.g., /dev/nvme0n1 or /dev/sda1), and \$TEST_DEV_SYSFS is the sysfs -# path of the disk (not the partition, e.g., /sys/block/nvme0n1 or +# it should define a device_requires() function. If this test cannot be run on +# the test device, it should set \$SKIP_REASON. \$TEST_DEV is the full path of +# the block device (e.g., /dev/nvme0n1 or /dev/sda1), and \$TEST_DEV_SYSFS is +# the sysfs path of the disk (not the partition, e.g., /sys/block/nvme0n1 or # /sys/block/sda). If the target device is a partition device, # \$TEST_DEV_PART_SYSFS is the sysfs path of the partition device (e.g., # /sys/block/nvme0n1/nvme0n1p1 or /sys/block/sda/sda1). Otherwise, @@ -175,7 +171,7 @@ DESCRIPTION="" # # Usually, device_requires() just needs to check that the test device is the # right type of hardware or supports any necessary features using the -# _test_dev_foo helpers. If device_requires() returns non-zero, the test will +# _test_dev_foo helpers. If device_requires() sets \$SKIP_REASON, the test will # be skipped on that device. # device_requires() { # _test_dev_is_foo && _test_dev_supports_bar diff --git a/tests/meta/007 b/tests/meta/007 index 7978190..ae57a67 100755 --- a/tests/meta/007 +++ b/tests/meta/007 @@ -10,7 +10,6 @@ DESCRIPTION="skip in requires()" requires() { SKIP_REASON="(╯°□°)╯︵ ┻━┻" - return 1 } test() { diff --git a/tests/meta/008 b/tests/meta/008 index 0305b56..6d7b032 100755 --- a/tests/meta/008 +++ b/tests/meta/008 @@ -10,7 +10,6 @@ DESCRIPTION="skip in device_requires()" device_requires() { SKIP_REASON="(╯°□°)╯︵ $TEST_DEV ┻━┻" - return 1 } test_device() { diff --git a/tests/meta/rc b/tests/meta/rc index 093edd1..121c295 100644 --- a/tests/meta/rc +++ b/tests/meta/rc @@ -9,17 +9,13 @@ group_requires() { if [[ "${META_REQUIRES_SKIP:-}" ]]; then SKIP_REASON="META_REQUIRES_SKIP was set" - return 1 fi - return 0 } group_device_requires() { if [[ "${META_DEVICE_REQUIRES_SKIP:-}" ]]; then SKIP_REASON="META_DEVICE_REQUIRES_SKIP was set" - return 1 fi - return 0 } _have_writeable_kmsg() { diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc index 278843a..1fd6314 100755 --- a/tests/nvmeof-mp/rc +++ b/tests/nvmeof-mp/rc @@ -19,10 +19,10 @@ group_requires() { # option. if _have_kernel_option NVME_MULTIPATH; then SKIP_REASON="CONFIG_NVME_MULTIPATH has been set in .config" - return 1 + return fi - _have_configfs || return $? + _have_configfs || return required_modules=( dm_multipath dm_queue_length @@ -38,30 +38,30 @@ group_requires() { scsi_dh_emc scsi_dh_rdac ) - _have_modules "${required_modules[@]}" || return 1 + _have_modules "${required_modules[@]}" || return for p in mkfs.ext4 mkfs.xfs multipath multipathd pidof; do - _have_program "$p" || return $? + _have_program "$p" || return done - _multipathd_version_ge 0.7.0 || return $? + _multipathd_version_ge 0.7.0 || return - _have_root || return $? + _have_root || return - _have_kernel_option DM_UEVENT || return $? + _have_kernel_option DM_UEVENT || return # shellcheck disable=SC2043 for name in multipathd; do if pidof "$name" >/dev/null; then SKIP_REASON="$name must be stopped before the nvmeof-mp tests are run" - return 1 + return fi done if [ -e /etc/multipath.conf ] && ! diff -q /etc/multipath.conf tests/nvmeof-mp/multipath.conf >&/dev/null then SKIP_REASON="/etc/multipath.conf already exists" - return 1 + return fi } diff --git a/tests/srp/015 b/tests/srp/015 index e9769dc..e03b204 100755 --- a/tests/srp/015 +++ b/tests/srp/015 @@ -8,10 +8,9 @@ DESCRIPTION="File I/O on top of multipath concurrently with logout and login (mq TIMED=1 requires() { - _have_modules siw - _have_kver 5 5 - # See also commit 4336c5821a7b ("rdma: add 'link add/delete' commands"). - _have_iproute2 190404 + # See also iproute commit 4336c5821a7b ("rdma: add 'link add/delete' + # commands"). + _have_modules siw && _have_kver 5 5 && _have_iproute2 190404 } test_disconnect_repeatedly() { diff --git a/tests/srp/rc b/tests/srp/rc index 2738c8f..07fa5e3 100755 --- a/tests/srp/rc +++ b/tests/srp/rc @@ -29,10 +29,10 @@ is_lio_configured() { group_requires() { local m name p required_modules - _have_configfs || return $? + _have_configfs || return if is_lio_configured; then SKIP_REASON="LIO must be unloaded before the SRP tests are run" - return 1 + return fi required_modules=( dm_multipath @@ -56,31 +56,31 @@ group_requires() { target_core_iblock target_core_mod ) - _have_modules "${required_modules[@]}" || return 1 + _have_modules "${required_modules[@]}" || return for p in mkfs.ext4 mkfs.xfs multipath multipathd pidof sg_reset; do - _have_program "$p" || return $? + _have_program "$p" || return done - _multipathd_version_ge 0.7.0 || return $? + _multipathd_version_ge 0.7.0 || return - _have_root || return $? + _have_root || return - _have_src_program discontiguous-io || return $? + _have_src_program discontiguous-io || return - _have_kernel_option DM_UEVENT || return $? + _have_kernel_option DM_UEVENT || return for name in srp_daemon multipathd; do if pidof "$name" >/dev/null; then SKIP_REASON="$name must be stopped before the SRP tests are run" - return 1 + return fi done if [ -e /etc/multipath.conf ] && ! diff -q /etc/multipath.conf tests/srp/multipath.conf >&/dev/null then SKIP_REASON="/etc/multipath.conf already exists" - return 1 + return fi } diff --git a/tests/zbd/rc b/tests/zbd/rc index 5dd2779..9c1dc52 100644 --- a/tests/zbd/rc +++ b/tests/zbd/rc @@ -12,11 +12,9 @@ # group_requires() { - _have_root || return $? - _have_program blkzone || return $? - _have_program dd || return $? - _have_kernel_option BLK_DEV_ZONED || return $? - _have_modules null_blk && _have_module_param null_blk zoned + _have_root && _have_program blkzone && _have_program dd && + _have_kernel_option BLK_DEV_ZONED && _have_modules null_blk && + _have_module_param null_blk zoned } group_device_requires() { -- 2.51.0