Hi Zhijian, thanks for this v2 patch. I found a few minor nits in the changes
in the "new" script. Please find my comments in line. Also, the word
"Supporting" in the commit title is not so meaningful. It could be
"common, tests: Print multiple skip reasons".
Could you respin the patch one more time?
On Jul 12, 2022 / 08:21, lizhijian@xxxxxxxxxxx wrote:
> Some test cases or test groups have rather large number of test
> run requirements and then they may have multiple skip reasons. However,
> blktests can report only single skip reason. To know all of the skip
> reasons, we need to repeat skip reason resolution and blktests run.
> This is a troublesome work.
>
> In this patch, we add skip reasons to SKIP_REASONS array, then all of
> the skip reasons will be printed by iterating SKIP_REASONS at one shot
> run.
>
> Most of the works are done by following script:
>
> sed -i 's/SKIP_REASON/SKIP_REASONS/' $(git ls-files)
> git grep -h 'SKIP_REASONS=' | awk -F'SKIP_REASONS=' '{print $2}' | sort | uniq |
> while read -r r
> do
> s="SKIP_REASONS=$r"
> f=$(git grep -l "$s")
> sed -i "s@$s@SKIP_REASONS+=($r)@" $f
> done
>
> Signed-off-by: Li Zhijian <lizhijian@xxxxxxxxxxx>
[...]
> diff --git a/new b/new
> index 41f8b8d5468b..817656fac97b 100755
> --- a/new
> +++ b/new
> @@ -64,29 +64,31 @@ if [[ ! -e tests/${group} ]]; then
>
> # TODO: if this test group has any extra requirements, it should define a
> # group_requires() function. If tests in this group cannot be run,
> -# group_requires() should set the \$SKIP_REASON variable.
> +# group_requires() should add strings to the \$SKIP_REASONS array which
> +# describe why the group cannot be run.
> #
> # Usually, group_requires() just needs to check that any necessary programs and
> # kernel features are available using the _have_foo helpers. If
> -# group_requires() sets \$SKIP_REASON, all tests in this group will be skipped.
> +# group_requires() adds strings to \$SKIP_REASONS, 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 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.
> +# group cannot be run on the test device, it should adds strings to
s/adds/add/
> +# \$SKIP_REASONS. \$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
Two #s are left in the two lines above.
> +# 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
> -# _require_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() adds strings to
> +# \$SKIP_REASONS, all tests in this group will be skipped on that device.
> # group_device_requires() {
> # _require_test_dev_is_foo && _require_test_dev_supports_bar
> # }
> @@ -149,28 +151,28 @@ DESCRIPTION=""
> # CAN_BE_ZONED=1
>
> # TODO: if this test has any extra requirements, it should define a requires()
> -# 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.
> +# function. If the test cannot be run, requires() should add strings to
> +# \$SKIP_REASONS. Usually, requires() just needs to check that any necessary
> +# programs and kernel features are available using the _have_foo helpers.
Double spaces in the live above.
> +# If requires() adds strings to \$SKIP_REASONS, the test is skipped.
> # requires() {
> # _have_foo
> # }
[...]
--
Shin'ichiro Kawasaki
