Hi Li, thank you for posting this from your github pull request. Please find
my comments in line. Other than those comments, this change looks good to me.
I ran blktests with this change and observed same result.
On Jul 07, 2022 / 03:47, 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.
As I commented on the github pull request, Bart's idea to use array for
SKIP_REASONS looks good to me, since new helper function is not required.
>
> 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..9f83d303830d 100755
> --- a/new
> +++ b/new
> @@ -64,18 +64,18 @@ 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 set the \$SKIP_REASONS variable.
To be precise, SKIP_REASONS is not to set, but to add description. I suggest to
change the text as follows:
... 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() sets \$SKIP_REASONS, all tests in this group will be skipped.
... group_requires() adds strings to \$SKIP_REASONS, all tests in this ...
> 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
> +# group cannot be run on the test device, it should set the \$SKIP_REASONS
Same here, and same for following changes in this "new" file.
> # 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
> @@ -85,7 +85,7 @@ 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
> -# _require_test_dev_foo helpers. If group_device_requires() sets \$SKIP_REASON,
> +# _require_test_dev_foo helpers. If group_device_requires() sets \$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,17 +149,17 @@ 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
> +# function. If the test cannot be run, requires() should set the \$SKIP_REASONS
> # 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.
> +# sets \$SKIP_REASONS, 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 cannot be run on
> -# the test device, it should set \$SKIP_REASON. \$TEST_DEV is the full path of
> +# the test device, it should set \$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 the target device is a partition device,
> @@ -169,7 +169,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
> -# _require_test_dev_foo helpers. If device_requires() sets \$SKIP_REASON, the
> +# _require_test_dev_foo helpers. If device_requires() sets \$SKIP_REASONS, the
> # test will be skipped on that device.
> # device_requires() {
> # _require_test_dev_is_foo && _require_test_dev_supports_bar
> @@ -185,7 +185,7 @@ DESCRIPTION=""
> # failure. You should prefer letting the test fail because of broken output
> # over, say, checking the exit status of every command you run.
> #
> -# If the test cannot be run, this function may set the \$SKIP_REASON variable
> +# If the test cannot be run, this function may set the \$SKIP_REASONS variable
> # and return. In that case, the return value and output are ignored, and the
> # test is considered skipped. This should only be done when the requirements
> # can only be detected with a non-trivial amount of setup; use
[...]
--
Shin'ichiro Kawasaki
