* [PATCH blktests v2] Fix unintentional skipping of tests
@ 2020-04-21 7:33 Klaus Jensen
2020-04-22 1:12 ` Shinichiro Kawasaki
0 siblings, 1 reply; 6+ messages in thread
From: Klaus Jensen @ 2020-04-21 7:33 UTC (permalink / raw)
To: Omar Sandoval; +Cc: linux-block, Omar Sandoval, Klaus Jensen, Klaus Jensen
From: Klaus Jensen <k.jensen@samsung.com>
cd11d001fe86 ("Support skipping tests from test{,_device}()") breaks a
handful of tests in the block group.
For example, block/005 uses _test_dev_is_rotational to check if the
device is rotational and uses the result to size up the fio run. As a
side-effect, _test_dev_is_rotational also sets SKIP_REASON, which (since
commit cd11d001fe86) causes the test to print out a "[not run]" even
through the test actually ran successfully.
Fix this by renaming the existing helpers to _require_foo (e.g. a
_require_test_dev_is_rotational) and add the non-_require variant where
needed.
Fixes: cd11d001fe86 ("Support skipping tests from test{,_device}()")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
check | 10 ++++------
common/iopoll | 4 ++--
common/rc | 35 ++++++++++++++++++++++++++++-------
new | 12 ++++++------
tests/block/004 | 8 ++++----
tests/block/007 | 3 ++-
tests/block/011 | 2 +-
tests/block/019 | 2 +-
tests/nvme/032 | 2 +-
tests/nvme/rc | 4 ++--
tests/scsi/006 | 2 +-
tests/scsi/rc | 6 +++---
tests/zbd/007 | 2 +-
tests/zbd/rc | 11 +++++++++--
14 files changed, 65 insertions(+), 38 deletions(-)
diff --git a/check b/check
index 398eca05e3a4..84ec086c408b 100755
--- a/check
+++ b/check
@@ -423,18 +423,16 @@ _call_test() {
_test_dev_is_zoned() {
if [[ ! -f "${TEST_DEV_SYSFS}/queue/zoned" ]] ||
grep -q none "${TEST_DEV_SYSFS}/queue/zoned"; then
- SKIP_REASON="${TEST_DEV} is not a zoned block device"
return 1
fi
return 0
}
-_test_dev_is_not_zoned() {
- if _test_dev_is_zoned; then
- SKIP_REASON="${TEST_DEV} is a zoned block device"
+_require_test_dev_is_zoned() {
+ if ! _test_dev_is_zoned; then
+ SKIP_REASON="${TEST_DEV} is not a zoned block device"
return 1
fi
- unset SKIP_REASON
return 0
}
@@ -497,7 +495,7 @@ _run_test() {
local unset_skip_reason=0
if [[ ! -v SKIP_REASON ]]; then
unset_skip_reason=1
- if (( !CAN_BE_ZONED )) && ! _test_dev_is_not_zoned; then
+ if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
SKIP_REASON="${TEST_DEV} is a zoned block device"
elif declare -fF device_requires >/dev/null; then
device_requires
diff --git a/common/iopoll b/common/iopoll
index 80a5f99b08ca..dfdd2cf6f08f 100644
--- a/common/iopoll
+++ b/common/iopoll
@@ -17,7 +17,7 @@ _have_fio_with_poll() {
return 0
}
-_test_dev_supports_io_poll() {
+_require_test_dev_supports_io_poll() {
local old_io_poll
if ! old_io_poll="$(cat "${TEST_DEV_SYSFS}/queue/io_poll" 2>/dev/null)"; then
SKIP_REASON="kernel does not support polling"
@@ -30,7 +30,7 @@ _test_dev_supports_io_poll() {
return 0
}
-_test_dev_supports_io_poll_delay() {
+_require_test_dev_supports_io_poll_delay() {
local old_io_poll_delay
if ! old_io_poll_delay="$(cat "${TEST_DEV_SYSFS}/queue/io_poll_delay" 2>/dev/null)"; then
SKIP_REASON="kernel does not support hybrid polling"
diff --git a/common/rc b/common/rc
index 1893dda2b2f7..dfa7ac0e4ffc 100644
--- a/common/rc
+++ b/common/rc
@@ -181,22 +181,36 @@ _have_tracepoint() {
return 0
}
-_test_dev_can_discard() {
- if [[ $(cat "${TEST_DEV_SYSFS}/queue/discard_max_bytes") -eq 0 ]]; then
- SKIP_REASON="$TEST_DEV does not support discard"
+_test_dev_is_rotational() {
+ if [[ $(cat "${TEST_DEV_SYSFS}/queue/rotational") -eq 0 ]]; then
return 1
fi
return 0
}
-_test_dev_is_rotational() {
- if [[ $(cat "${TEST_DEV_SYSFS}/queue/rotational") -eq 0 ]]; then
+_require_test_dev_is_rotational() {
+ if ! _test_dev_is_rotational; then
SKIP_REASON="$TEST_DEV is not rotational"
return 1
fi
return 0
}
+_test_dev_can_discard() {
+ if [[ $(cat "${TEST_DEV_SYSFS}/queue/discard_max_bytes") -eq 0 ]]; then
+ return 1
+ fi
+ return 0
+}
+
+_require_test_dev_can_discard() {
+ if ! _test_dev_can_discard; then
+ SKIP_REASON="$TEST_DEV does not support discard"
+ return 1
+ fi
+ return 0
+}
+
_test_dev_queue_get() {
if [[ $1 = scheduler ]]; then
sed -e 's/.*\[//' -e 's/\].*//' "${TEST_DEV_SYSFS}/queue/scheduler"
@@ -214,7 +228,7 @@ _test_dev_queue_set() {
echo "$2" >"${TEST_DEV_SYSFS}/queue/$1"
}
-_test_dev_is_pci() {
+_require_test_dev_is_pci() {
if ! readlink -f "$TEST_DEV_SYSFS/device" | grep -q pci; then
# nvme needs some special casing
if readlink -f "$TEST_DEV_SYSFS/device" | grep -q nvme; then
@@ -247,7 +261,7 @@ _get_pci_parent_from_blkdev() {
tail -2 | head -1
}
-_test_dev_in_hotplug_slot() {
+_require_test_dev_in_hotplug_slot() {
local parent
parent="$(_get_pci_parent_from_blkdev)"
@@ -262,6 +276,13 @@ _test_dev_in_hotplug_slot() {
_test_dev_is_partition() {
if [[ -z ${TEST_DEV_PART_SYSFS} ]]; then
+ return 1
+ fi
+ return 0
+}
+
+_require_test_dev_is_partition() {
+ if ! _test_dev_is_partition; then
SKIP_REASON="${TEST_DEV} is not a partition device"
return 1
fi
diff --git a/new b/new
index 31973ed1add2..73f0faa8fa96 100755
--- a/new
+++ b/new
@@ -85,10 +85,10 @@ group_requires() {
#
# 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() sets \$SKIP_REASON, all
-# tests in this group will be skipped on that device.
+# _require_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
+# _require_test_dev_is_foo && _require_test_dev_supports_bar
# }
# TODO: define any helpers that are specific to this group.
@@ -171,10 +171,10 @@ 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() sets \$SKIP_REASON, the test will
-# be skipped on that device.
+# _require_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
+# _require_test_dev_is_foo && _require_test_dev_supports_bar
# }
# TODO: define the test. The output of this function (stdout and stderr) will
diff --git a/tests/block/004 b/tests/block/004
index d181725e6f80..92060858d4c6 100755
--- a/tests/block/004
+++ b/tests/block/004
@@ -14,10 +14,6 @@ requires() {
_have_fio
}
-device_requires() {
- ! _test_dev_is_zoned || _have_fio_zbd_zonemode
-}
-
test_device() {
echo "Running ${TEST_NAME}"
@@ -25,6 +21,10 @@ test_device() {
local zbdmode=""
if _test_dev_is_zoned; then
+ if ! _have_fio_zbd_zonemode; then
+ return
+ fi
+
_test_dev_queue_set scheduler deadline
directio="--direct=1"
zbdmode="--zonemode=zbd"
diff --git a/tests/block/007 b/tests/block/007
index f03935084ce6..b19a57024b42 100755
--- a/tests/block/007
+++ b/tests/block/007
@@ -15,7 +15,8 @@ requires() {
}
device_requires() {
- _test_dev_supports_io_poll && _test_dev_supports_io_poll_delay
+ _require_test_dev_supports_io_poll && \
+ _require_test_dev_supports_io_poll_delay
}
run_fio_job() {
diff --git a/tests/block/011 b/tests/block/011
index c3432a63e274..4f331b4a7522 100755
--- a/tests/block/011
+++ b/tests/block/011
@@ -15,7 +15,7 @@ requires() {
}
device_requires() {
- _test_dev_is_pci
+ _require_test_dev_is_pci
}
test_device() {
diff --git a/tests/block/019 b/tests/block/019
index 7cd26bd512bc..113a3d6e8986 100755
--- a/tests/block/019
+++ b/tests/block/019
@@ -14,7 +14,7 @@ requires() {
}
device_requires() {
- _test_dev_is_pci && _test_dev_in_hotplug_slot
+ _require_test_dev_is_pci && _require_test_dev_in_hotplug_slot
}
test_device() {
diff --git a/tests/nvme/032 b/tests/nvme/032
index a91a473ac5df..ce45657951a1 100755
--- a/tests/nvme/032
+++ b/tests/nvme/032
@@ -19,7 +19,7 @@ requires() {
}
device_requires() {
- _test_dev_is_nvme
+ _require_test_dev_is_nvme
}
test_device() {
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 40f0413d32d2..6ffa971b4308 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -11,12 +11,12 @@ group_requires() {
}
group_device_requires() {
- _test_dev_is_nvme
+ _require_test_dev_is_nvme
}
NVMET_CFS="/sys/kernel/config/nvmet/"
-_test_dev_is_nvme() {
+_require_test_dev_is_nvme() {
if ! readlink -f "$TEST_DEV_SYSFS/device" | grep -q nvme; then
SKIP_REASON="$TEST_DEV is not a NVMe device"
return 1
diff --git a/tests/scsi/006 b/tests/scsi/006
index f220f61e3c1e..05ed6520d600 100755
--- a/tests/scsi/006
+++ b/tests/scsi/006
@@ -12,7 +12,7 @@ DESCRIPTION="toggle SCSI cache type"
QUICK=1
device_requires() {
- _test_dev_is_scsi_disk
+ _require_test_dev_is_scsi_disk
}
test_device() {
diff --git a/tests/scsi/rc b/tests/scsi/rc
index 2a192fd0f969..1477cecc5593 100644
--- a/tests/scsi/rc
+++ b/tests/scsi/rc
@@ -11,14 +11,14 @@ group_requires() {
}
group_device_requires() {
- _test_dev_is_scsi
+ _require_test_dev_is_scsi
}
_have_scsi_generic() {
_have_modules sg
}
-_test_dev_is_scsi() {
+_require_test_dev_is_scsi() {
if [[ ! -d ${TEST_DEV_SYSFS}/device/scsi_device ]]; then
SKIP_REASON="$TEST_DEV is not a SCSI device"
return 1
@@ -26,7 +26,7 @@ _test_dev_is_scsi() {
return 0
}
-_test_dev_is_scsi_disk() {
+_require_test_dev_is_scsi_disk() {
if [[ ! -d ${TEST_DEV_SYSFS}/device/scsi_disk ]]; then
SKIP_REASON="$TEST_DEV is not a SCSI disk"
return 1
diff --git a/tests/zbd/007 b/tests/zbd/007
index b4dcbd89f179..2376b3aedaa0 100755
--- a/tests/zbd/007
+++ b/tests/zbd/007
@@ -18,7 +18,7 @@ requires() {
}
device_requires() {
- _test_dev_is_logical
+ _require_test_dev_is_logical
}
# Select test target zones. Pick up the first sequential required zones. If
diff --git a/tests/zbd/rc b/tests/zbd/rc
index 9c1dc5210b1a..a910a2425567 100644
--- a/tests/zbd/rc
+++ b/tests/zbd/rc
@@ -18,7 +18,7 @@ group_requires() {
}
group_device_requires() {
- _test_dev_is_zoned
+ _require_test_dev_is_zoned
}
_fallback_null_blk_zoned() {
@@ -254,13 +254,20 @@ _find_two_contiguous_seq_zones() {
_test_dev_is_dm() {
if [[ ! -r "${TEST_DEV_SYSFS}/dm/name" ]]; then
+ return 1
+ fi
+ return 0
+}
+
+_require_test_dev_is_dm() {
+ if ! _test_dev_is_dm; then
SKIP_REASON="$TEST_DEV is not device-mapper"
return 1
fi
return 0
}
-_test_dev_is_logical() {
+_require_test_dev_is_logical() {
if ! _test_dev_is_partition && ! _test_dev_is_dm; then
SKIP_REASON="$TEST_DEV is not a logical device"
return 1
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH blktests v2] Fix unintentional skipping of tests
2020-04-21 7:33 [PATCH blktests v2] Fix unintentional skipping of tests Klaus Jensen
@ 2020-04-22 1:12 ` Shinichiro Kawasaki
2020-04-22 5:13 ` Klaus Birkelund Jensen
0 siblings, 1 reply; 6+ messages in thread
From: Shinichiro Kawasaki @ 2020-04-22 1:12 UTC (permalink / raw)
To: Klaus Jensen; +Cc: Omar Sandoval, linux-block, Omar Sandoval, Klaus Jensen
Hello Klaus,
Thank you for this patch. I also face the unexpected "not run" messages and this
patch fixes them. I made a couple of comments on your patch in line.
On Apr 21, 2020 / 09:33, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> cd11d001fe86 ("Support skipping tests from test{,_device}()") breaks a
> handful of tests in the block group.
>
> For example, block/005 uses _test_dev_is_rotational to check if the
> device is rotational and uses the result to size up the fio run. As a
> side-effect, _test_dev_is_rotational also sets SKIP_REASON, which (since
> commit cd11d001fe86) causes the test to print out a "[not run]" even
> through the test actually ran successfully.
>
> Fix this by renaming the existing helpers to _require_foo (e.g. a
> _require_test_dev_is_rotational) and add the non-_require variant where
> needed.
>
> Fixes: cd11d001fe86 ("Support skipping tests from test{,_device}()")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> check | 10 ++++------
> common/iopoll | 4 ++--
> common/rc | 35 ++++++++++++++++++++++++++++-------
> new | 12 ++++++------
> tests/block/004 | 8 ++++----
> tests/block/007 | 3 ++-
> tests/block/011 | 2 +-
> tests/block/019 | 2 +-
> tests/nvme/032 | 2 +-
> tests/nvme/rc | 4 ++--
> tests/scsi/006 | 2 +-
> tests/scsi/rc | 6 +++---
> tests/zbd/007 | 2 +-
> tests/zbd/rc | 11 +++++++++--
> 14 files changed, 65 insertions(+), 38 deletions(-)
>
> diff --git a/check b/check
> index 398eca05e3a4..84ec086c408b 100755
> --- a/check
> +++ b/check
> @@ -423,18 +423,16 @@ _call_test() {
> _test_dev_is_zoned() {
> if [[ ! -f "${TEST_DEV_SYSFS}/queue/zoned" ]] ||
> grep -q none "${TEST_DEV_SYSFS}/queue/zoned"; then
> - SKIP_REASON="${TEST_DEV} is not a zoned block device"
> return 1
> fi
> return 0
> }
>
> -_test_dev_is_not_zoned() {
> - if _test_dev_is_zoned; then
> - SKIP_REASON="${TEST_DEV} is a zoned block device"
> +_require_test_dev_is_zoned() {
> + if ! _test_dev_is_zoned; then
> + SKIP_REASON="${TEST_DEV} is not a zoned block device"
> return 1
> fi
> - unset SKIP_REASON
> return 0
> }
>
> @@ -497,7 +495,7 @@ _run_test() {
> local unset_skip_reason=0
> if [[ ! -v SKIP_REASON ]]; then
> unset_skip_reason=1
> - if (( !CAN_BE_ZONED )) && ! _test_dev_is_not_zoned; then
> + if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
> SKIP_REASON="${TEST_DEV} is a zoned block device"
> elif declare -fF device_requires >/dev/null; then
> device_requires
> diff --git a/common/iopoll b/common/iopoll
> index 80a5f99b08ca..dfdd2cf6f08f 100644
> --- a/common/iopoll
> +++ b/common/iopoll
> @@ -17,7 +17,7 @@ _have_fio_with_poll() {
> return 0
> }
>
> -_test_dev_supports_io_poll() {
> +_require_test_dev_supports_io_poll() {
> local old_io_poll
> if ! old_io_poll="$(cat "${TEST_DEV_SYSFS}/queue/io_poll" 2>/dev/null)"; then
> SKIP_REASON="kernel does not support polling"
> @@ -30,7 +30,7 @@ _test_dev_supports_io_poll() {
> return 0
> }
>
> -_test_dev_supports_io_poll_delay() {
> +_require_test_dev_supports_io_poll_delay() {
> local old_io_poll_delay
> if ! old_io_poll_delay="$(cat "${TEST_DEV_SYSFS}/queue/io_poll_delay" 2>/dev/null)"; then
> SKIP_REASON="kernel does not support hybrid polling"
> diff --git a/common/rc b/common/rc
> index 1893dda2b2f7..dfa7ac0e4ffc 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -181,22 +181,36 @@ _have_tracepoint() {
> return 0
> }
>
> -_test_dev_can_discard() {
> - if [[ $(cat "${TEST_DEV_SYSFS}/queue/discard_max_bytes") -eq 0 ]]; then
> - SKIP_REASON="$TEST_DEV does not support discard"
> +_test_dev_is_rotational() {
> + if [[ $(cat "${TEST_DEV_SYSFS}/queue/rotational") -eq 0 ]]; then
> return 1
> fi
> return 0
> }
>
> -_test_dev_is_rotational() {
> - if [[ $(cat "${TEST_DEV_SYSFS}/queue/rotational") -eq 0 ]]; then
> +_require_test_dev_is_rotational() {
> + if ! _test_dev_is_rotational; then
> SKIP_REASON="$TEST_DEV is not rotational"
> return 1
> fi
> return 0
> }
>
> +_test_dev_can_discard() {
> + if [[ $(cat "${TEST_DEV_SYSFS}/queue/discard_max_bytes") -eq 0 ]]; then
> + return 1
> + fi
> + return 0
> +}
> +
> +_require_test_dev_can_discard() {
> + if ! _test_dev_can_discard; then
> + SKIP_REASON="$TEST_DEV does not support discard"
> + return 1
> + fi
> + return 0
> +}
> +
Don't we need replace _test_dev_can_discard() in block/003 with
_require_test_dev_can_discard()?
> _test_dev_queue_get() {
> if [[ $1 = scheduler ]]; then
> sed -e 's/.*\[//' -e 's/\].*//' "${TEST_DEV_SYSFS}/queue/scheduler"
> @@ -214,7 +228,7 @@ _test_dev_queue_set() {
> echo "$2" >"${TEST_DEV_SYSFS}/queue/$1"
> }
>
> -_test_dev_is_pci() {
> +_require_test_dev_is_pci() {
> if ! readlink -f "$TEST_DEV_SYSFS/device" | grep -q pci; then
> # nvme needs some special casing
> if readlink -f "$TEST_DEV_SYSFS/device" | grep -q nvme; then
> @@ -247,7 +261,7 @@ _get_pci_parent_from_blkdev() {
> tail -2 | head -1
> }
>
> -_test_dev_in_hotplug_slot() {
> +_require_test_dev_in_hotplug_slot() {
> local parent
> parent="$(_get_pci_parent_from_blkdev)"
>
> @@ -262,6 +276,13 @@ _test_dev_in_hotplug_slot() {
>
> _test_dev_is_partition() {
> if [[ -z ${TEST_DEV_PART_SYSFS} ]]; then
> + return 1
> + fi
> + return 0
> +}
> +
> +_require_test_dev_is_partition() {
> + if ! _test_dev_is_partition; then
> SKIP_REASON="${TEST_DEV} is not a partition device"
> return 1
> fi
> diff --git a/new b/new
> index 31973ed1add2..73f0faa8fa96 100755
> --- a/new
> +++ b/new
> @@ -85,10 +85,10 @@ group_requires() {
> #
> # 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() sets \$SKIP_REASON, all
> -# tests in this group will be skipped on that device.
> +# _require_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
> +# _require_test_dev_is_foo && _require_test_dev_supports_bar
> # }
>
> # TODO: define any helpers that are specific to this group.
> @@ -171,10 +171,10 @@ 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() sets \$SKIP_REASON, the test will
> -# be skipped on that device.
> +# _require_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
> +# _require_test_dev_is_foo && _require_test_dev_supports_bar
> # }
>
> # TODO: define the test. The output of this function (stdout and stderr) will
> diff --git a/tests/block/004 b/tests/block/004
> index d181725e6f80..92060858d4c6 100755
> --- a/tests/block/004
> +++ b/tests/block/004
> @@ -14,10 +14,6 @@ requires() {
> _have_fio
> }
>
> -device_requires() {
> - ! _test_dev_is_zoned || _have_fio_zbd_zonemode
> -}
> -
> test_device() {
> echo "Running ${TEST_NAME}"
>
> @@ -25,6 +21,10 @@ test_device() {
> local zbdmode=""
>
> if _test_dev_is_zoned; then
> + if ! _have_fio_zbd_zonemode; then
> + return
> + fi
> +
This check is equivalent to device_requires() you removed, isn't it?
If the skip check in test_device() is the last resort, it would be the
better to check in device_requires(), I think.
> _test_dev_queue_set scheduler deadline
> directio="--direct=1"
> zbdmode="--zonemode=zbd"
> diff --git a/tests/block/007 b/tests/block/007
> index f03935084ce6..b19a57024b42 100755
> --- a/tests/block/007
> +++ b/tests/block/007
> @@ -15,7 +15,8 @@ requires() {
> }
>
> device_requires() {
> - _test_dev_supports_io_poll && _test_dev_supports_io_poll_delay
> + _require_test_dev_supports_io_poll && \
> + _require_test_dev_supports_io_poll_delay
> }
>
> run_fio_job() {
> diff --git a/tests/block/011 b/tests/block/011
> index c3432a63e274..4f331b4a7522 100755
> --- a/tests/block/011
> +++ b/tests/block/011
> @@ -15,7 +15,7 @@ requires() {
> }
>
> device_requires() {
> - _test_dev_is_pci
> + _require_test_dev_is_pci
> }
>
> test_device() {
> diff --git a/tests/block/019 b/tests/block/019
> index 7cd26bd512bc..113a3d6e8986 100755
> --- a/tests/block/019
> +++ b/tests/block/019
> @@ -14,7 +14,7 @@ requires() {
> }
>
> device_requires() {
> - _test_dev_is_pci && _test_dev_in_hotplug_slot
> + _require_test_dev_is_pci && _require_test_dev_in_hotplug_slot
> }
>
> test_device() {
> diff --git a/tests/nvme/032 b/tests/nvme/032
> index a91a473ac5df..ce45657951a1 100755
> --- a/tests/nvme/032
> +++ b/tests/nvme/032
> @@ -19,7 +19,7 @@ requires() {
> }
>
> device_requires() {
> - _test_dev_is_nvme
> + _require_test_dev_is_nvme
> }
>
> test_device() {
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 40f0413d32d2..6ffa971b4308 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -11,12 +11,12 @@ group_requires() {
> }
>
> group_device_requires() {
> - _test_dev_is_nvme
> + _require_test_dev_is_nvme
> }
>
> NVMET_CFS="/sys/kernel/config/nvmet/"
>
> -_test_dev_is_nvme() {
> +_require_test_dev_is_nvme() {
> if ! readlink -f "$TEST_DEV_SYSFS/device" | grep -q nvme; then
> SKIP_REASON="$TEST_DEV is not a NVMe device"
> return 1
> diff --git a/tests/scsi/006 b/tests/scsi/006
> index f220f61e3c1e..05ed6520d600 100755
> --- a/tests/scsi/006
> +++ b/tests/scsi/006
> @@ -12,7 +12,7 @@ DESCRIPTION="toggle SCSI cache type"
> QUICK=1
>
> device_requires() {
> - _test_dev_is_scsi_disk
> + _require_test_dev_is_scsi_disk
> }
>
> test_device() {
> diff --git a/tests/scsi/rc b/tests/scsi/rc
> index 2a192fd0f969..1477cecc5593 100644
> --- a/tests/scsi/rc
> +++ b/tests/scsi/rc
> @@ -11,14 +11,14 @@ group_requires() {
> }
>
> group_device_requires() {
> - _test_dev_is_scsi
> + _require_test_dev_is_scsi
> }
>
> _have_scsi_generic() {
> _have_modules sg
> }
>
> -_test_dev_is_scsi() {
> +_require_test_dev_is_scsi() {
> if [[ ! -d ${TEST_DEV_SYSFS}/device/scsi_device ]]; then
> SKIP_REASON="$TEST_DEV is not a SCSI device"
> return 1
> @@ -26,7 +26,7 @@ _test_dev_is_scsi() {
> return 0
> }
>
> -_test_dev_is_scsi_disk() {
> +_require_test_dev_is_scsi_disk() {
> if [[ ! -d ${TEST_DEV_SYSFS}/device/scsi_disk ]]; then
> SKIP_REASON="$TEST_DEV is not a SCSI disk"
> return 1
> diff --git a/tests/zbd/007 b/tests/zbd/007
> index b4dcbd89f179..2376b3aedaa0 100755
> --- a/tests/zbd/007
> +++ b/tests/zbd/007
> @@ -18,7 +18,7 @@ requires() {
> }
>
> device_requires() {
> - _test_dev_is_logical
> + _require_test_dev_is_logical
> }
>
> # Select test target zones. Pick up the first sequential required zones. If
> diff --git a/tests/zbd/rc b/tests/zbd/rc
> index 9c1dc5210b1a..a910a2425567 100644
> --- a/tests/zbd/rc
> +++ b/tests/zbd/rc
> @@ -18,7 +18,7 @@ group_requires() {
> }
>
> group_device_requires() {
> - _test_dev_is_zoned
> + _require_test_dev_is_zoned
> }
>
> _fallback_null_blk_zoned() {
> @@ -254,13 +254,20 @@ _find_two_contiguous_seq_zones() {
>
> _test_dev_is_dm() {
> if [[ ! -r "${TEST_DEV_SYSFS}/dm/name" ]]; then
> + return 1
> + fi
> + return 0
> +}
> +
> +_require_test_dev_is_dm() {
> + if ! _test_dev_is_dm; then
> SKIP_REASON="$TEST_DEV is not device-mapper"
> return 1
> fi
> return 0
> }
>
> -_test_dev_is_logical() {
> +_require_test_dev_is_logical() {
> if ! _test_dev_is_partition && ! _test_dev_is_dm; then
> SKIP_REASON="$TEST_DEV is not a logical device"
> return 1
> --
> 2.26.2
>
--
Best Regards,
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH blktests v2] Fix unintentional skipping of tests
2020-04-22 1:12 ` Shinichiro Kawasaki
@ 2020-04-22 5:13 ` Klaus Birkelund Jensen
2020-04-22 5:29 ` Klaus Birkelund Jensen
0 siblings, 1 reply; 6+ messages in thread
From: Klaus Birkelund Jensen @ 2020-04-22 5:13 UTC (permalink / raw)
To: Shinichiro Kawasaki
Cc: Omar Sandoval, linux-block, Omar Sandoval, Klaus Jensen
On Apr 22 01:12, Shinichiro Kawasaki wrote:
> Hello Klaus,
>
Hi Shinichiro,
Thanks for taking a look.
> Thank you for this patch. I also face the unexpected "not run" messages and this
> patch fixes them. I made a couple of comments on your patch in line.
>
> On Apr 21, 2020 / 09:33, Klaus Jensen wrote:
> >
> > +_test_dev_can_discard() {
> > + if [[ $(cat "${TEST_DEV_SYSFS}/queue/discard_max_bytes") -eq 0 ]]; then
> > + return 1
> > + fi
> > + return 0
> > +}
> > +
> > +_require_test_dev_can_discard() {
> > + if ! _test_dev_can_discard; then
> > + SKIP_REASON="$TEST_DEV does not support discard"
> > + return 1
> > + fi
> > + return 0
> > +}
> > +
>
> Don't we need replace _test_dev_can_discard() in block/003 with
> _require_test_dev_can_discard()?
>
Thought I got them all... Good catch!
> >
> > -device_requires() {
> > - ! _test_dev_is_zoned || _have_fio_zbd_zonemode
> > -}
> > -
> > test_device() {
> > echo "Running ${TEST_NAME}"
> >
> > @@ -25,6 +21,10 @@ test_device() {
> > local zbdmode=""
> >
> > if _test_dev_is_zoned; then
> > + if ! _have_fio_zbd_zonemode; then
> > + return
> > + fi
> > +
>
> This check is equivalent to device_requires() you removed, isn't it?
> If the skip check in test_device() is the last resort, it would be the
> better to check in device_requires(), I think.
>
I did fix something... just not the real problem I think.
Negations doesnt really work well in device_requires. If changed to
! _require_test_dev_is_zoned || _have_fio_zbd_zonemode
then, for non-zoned devices, even though device_requires returns 0,
SKIP_REASON ends up being set to "is not a zoned block device" and skips
the test in _call_test due to this.
There are two fixes; either we add a _require_test_dev_is_not_zoned
again or put the negated check in an arithmetic context, that is
(( !_require_test_dev_is_zoned )) || _have_fio_zbd_zonemode
I think the second option is a hack, so we'd better go with the first
choice.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH blktests v2] Fix unintentional skipping of tests
2020-04-22 5:13 ` Klaus Birkelund Jensen
@ 2020-04-22 5:29 ` Klaus Birkelund Jensen
2020-04-22 6:49 ` Shinichiro Kawasaki
0 siblings, 1 reply; 6+ messages in thread
From: Klaus Birkelund Jensen @ 2020-04-22 5:29 UTC (permalink / raw)
To: Shinichiro Kawasaki
Cc: Omar Sandoval, linux-block, Omar Sandoval, Klaus Jensen
On Apr 22 07:13, Klaus Birkelund Jensen wrote:
> On Apr 22 01:12, Shinichiro Kawasaki wrote:
> > On Apr 21, 2020 / 09:33, Klaus Jensen wrote:
> > >
> > > -device_requires() {
> > > - ! _test_dev_is_zoned || _have_fio_zbd_zonemode
> > > -}
> > > -
> > > test_device() {
> > > echo "Running ${TEST_NAME}"
> > >
> > > @@ -25,6 +21,10 @@ test_device() {
> > > local zbdmode=""
> > >
> > > if _test_dev_is_zoned; then
> > > + if ! _have_fio_zbd_zonemode; then
> > > + return
> > > + fi
> > > +
> >
> > This check is equivalent to device_requires() you removed, isn't it?
> > If the skip check in test_device() is the last resort, it would be the
> > better to check in device_requires(), I think.
> >
>
> I did fix something... just not the real problem I think.
>
> Negations doesnt really work well in device_requires. If changed to
>
> ! _require_test_dev_is_zoned || _have_fio_zbd_zonemode
>
> then, for non-zoned devices, even though device_requires returns 0,
> SKIP_REASON ends up being set to "is not a zoned block device" and skips
> the test in _call_test due to this.
>
> There are two fixes; either we add a _require_test_dev_is_not_zoned
> again or put the negated check in an arithmetic context, that is
>
> (( !_require_test_dev_is_zoned )) || _have_fio_zbd_zonemode
>
> I think the second option is a hack, so we'd better go with the first
> choice.
Doh.
The _is_not_zoned version would of course just cause the test to be
skipped for zoned devices instead.
So I actually think my original fix is the best option here.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH blktests v2] Fix unintentional skipping of tests
2020-04-22 5:29 ` Klaus Birkelund Jensen
@ 2020-04-22 6:49 ` Shinichiro Kawasaki
2020-04-22 7:18 ` Klaus Birkelund Jensen
0 siblings, 1 reply; 6+ messages in thread
From: Shinichiro Kawasaki @ 2020-04-22 6:49 UTC (permalink / raw)
To: Klaus Birkelund Jensen
Cc: Omar Sandoval, linux-block, Omar Sandoval, Klaus Jensen
On Apr 22, 2020 / 07:29, Klaus Birkelund Jensen wrote:
> On Apr 22 07:13, Klaus Birkelund Jensen wrote:
> > On Apr 22 01:12, Shinichiro Kawasaki wrote:
> > > On Apr 21, 2020 / 09:33, Klaus Jensen wrote:
> > > >
> > > > -device_requires() {
> > > > - ! _test_dev_is_zoned || _have_fio_zbd_zonemode
> > > > -}
> > > > -
> > > > test_device() {
> > > > echo "Running ${TEST_NAME}"
> > > >
> > > > @@ -25,6 +21,10 @@ test_device() {
> > > > local zbdmode=""
> > > >
> > > > if _test_dev_is_zoned; then
> > > > + if ! _have_fio_zbd_zonemode; then
> > > > + return
> > > > + fi
> > > > +
> > >
> > > This check is equivalent to device_requires() you removed, isn't it?
> > > If the skip check in test_device() is the last resort, it would be the
> > > better to check in device_requires(), I think.
> > >
> >
> > I did fix something... just not the real problem I think.
> >
> > Negations doesnt really work well in device_requires. If changed to
> >
> > ! _require_test_dev_is_zoned || _have_fio_zbd_zonemode
> >
> > then, for non-zoned devices, even though device_requires returns 0,
> > SKIP_REASON ends up being set to "is not a zoned block device" and skips
> > the test in _call_test due to this.
> >
> > There are two fixes; either we add a _require_test_dev_is_not_zoned
> > again or put the negated check in an arithmetic context, that is
> >
> > (( !_require_test_dev_is_zoned )) || _have_fio_zbd_zonemode
> >
> > I think the second option is a hack, so we'd better go with the first
> > choice.
>
> Doh.
>
> The _is_not_zoned version would of course just cause the test to be
> skipped for zoned devices instead.
>
> So I actually think my original fix is the best option here.
Thank you for sharing your thoghts. I agree not to use _is_not_zoned version.
I think _test_dev_is_zoned in device_requires() does not need to be replaced
with _require_test_dev_is_zoned. It does not check requirement. It just checks
if _have_fio_zbd_zonemode check is required or not. Then, how about the code
below? (_test_dev_is_zoned does not touch SKIP_REASON.)
device_requires() {
if _test_dev_is_zoned; then
_have_fio_zbd_zonemode
fi
}
The above code is equivalent to below (less readable though).
device_requires() {
! _test_dev_is_zoned || _have_fio_zbd_zonemode
}
My suggestion is one of the two above. Your original fix also works good, then
this suggestion is not so strong.
--
Best Regards,
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH blktests v2] Fix unintentional skipping of tests
2020-04-22 6:49 ` Shinichiro Kawasaki
@ 2020-04-22 7:18 ` Klaus Birkelund Jensen
0 siblings, 0 replies; 6+ messages in thread
From: Klaus Birkelund Jensen @ 2020-04-22 7:18 UTC (permalink / raw)
To: Shinichiro Kawasaki
Cc: Omar Sandoval, linux-block, Omar Sandoval, Klaus Jensen
On Apr 22 06:49, Shinichiro Kawasaki wrote:
> On Apr 22, 2020 / 07:29, Klaus Birkelund Jensen wrote:
> > On Apr 22 07:13, Klaus Birkelund Jensen wrote:
> > > On Apr 22 01:12, Shinichiro Kawasaki wrote:
> > > > On Apr 21, 2020 / 09:33, Klaus Jensen wrote:
> > > > >
> > > > > -device_requires() {
> > > > > - ! _test_dev_is_zoned || _have_fio_zbd_zonemode
> > > > > -}
> > > > > -
> > > > > test_device() {
> > > > > echo "Running ${TEST_NAME}"
> > > > >
> > > > > @@ -25,6 +21,10 @@ test_device() {
> > > > > local zbdmode=""
> > > > >
> > > > > if _test_dev_is_zoned; then
> > > > > + if ! _have_fio_zbd_zonemode; then
> > > > > + return
> > > > > + fi
> > > > > +
> > > >
> > > > This check is equivalent to device_requires() you removed, isn't it?
> > > > If the skip check in test_device() is the last resort, it would be the
> > > > better to check in device_requires(), I think.
> > > >
> > >
> > > I did fix something... just not the real problem I think.
> > >
> > > Negations doesnt really work well in device_requires. If changed to
> > >
> > > ! _require_test_dev_is_zoned || _have_fio_zbd_zonemode
> > >
> > > then, for non-zoned devices, even though device_requires returns 0,
> > > SKIP_REASON ends up being set to "is not a zoned block device" and skips
> > > the test in _call_test due to this.
> > >
> > > There are two fixes; either we add a _require_test_dev_is_not_zoned
> > > again or put the negated check in an arithmetic context, that is
> > >
> > > (( !_require_test_dev_is_zoned )) || _have_fio_zbd_zonemode
> > >
> > > I think the second option is a hack, so we'd better go with the first
> > > choice.
> >
> > Doh.
> >
> > The _is_not_zoned version would of course just cause the test to be
> > skipped for zoned devices instead.
> >
> > So I actually think my original fix is the best option here.
>
> Thank you for sharing your thoghts. I agree not to use _is_not_zoned version.
>
> I think _test_dev_is_zoned in device_requires() does not need to be replaced
> with _require_test_dev_is_zoned. It does not check requirement. It just checks
> if _have_fio_zbd_zonemode check is required or not.
Right, that is a good observation. I'll revert my change there and keep
it as
device_requires() {
! _test_dev_is_zoned || _have_fio_zbd_zonemode
}
Thanks,
Klaus
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-22 7:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21 7:33 [PATCH blktests v2] Fix unintentional skipping of tests Klaus Jensen
2020-04-22 1:12 ` Shinichiro Kawasaki
2020-04-22 5:13 ` Klaus Birkelund Jensen
2020-04-22 5:29 ` Klaus Birkelund Jensen
2020-04-22 6:49 ` Shinichiro Kawasaki
2020-04-22 7:18 ` Klaus Birkelund Jensen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.