* Re: [PATCH] check: run auto test group by default
2018-05-04 0:05 [PATCH] check: run auto test group by default Dave Chinner
@ 2018-05-04 3:06 ` Matthew Wilcox
2018-05-04 3:13 ` Eric Sandeen
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Matthew Wilcox @ 2018-05-04 3:06 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests
On Fri, May 04, 2018 at 10:05:09AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] check: run auto test group by default
2018-05-04 0:05 [PATCH] check: run auto test group by default Dave Chinner
2018-05-04 3:06 ` Matthew Wilcox
@ 2018-05-04 3:13 ` Eric Sandeen
2018-05-04 3:50 ` Darrick J. Wong
2018-05-04 5:55 ` Amir Goldstein
3 siblings, 0 replies; 7+ messages in thread
From: Eric Sandeen @ 2018-05-04 3:13 UTC (permalink / raw)
To: Dave Chinner, fstests; +Cc: willy
On 5/3/18 7:05 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
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.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
> check | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/check b/check
> index 84a35e6724ab..170004d4b2a2 100755
> --- a/check
> +++ b/check
> @@ -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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] check: run auto test group by default
2018-05-04 0:05 [PATCH] check: run auto test group by default Dave Chinner
2018-05-04 3:06 ` Matthew Wilcox
2018-05-04 3:13 ` Eric Sandeen
@ 2018-05-04 3:50 ` Darrick J. Wong
2018-05-04 14:58 ` Eryu Guan
2018-05-04 5:55 ` Amir Goldstein
3 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2018-05-04 3:50 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, willy
On Fri, May 04, 2018 at 10:05:09AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
> check | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/check b/check
> index 84a35e6724ab..170004d4b2a2 100755
> --- a/check
> +++ b/check
> @@ -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,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> +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
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] check: run auto test group by default
2018-05-04 3:50 ` Darrick J. Wong
@ 2018-05-04 14:58 ` Eryu Guan
0 siblings, 0 replies; 7+ messages in thread
From: Eryu Guan @ 2018-05-04 14:58 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Dave Chinner, fstests, willy
On Thu, May 03, 2018 at 08:50:39PM -0700, Darrick J. Wong wrote:
> On Fri, May 04, 2018 at 10:05:09AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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.
> >
> > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > ---
> > check | 34 +++++++++++++++++++++-------------
> > 1 file changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/check b/check
> > index 84a35e6724ab..170004d4b2a2 100755
> > --- a/check
> > +++ b/check
> > @@ -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.
>
> Looks ok otherwise,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Thanks.
Eryu
>
> --D
>
> > +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
> > --
> > 2.17.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] check: run auto test group by default
2018-05-04 0:05 [PATCH] check: run auto test group by default Dave Chinner
` (2 preceding siblings ...)
2018-05-04 3:50 ` Darrick J. Wong
@ 2018-05-04 5:55 ` Amir Goldstein
2018-05-04 7:06 ` Eryu Guan
3 siblings, 1 reply; 7+ messages in thread
From: Amir Goldstein @ 2018-05-04 5:55 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, willy
On Fri, May 4, 2018 at 3:05 AM, Dave Chinner <david@fromorbit.com> wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
It's great that we are being more welcoming to xfstests newcomers!
in that note, please update README.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] check: run auto test group by default
2018-05-04 5:55 ` Amir Goldstein
@ 2018-05-04 7:06 ` Eryu Guan
0 siblings, 0 replies; 7+ messages in thread
From: Eryu Guan @ 2018-05-04 7:06 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Dave Chinner, fstests, willy
On Fri, May 04, 2018 at 08:55:37AM +0300, Amir Goldstein wrote:
> On Fri, May 4, 2018 at 3:05 AM, Dave Chinner <david@fromorbit.com> wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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.
> >
> > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
>
> 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
>
> Thanks,
> Amir.
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread