Skip to contentSkip to conversation

check: run auto test group by default

PATCH In review Dave Chinner 6 participants 7 messages May 4, 2018

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 check: run auto test group by default

From: Dave Chinner <[email protected]>

Everyone who starts using fstests runs "check" without parameters, and then has problems with it running dangerous tests. most people just want fstests to act as a regression test suite, not a fuzzer or exercise known crash conditions. Hence make the default behaviour to be "run the auto group" rather than "run every test".

To enable people to run all tests easily (if they really want to) add a special group keyword named "all". This wildcard will trigger selecting all the tests in fstests as per the original "check without parameters" behaviour.

---
 check | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)
check +21 -13
index 84a35e6724ab..170004d4b2a2 100755
@@ -102,6 +102,8 @@ a test file name match pattern (e.g. xfs/*).
 group argument is either a name of a tests group to collect from all
 the test dirs (e.g. quick) or a name of a tests group to collect from
 a specific tests dir in the form of <test dir>/<group name> (e.g. xfs/quick).
+If you want to run all the tests in teh test suite, use "-g all" to specify all
+groups.
 
 exclude_file argument refers to a name of a file inside each test directory.
 for every test dir where this file is found, the listed test names are
@@ -220,22 +222,24 @@ _prepare_test_list()
 	fi
 
 	# Specified groups to include
-	for group in $GROUP_LIST; do
-		list=$(get_group_list $group)
-		if [ -z "$list" ]; then
-			echo "Group \"$group\" is empty or not defined?"
-			exit 1
-		fi
+	# Note that the CLI processing adds a leading space to the first group
+	# parameter, so we have to catch that here checking for "all"
+	if ! $have_test_arg && [ "$GROUP_LIST" == " all" ]; then
+		# no test numbers, do everything
+		get_all_tests
+	else
+		for group in $GROUP_LIST; do
+			list=$(get_group_list $group)
+			if [ -z "$list" ]; then
+				echo "Group \"$group\" is empty or not defined?"
+				exit 1
+			fi
 
-		for t in $list; do
-			grep -s "^$t\$" $tmp.list >/dev/null || \
+			for t in $list; do
+				grep -s "^$t\$" $tmp.list >/dev/null || \
 							echo "$t" >>$tmp.list
+			done
 		done
-	done
-
-	if ! $have_test_arg && [ -z "$GROUP_LIST" ]; then
-		# no test numbers, do everything
-		get_all_tests
 	fi
 
 	# Specified groups to exclude
@@ -364,6 +368,10 @@ if $have_test_arg; then
 
 		shift
 	done
+elif [ -z "$GROUP_LIST" ]; then
+	# default group list is the auto group. If any other group or test is
+	# specified, we use that instead.
+	GROUP_LIST="auto"
 fi
 
 # we need common/rc
Signature · 1 line
2.17.0
#1 lore raw
Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Everyone who starts using fstests runs "check" without parameters, and then has problems with it running dangerous tests. most people just want fstests to act as a regression test suite, not a fuzzer or exercise known crash conditions. Hence make the default behaviour to be "run the auto group" rather than "run every test".

Thanks, Dave. This makes a lot of sense to me. I don't feel like I understand the shell script well enough to offer a meaningful review though.

Reviewed-by on v1 1/1
#2 lore raw
In reply to Dave Chinner
Quoted 8 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Everyone who starts using fstests runs "check" without parameters, and then has problems with it running dangerous tests. most people just want fstests to act as a regression test suite, not a fuzzer or exercise known crash conditions. Hence make the default behaviour to be "run the auto group" rather than "run every test".

Quoted 6 lines

To enable people to run all tests easily (if they really want to) add a special group keyword named "all". This wildcard will trigger selecting all the tests in fstests as per the original "check without parameters" behaviour.

I haven't tested but I assume you have ;) and this looks OK to me.

I agree that this makes things much more friendly to the new user, thanks.

Quoted 67 lines
---
 check | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)
check +21 -13
index 84a35e6724ab..170004d4b2a2 100755
@@ -102,6 +102,8 @@ a test file name match pattern (e.g. xfs/*).
 group argument is either a name of a tests group to collect from all
 the test dirs (e.g. quick) or a name of a tests group to collect from
 a specific tests dir in the form of <test dir>/<group name> (e.g. xfs/quick).
+If you want to run all the tests in teh test suite, use "-g all" to specify all
+groups.
 
 exclude_file argument refers to a name of a file inside each test directory.
 for every test dir where this file is found, the listed test names are
@@ -220,22 +222,24 @@ _prepare_test_list()
 	fi
 
 	# Specified groups to include
-	for group in $GROUP_LIST; do
-		list=$(get_group_list $group)
-		if [ -z "$list" ]; then
-			echo "Group \"$group\" is empty or not defined?"
-			exit 1
-		fi
+	# Note that the CLI processing adds a leading space to the first group
+	# parameter, so we have to catch that here checking for "all"
+	if ! $have_test_arg && [ "$GROUP_LIST" == " all" ]; then
+		# no test numbers, do everything
+		get_all_tests
+	else
+		for group in $GROUP_LIST; do
+			list=$(get_group_list $group)
+			if [ -z "$list" ]; then
+				echo "Group \"$group\" is empty or not defined?"
+				exit 1
+			fi
 
-		for t in $list; do
-			grep -s "^$t\$" $tmp.list >/dev/null || \
+			for t in $list; do
+				grep -s "^$t\$" $tmp.list >/dev/null || \
 							echo "$t" >>$tmp.list
+			done
 		done
-	done
-
-	if ! $have_test_arg && [ -z "$GROUP_LIST" ]; then
-		# no test numbers, do everything
-		get_all_tests
 	fi
 
 	# Specified groups to exclude
@@ -364,6 +368,10 @@ if $have_test_arg; then
 
 		shift
 	done
+elif [ -z "$GROUP_LIST" ]; then
+	# default group list is the auto group. If any other group or test is
+	# specified, we use that instead.
+	GROUP_LIST="auto"
 fi
 
 # we need common/rc
Reviewed-by on v1 1/1
#3 lore raw
In reply to Dave Chinner
Quoted 23 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Everyone who starts using fstests runs "check" without parameters, and then has problems with it running dangerous tests. most people just want fstests to act as a regression test suite, not a fuzzer or exercise known crash conditions. Hence make the default behaviour to be "run the auto group" rather than "run every test".

To enable people to run all tests easily (if they really want to) add a special group keyword named "all". This wildcard will trigger selecting all the tests in fstests as per the original "check without parameters" behaviour.

---
 check | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)
check +0 -0
index 84a35e6724ab..170004d4b2a2 100755

check a test file name match pattern (e.g. xfs/*). · line 105

Quoted 4 lines
check +1 -0
@@ -102,6 +102,8 @@ a test file name match pattern (e.g. xfs/*).
 group argument is either a name of a tests group to collect from all
 the test dirs (e.g. quick) or a name of a tests group to collect from
 a specific tests dir in the form of <test dir>/<group name> (e.g. xfs/quick).
+If you want to run all the tests in teh test suite, use "-g all" to specify all

...the test suite... (unless you really mean the rock band :P)

Looks ok otherwise,

--D

Quoted 60 lines
check +20 -13
+groups.
 
 exclude_file argument refers to a name of a file inside each test directory.
 for every test dir where this file is found, the listed test names are
@@ -220,22 +222,24 @@ _prepare_test_list()
 	fi
 
 	# Specified groups to include
-	for group in $GROUP_LIST; do
-		list=$(get_group_list $group)
-		if [ -z "$list" ]; then
-			echo "Group \"$group\" is empty or not defined?"
-			exit 1
-		fi
+	# Note that the CLI processing adds a leading space to the first group
+	# parameter, so we have to catch that here checking for "all"
+	if ! $have_test_arg && [ "$GROUP_LIST" == " all" ]; then
+		# no test numbers, do everything
+		get_all_tests
+	else
+		for group in $GROUP_LIST; do
+			list=$(get_group_list $group)
+			if [ -z "$list" ]; then
+				echo "Group \"$group\" is empty or not defined?"
+				exit 1
+			fi
 
-		for t in $list; do
-			grep -s "^$t\$" $tmp.list >/dev/null || \
+			for t in $list; do
+				grep -s "^$t\$" $tmp.list >/dev/null || \
 							echo "$t" >>$tmp.list
+			done
 		done
-	done
-
-	if ! $have_test_arg && [ -z "$GROUP_LIST" ]; then
-		# no test numbers, do everything
-		get_all_tests
 	fi
 
 	# Specified groups to exclude
@@ -364,6 +368,10 @@ if $have_test_arg; then
 
 		shift
 	done
+elif [ -z "$GROUP_LIST" ]; then
+	# default group list is the auto group. If any other group or test is
+	# specified, we use that instead.
+	GROUP_LIST="auto"
 fi
 
 # we need common/rc
Signature · 6 lines
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#4 lore raw
Quoted 19 lines · Darrick J. Wong
Quoted 18 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Everyone who starts using fstests runs "check" without parameters, and then has problems with it running dangerous tests. most people just want fstests to act as a regression test suite, not a fuzzer or exercise known crash conditions. Hence make the default behaviour to be "run the auto group" rather than "run every test".

To enable people to run all tests easily (if they really want to) add a special group keyword named "all". This wildcard will trigger selecting all the tests in fstests as per the original "check without parameters" behaviour.

---
 check | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)
Quoted 11 lines

check a test file name match pattern (e.g. xfs/*). · line 105

Quoted 9 lines
check +1 -0
index 84a35e6724ab..170004d4b2a2 100755
@@ -102,6 +102,8 @@ a test file name match pattern (e.g. xfs/*).
 group argument is either a name of a tests group to collect from all
 the test dirs (e.g. quick) or a name of a tests group to collect from
 a specific tests dir in the form of <test dir>/<group name> (e.g. xfs/quick).
+If you want to run all the tests in teh test suite, use "-g all" to specify all

...the test suite... (unless you really mean the rock band :P)

Fixed on commit.

Quoted 3 lines

Looks ok otherwise,

Thanks.

Eryu

Quoted 67 lines

--D

Quoted 60 lines
check +20 -13
+groups.
 
 exclude_file argument refers to a name of a file inside each test directory.
 for every test dir where this file is found, the listed test names are
@@ -220,22 +222,24 @@ _prepare_test_list()
 	fi
 
 	# Specified groups to include
-	for group in $GROUP_LIST; do
-		list=$(get_group_list $group)
-		if [ -z "$list" ]; then
-			echo "Group \"$group\" is empty or not defined?"
-			exit 1
-		fi
+	# Note that the CLI processing adds a leading space to the first group
+	# parameter, so we have to catch that here checking for "all"
+	if ! $have_test_arg && [ "$GROUP_LIST" == " all" ]; then
+		# no test numbers, do everything
+		get_all_tests
+	else
+		for group in $GROUP_LIST; do
+			list=$(get_group_list $group)
+			if [ -z "$list" ]; then
+				echo "Group \"$group\" is empty or not defined?"
+				exit 1
+			fi
 
-		for t in $list; do
-			grep -s "^$t\$" $tmp.list >/dev/null || \
+			for t in $list; do
+				grep -s "^$t\$" $tmp.list >/dev/null || \
 							echo "$t" >>$tmp.list
+			done
 		done
-	done
-
-	if ! $have_test_arg && [ -z "$GROUP_LIST" ]; then
-		# no test numbers, do everything
-		get_all_tests
 	fi
 
 	# Specified groups to exclude
@@ -364,6 +368,10 @@ if $have_test_arg; then
 
 		shift
 	done
+elif [ -z "$GROUP_LIST" ]; then
+	# default group list is the auto group. If any other group or test is
+	# specified, we use that instead.
+	GROUP_LIST="auto"
 fi
 
 # we need common/rc
Signature · 6 lines
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
In reply to Dave Chinner
Quoted 8 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Everyone who starts using fstests runs "check" without parameters, and then has problems with it running dangerous tests. most people just want fstests to act as a regression test suite, not a fuzzer or exercise known crash conditions. Hence make the default behaviour to be "run the auto group" rather than "run every test".

Quoted 6 lines

To enable people to run all tests easily (if they really want to) add a special group keyword named "all". This wildcard will trigger selecting all the tests in fstests as per the original "check without parameters" behaviour.

It's great that we are being more welcoming to xfstests newcomers! in that note, please update README.

Thanks,
Amir.

#6 lore raw
Quoted 14 lines · Amir Goldstein
Quoted 13 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Everyone who starts using fstests runs "check" without parameters, and then has problems with it running dangerous tests. most people just want fstests to act as a regression test suite, not a fuzzer or exercise known crash conditions. Hence make the default behaviour to be "run the auto group" rather than "run every test".

To enable people to run all tests easily (if they really want to) add a special group keyword named "all". This wildcard will trigger selecting all the tests in fstests as per the original "check without parameters" behaviour.

Quoted 4 lines
Quoted 1 line

It's great that we are being more welcoming to xfstests newcomers! in that note, please update README.

Yeah, a line in README to describe this would be great.

Thanks,
Eryu

Quoted 7 lines

Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html