[PATCH blktests] Fix unintended skipping of tests

[PATCH blktests] Fix unintended skipping of tests

[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 adding a _dev_is_rotational function and amend the affected
tests to use it.

Fixes: cd11d001fe86 ("Support skipping tests from test{,_device}()")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 common/rc       | 9 ++++++++-
 tests/block/005 | 2 +-
 tests/block/007 | 2 +-
 tests/block/008 | 2 +-
 tests/block/011 | 2 +-
 5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/common/rc b/common/rc
index 1893dda2b2f7..04755ca6b018 100644
--- a/common/rc
+++ b/common/rc
@@ -181,6 +181,13 @@ _have_tracepoint() {
 	return 0
 }
 
+_dev_is_rotational() {
+	if [[ $(cat "${TEST_DEV_SYSFS}/queue/rotational") -eq 0 ]]; then
+		return 1
+	fi
+	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"
@@ -190,7 +197,7 @@ _test_dev_can_discard() {
 }
 
 _test_dev_is_rotational() {
-	if [[ $(cat "${TEST_DEV_SYSFS}/queue/rotational") -eq 0 ]]; then
+	if ! _dev_is_rotational; then
 		SKIP_REASON="$TEST_DEV is not rotational"
 		return 1
 	fi
diff --git a/tests/block/005 b/tests/block/005
index 77b9e2f57203..35c21d5c368a 100755
--- a/tests/block/005
+++ b/tests/block/005
@@ -21,7 +21,7 @@ test_device() {
 	# shellcheck disable=SC2207
 	scheds=($(sed 's/[][]//g' "${TEST_DEV_SYSFS}/queue/scheduler"))
 
-	if _test_dev_is_rotational; then
+	if _dev_is_rotational; then
 		size="32m"
 	else
 		size="1g"
diff --git a/tests/block/007 b/tests/block/007
index f03935084ce6..6cffc453c1fa 100755
--- a/tests/block/007
+++ b/tests/block/007
@@ -19,7 +19,7 @@ device_requires() {
 }
 
 run_fio_job() {
-	if _test_dev_is_rotational; then
+	if _dev_is_rotational; then
 		size="32m"
 	else
 		size="1g"
diff --git a/tests/block/008 b/tests/block/008
index 3d3fcb51be2e..d63c6b0fee2b 100755
--- a/tests/block/008
+++ b/tests/block/008
@@ -17,7 +17,7 @@ requires() {
 test_device() {
 	echo "Running ${TEST_NAME}"
 
-	if _test_dev_is_rotational; then
+	if _dev_is_rotational; then
 		size="32m"
 	else
 		size="1g"
diff --git a/tests/block/011 b/tests/block/011
index c3432a63e274..50beb9b94095 100755
--- a/tests/block/011
+++ b/tests/block/011
@@ -23,7 +23,7 @@ test_device() {
 
 	pdev="$(_get_pci_dev_from_blkdev)"
 
-	if _test_dev_is_rotational; then
+	if _dev_is_rotational; then
 		size="32m"
 	else
 		size="1g"
-- 
2.26.0

Re: [PATCH blktests] Fix unintended skipping of tests

[Omar Sandoval]

On Wed, Apr 15, 2020 at 12:11:51AM +0200, 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.

Oof, I thought I checked for this sort of thing, but clearly I missed
this one. It might be better to rename the existing helpers _require_foo
(e.g., _require_test_dev_is_rotational), and have the variant without
the _require whenever it's needed. Would you mind writing a patch for
that?

Thanks,
Omar

Re: [PATCH blktests] Fix unintended skipping of tests

[Klaus Birkelund Jensen]

On Apr 16 14:44, Omar Sandoval wrote:
> On Wed, Apr 15, 2020 at 12:11:51AM +0200, 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.
> 
> Oof, I thought I checked for this sort of thing, but clearly I missed
> this one. It might be better to rename the existing helpers _require_foo
> (e.g., _require_test_dev_is_rotational), and have the variant without
> the _require whenever it's needed. Would you mind writing a patch for
> that?
> 

Sounds good to me. I'll whip that up :)


K