diff mbox series

[blktests,01/11] check: factor out _run_test()

Message ID 20240411111228.2290407-2-shinichiro.kawasaki@wdc.com (mailing list archive)
State New, archived
Headers show
Series support test case repeat by different conditions | expand

Commit Message

Shinichiro Kawasaki April 11, 2024, 11:12 a.m. UTC
The function _run_test() is rather complex and has deep nests. Before
modifying it for repeated test case runs, simplify it. Factor out some
part of the function to the new functions _check_and_call_test() and
_check_and_call_test_device().

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 check | 90 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 53 insertions(+), 37 deletions(-)

Comments

Nitesh Shetty April 11, 2024, 1:34 p.m. UTC | #1
On 11/04/24 08:12PM, Shin'ichiro Kawasaki wrote:
>The function _run_test() is rather complex and has deep nests. Before
>modifying it for repeated test case runs, simplify it. Factor out some
>part of the function to the new functions _check_and_call_test() and
>_check_and_call_test_device().
>
>Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>---
> check | 90 +++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 53 insertions(+), 37 deletions(-)
>
>diff --git a/check b/check
>index 55871b0..b1f5212 100755
>--- a/check
>+++ b/check
>@@ -463,6 +463,56 @@ _unload_modules() {
> 	unset MODULES_TO_UNLOAD
> }
>
>+_check_and_call_test() {

ret should be declared ret as local ?

>+	if declare -fF requires >/dev/null; then
>+		requires
>+	fi
>+
>+	RESULTS_DIR="$OUTPUT/nodev"
>+	_call_test test
>+	ret=$?
>+	if (( RUN_ZONED_TESTS && CAN_BE_ZONED )); then
>+		RESULTS_DIR="$OUTPUT/nodev_zoned"
>+		RUN_FOR_ZONED=1
>+		_call_test test
>+		ret=$(( ret || $? ))
>+	fi
>+
>+	return $ret
>+}
>+
>+_check_and_call_test_device() {
>+	local unset_skip_reason

Same here, ret should declared be local ?

Reviewed-by: Nitesh Shetty <nj.shetty@samsung.com>
Shinichiro Kawasaki April 12, 2024, 10:59 a.m. UTC | #2
On Apr 11, 2024 / 19:04, Nitesh Shetty wrote:
> On 11/04/24 08:12PM, Shin'ichiro Kawasaki wrote:
> > The function _run_test() is rather complex and has deep nests. Before
> > modifying it for repeated test case runs, simplify it. Factor out some
> > part of the function to the new functions _check_and_call_test() and
> > _check_and_call_test_device().
> > 
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> > check | 90 +++++++++++++++++++++++++++++++++++------------------------
> > 1 file changed, 53 insertions(+), 37 deletions(-)
> > 
> > diff --git a/check b/check
> > index 55871b0..b1f5212 100755
> > --- a/check
> > +++ b/check
> > @@ -463,6 +463,56 @@ _unload_modules() {
> > 	unset MODULES_TO_UNLOAD
> > }
> > 
> > +_check_and_call_test() {
> 
> ret should be declared ret as local ?

Yes, will do so in v2.

> 
> > +	if declare -fF requires >/dev/null; then
> > +		requires
> > +	fi
> > +
> > +	RESULTS_DIR="$OUTPUT/nodev"
> > +	_call_test test
> > +	ret=$?
> > +	if (( RUN_ZONED_TESTS && CAN_BE_ZONED )); then
> > +		RESULTS_DIR="$OUTPUT/nodev_zoned"
> > +		RUN_FOR_ZONED=1
> > +		_call_test test
> > +		ret=$(( ret || $? ))
> > +	fi
> > +
> > +	return $ret
> > +}
> > +
> > +_check_and_call_test_device() {
> > +	local unset_skip_reason
> 
> Same here, ret should declared be local ?

Yes, and thanks for the review :)

> 
> Reviewed-by: Nitesh Shetty <nj.shetty@samsung.com>
diff mbox series

Patch

diff --git a/check b/check
index 55871b0..b1f5212 100755
--- a/check
+++ b/check
@@ -463,6 +463,56 @@  _unload_modules() {
 	unset MODULES_TO_UNLOAD
 }
 
+_check_and_call_test() {
+	if declare -fF requires >/dev/null; then
+		requires
+	fi
+
+	RESULTS_DIR="$OUTPUT/nodev"
+	_call_test test
+	ret=$?
+	if (( RUN_ZONED_TESTS && CAN_BE_ZONED )); then
+		RESULTS_DIR="$OUTPUT/nodev_zoned"
+		RUN_FOR_ZONED=1
+		_call_test test
+		ret=$(( ret || $? ))
+	fi
+
+	return $ret
+}
+
+_check_and_call_test_device() {
+	local unset_skip_reason
+
+	if declare -fF requires >/dev/null; then
+		requires
+	fi
+
+	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"]}"
+
+		unset_skip_reason=0
+		if [[ ! -v SKIP_REASONS ]]; then
+			unset_skip_reason=1
+			if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
+				SKIP_REASONS+=("${TEST_DEV} is a zoned block device")
+			elif declare -fF device_requires >/dev/null; then
+				device_requires
+			fi
+		fi
+		RESULTS_DIR="$OUTPUT/$(basename "$TEST_DEV")"
+		if ! _call_test test_device; then
+			ret=1
+		fi
+		if (( unset_skip_reason )); then
+			unset SKIP_REASONS
+		fi
+	done
+
+	return $ret
+}
+
 _run_test() {
 	TEST_NAME="$1"
 	CAN_BE_ZONED=0
@@ -482,19 +532,8 @@  _run_test() {
 	. "tests/${TEST_NAME}"
 
 	if declare -fF test >/dev/null; then
-		if declare -fF requires >/dev/null; then
-			requires
-		fi
-
-		RESULTS_DIR="$OUTPUT/nodev"
-		_call_test test
+		_check_and_call_test
 		ret=$?
-		if (( RUN_ZONED_TESTS && CAN_BE_ZONED )); then
-			RESULTS_DIR="$OUTPUT/nodev_zoned"
-			RUN_FOR_ZONED=1
-			_call_test test
-			ret=$(( ret || $? ))
-		fi
 	else
 		if [[ ${#TEST_DEVS[@]} -eq 0 ]] && \
 			declare -fF fallback_device >/dev/null; then
@@ -516,31 +555,8 @@  _run_test() {
 			return 0
 		fi
 
-		if declare -fF requires >/dev/null; then
-			requires
-		fi
-
-		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"]}"
-
-			local unset_skip_reason=0
-			if [[ ! -v SKIP_REASONS ]]; then
-				unset_skip_reason=1
-				if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
-					SKIP_REASONS+=("${TEST_DEV} is a zoned block device")
-				elif declare -fF device_requires >/dev/null; then
-					device_requires
-				fi
-			fi
-			RESULTS_DIR="$OUTPUT/$(basename "$TEST_DEV")"
-			if ! _call_test test_device; then
-				ret=1
-			fi
-			if (( unset_skip_reason )); then
-				unset SKIP_REASONS
-			fi
-		done
+		_check_and_call_test_device
+		ret=$?
 
 		if (( FALLBACK_DEVICE )); then
 			cleanup_fallback_device