diff mbox series

[04/12] fstests: fix group list generation for whacky test names

Message ID 20220517070111.1381936-5-david@fromorbit.com (mailing list archive)
State New, archived
Headers show
Series fstests: fixes and more fixes... | expand

Commit Message

Dave Chinner May 17, 2022, 7:01 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

Darrick noticed that tests/xfs/191-input-validation didn't get
generated properly. Fix the regex to handle this.

$ grep -I -R "^_begin_fstest" tests/xfs | \
  sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
$
$ grep -I -R "^_begin_fstest" tests/xfs | \
  sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
191 auto quick mkfs realtime
$

Use the regexes for matching test names defined in common/test_names
rather than trying to open code it.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tools/mkgroupfile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Darrick J. Wong May 19, 2022, 6:52 p.m. UTC | #1 
On Tue, May 17, 2022 at 05:01:03PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Darrick noticed that tests/xfs/191-input-validation didn't get
> generated properly. Fix the regex to handle this.
> 
> $ grep -I -R "^_begin_fstest" tests/xfs | \
>   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> $
> $ grep -I -R "^_begin_fstest" tests/xfs | \
>   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> 191 auto quick mkfs realtime
> $
> 
> Use the regexes for matching test names defined in common/test_names
> rather than trying to open code it.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Definitely an improvement, thank you.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tools/mkgroupfile | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> index 24435898..414cb538 100755
> --- a/tools/mkgroupfile
> +++ b/tools/mkgroupfile
> @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
>  	exit 1
>  fi
>  
> +. ../../common/test_names
> +
>  cleanup()
>  {
>  	rm -f $new_groups.check
> @@ -60,7 +62,8 @@ ENDL
>  
>  	# Aggregate the groups each test belongs to for the group file
>  	grep -I -R "^_begin_fstest" $test_dir/ | \
> -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> +		sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> +		>> $new_groups
>  
>  	# Create the list of unique groups for existence checking
>  	grep -I -R "^_begin_fstest" $test_dir/ | \
> -- 
> 2.35.1
>
Zorro Lang May 20, 2022, 8:36 a.m. UTC | #2 
On Tue, May 17, 2022 at 05:01:03PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Darrick noticed that tests/xfs/191-input-validation didn't get
> generated properly. Fix the regex to handle this.
> 
> $ grep -I -R "^_begin_fstest" tests/xfs | \
>   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> $
> $ grep -I -R "^_begin_fstest" tests/xfs | \
>   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> 191 auto quick mkfs realtime
> $
> 
> Use the regexes for matching test names defined in common/test_names
> rather than trying to open code it.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Hi Dave,

After testing, looks like this patch brings in a regression issue, it causes
case number aren't sorted in group.list, then the xfstests/new program can't
get a right new case number which is unused. For example:

# make
# ./new generic
Next test id is 019
Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
# ls tests/generic/019
tests/generic/019

Then comparing the tests/generic/group.list with old generic/group.list, found
lots of difference, due to the number in new group.list isn't sorted from
small to big (I haven't gotten chance to check if there're missing number).

Thanks,
Zorro

>  tools/mkgroupfile | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> index 24435898..414cb538 100755
> --- a/tools/mkgroupfile
> +++ b/tools/mkgroupfile
> @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
>  	exit 1
>  fi
>  
> +. ../../common/test_names
> +
>  cleanup()
>  {
>  	rm -f $new_groups.check
> @@ -60,7 +62,8 @@ ENDL
>  
>  	# Aggregate the groups each test belongs to for the group file
>  	grep -I -R "^_begin_fstest" $test_dir/ | \
> -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> +		sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> +		>> $new_groups
>  
>  	# Create the list of unique groups for existence checking
>  	grep -I -R "^_begin_fstest" $test_dir/ | \
> -- 
> 2.35.1
>
Zorro Lang May 20, 2022, 8:54 a.m. UTC | #3 
On Fri, May 20, 2022 at 04:36:47PM +0800, Zorro Lang wrote:
> On Tue, May 17, 2022 at 05:01:03PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Darrick noticed that tests/xfs/191-input-validation didn't get
> > generated properly. Fix the regex to handle this.
> > 
> > $ grep -I -R "^_begin_fstest" tests/xfs | \
> >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > $
> > $ grep -I -R "^_begin_fstest" tests/xfs | \
> >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > 191 auto quick mkfs realtime
> > $
> > 
> > Use the regexes for matching test names defined in common/test_names
> > rather than trying to open code it.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> 
> Hi Dave,
> 
> After testing, looks like this patch brings in a regression issue, it causes
> case number aren't sorted in group.list, then the xfstests/new program can't
> get a right new case number which is unused. For example:

Oh, this issue isn't from this patch, it's from 441606d2 ("fstests: faster group
file creation"). Hmm... I saw your used "sort -u" in that patch, I'm going to
look into what's wrong with that.

Thanks,
Zorro

> 
> # make
> # ./new generic
> Next test id is 019
> Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
> # ls tests/generic/019
> tests/generic/019
> 
> Then comparing the tests/generic/group.list with old generic/group.list, found
> lots of difference, due to the number in new group.list isn't sorted from
> small to big (I haven't gotten chance to check if there're missing number).
> 
> Thanks,
> Zorro
> 
> >  tools/mkgroupfile | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > index 24435898..414cb538 100755
> > --- a/tools/mkgroupfile
> > +++ b/tools/mkgroupfile
> > @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
> >  	exit 1
> >  fi
> >  
> > +. ../../common/test_names
> > +
> >  cleanup()
> >  {
> >  	rm -f $new_groups.check
> > @@ -60,7 +62,8 @@ ENDL
> >  
> >  	# Aggregate the groups each test belongs to for the group file
> >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > +		sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > +		>> $new_groups
> >  
> >  	# Create the list of unique groups for existence checking
> >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > -- 
> > 2.35.1
> >
Zorro Lang May 20, 2022, 9:25 a.m. UTC | #4 
On Fri, May 20, 2022 at 04:54:45PM +0800, Zorro Lang wrote:
> On Fri, May 20, 2022 at 04:36:47PM +0800, Zorro Lang wrote:
> > On Tue, May 17, 2022 at 05:01:03PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Darrick noticed that tests/xfs/191-input-validation didn't get
> > > generated properly. Fix the regex to handle this.
> > > 
> > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > > $
> > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > > 191 auto quick mkfs realtime
> > > $
> > > 
> > > Use the regexes for matching test names defined in common/test_names
> > > rather than trying to open code it.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > 
> > Hi Dave,
> > 
> > After testing, looks like this patch brings in a regression issue, it causes
> > case number aren't sorted in group.list, then the xfstests/new program can't
> > get a right new case number which is unused. For example:
> 
> Oh, this issue isn't from this patch, it's from 441606d2 ("fstests: faster group
> file creation"). Hmm... I saw your used "sort -u" in that patch, I'm going to
> look into what's wrong with that.
> 
> Thanks,
> Zorro
> 
> > 
> > # make
> > # ./new generic
> > Next test id is 019
> > Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
> > # ls tests/generic/019
> > tests/generic/019
> > 
> > Then comparing the tests/generic/group.list with old generic/group.list, found
> > lots of difference, due to the number in new group.list isn't sorted from
> > small to big (I haven't gotten chance to check if there're missing number).
> > 
> > Thanks,
> > Zorro
> > 
> > >  tools/mkgroupfile | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > index 24435898..414cb538 100755
> > > --- a/tools/mkgroupfile
> > > +++ b/tools/mkgroupfile
> > > @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
> > >  	exit 1
> > >  fi
> > >  
> > > +. ../../common/test_names
> > > +
> > >  cleanup()
> > >  {
> > >  	rm -f $new_groups.check
> > > @@ -60,7 +62,8 @@ ENDL
> > >  
> > >  	# Aggregate the groups each test belongs to for the group file
> > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > > +		sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > > +		>> $new_groups

I think add a "sort -ug" [1] will help to fix this problem. I'm wondering
why we use ">> $new_groups" at here, why an appending write is needed. I think
there's not a 'loop running' for this code, right? If there's, please correct
me, then the 'sort -ug' have to be moved to other place.

Thanks,
Zorro

[1]
-               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
-               >> $new_groups
+               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" | \
+               sort -ug > $new_groups

I'm wondering why we use

> > >  
> > >  	# Create the list of unique groups for existence checking
> > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > -- 
> > > 2.35.1
> > >
Darrick J. Wong May 20, 2022, 4:23 p.m. UTC | #5 
On Fri, May 20, 2022 at 05:25:15PM +0800, Zorro Lang wrote:
> On Fri, May 20, 2022 at 04:54:45PM +0800, Zorro Lang wrote:
> > On Fri, May 20, 2022 at 04:36:47PM +0800, Zorro Lang wrote:
> > > On Tue, May 17, 2022 at 05:01:03PM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > Darrick noticed that tests/xfs/191-input-validation didn't get
> > > > generated properly. Fix the regex to handle this.
> > > > 
> > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > > > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > > > $
> > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > > > 191 auto quick mkfs realtime
> > > > $
> > > > 
> > > > Use the regexes for matching test names defined in common/test_names
> > > > rather than trying to open code it.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > > 
> > > Hi Dave,
> > > 
> > > After testing, looks like this patch brings in a regression issue, it causes
> > > case number aren't sorted in group.list, then the xfstests/new program can't
> > > get a right new case number which is unused. For example:
> > 
> > Oh, this issue isn't from this patch, it's from 441606d2 ("fstests: faster group
> > file creation"). Hmm... I saw your used "sort -u" in that patch, I'm going to
> > look into what's wrong with that.
> > 
> > Thanks,
> > Zorro
> > 
> > > 
> > > # make
> > > # ./new generic
> > > Next test id is 019
> > > Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
> > > # ls tests/generic/019
> > > tests/generic/019
> > > 
> > > Then comparing the tests/generic/group.list with old generic/group.list, found
> > > lots of difference, due to the number in new group.list isn't sorted from
> > > small to big (I haven't gotten chance to check if there're missing number).
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > > >  tools/mkgroupfile | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > > index 24435898..414cb538 100755
> > > > --- a/tools/mkgroupfile
> > > > +++ b/tools/mkgroupfile
> > > > @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
> > > >  	exit 1
> > > >  fi
> > > >  
> > > > +. ../../common/test_names
> > > > +
> > > >  cleanup()
> > > >  {
> > > >  	rm -f $new_groups.check
> > > > @@ -60,7 +62,8 @@ ENDL
> > > >  
> > > >  	# Aggregate the groups each test belongs to for the group file
> > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > > > +		sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > > > +		>> $new_groups
> 
> I think add a "sort -ug" [1] will help to fix this problem.

Yes please, it's much easier for us humans to index the file if it's
already sorted numerically.

> I'm wondering
> why we use ">> $new_groups" at here, why an appending write is needed. I think
> there's not a 'loop running' for this code, right? If there's, please correct
> me, then the 'sort -ug' have to be moved to other place.

The function writes a header into the group list file warning readers
that it's an autogenerated file.

--D

> Thanks,
> Zorro
> 
> [1]
> -               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> -               >> $new_groups
> +               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" | \
> +               sort -ug > $new_groups
> 
> I'm wondering why we use
> 
> > > >  
> > > >  	# Create the list of unique groups for existence checking
> > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > -- 
> > > > 2.35.1
> > > > 
>
Zorro Lang May 21, 2022, 12:27 a.m. UTC | #6 
On Fri, May 20, 2022 at 09:23:10AM -0700, Darrick J. Wong wrote:
> On Fri, May 20, 2022 at 05:25:15PM +0800, Zorro Lang wrote:
> > On Fri, May 20, 2022 at 04:54:45PM +0800, Zorro Lang wrote:
> > > On Fri, May 20, 2022 at 04:36:47PM +0800, Zorro Lang wrote:
> > > > On Tue, May 17, 2022 at 05:01:03PM +1000, Dave Chinner wrote:
> > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > 
> > > > > Darrick noticed that tests/xfs/191-input-validation didn't get
> > > > > generated properly. Fix the regex to handle this.
> > > > > 
> > > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > > >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > > > > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > > > > $
> > > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > > >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > > > > 191 auto quick mkfs realtime
> > > > > $
> > > > > 
> > > > > Use the regexes for matching test names defined in common/test_names
> > > > > rather than trying to open code it.
> > > > > 
> > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > ---
> > > > 
> > > > Hi Dave,
> > > > 
> > > > After testing, looks like this patch brings in a regression issue, it causes
> > > > case number aren't sorted in group.list, then the xfstests/new program can't
> > > > get a right new case number which is unused. For example:
> > > 
> > > Oh, this issue isn't from this patch, it's from 441606d2 ("fstests: faster group
> > > file creation"). Hmm... I saw your used "sort -u" in that patch, I'm going to
> > > look into what's wrong with that.
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > > > 
> > > > # make
> > > > # ./new generic
> > > > Next test id is 019
> > > > Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
> > > > # ls tests/generic/019
> > > > tests/generic/019
> > > > 
> > > > Then comparing the tests/generic/group.list with old generic/group.list, found
> > > > lots of difference, due to the number in new group.list isn't sorted from
> > > > small to big (I haven't gotten chance to check if there're missing number).
> > > > 
> > > > Thanks,
> > > > Zorro
> > > > 
> > > > >  tools/mkgroupfile | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > > > index 24435898..414cb538 100755
> > > > > --- a/tools/mkgroupfile
> > > > > +++ b/tools/mkgroupfile
> > > > > @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
> > > > >  	exit 1
> > > > >  fi
> > > > >  
> > > > > +. ../../common/test_names
> > > > > +
> > > > >  cleanup()
> > > > >  {
> > > > >  	rm -f $new_groups.check
> > > > > @@ -60,7 +62,8 @@ ENDL
> > > > >  
> > > > >  	# Aggregate the groups each test belongs to for the group file
> > > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > > > > +		sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > > > > +		>> $new_groups
> > 
> > I think add a "sort -ug" [1] will help to fix this problem.
> 
> Yes please, it's much easier for us humans to index the file if it's
> already sorted numerically.
> 
> > I'm wondering
> > why we use ">> $new_groups" at here, why an appending write is needed. I think
> > there's not a 'loop running' for this code, right? If there's, please correct
> > me, then the 'sort -ug' have to be moved to other place.
> 
> The function writes a header into the group list file warning readers
> that it's an autogenerated file.

Oh, you're right, there's a comment as a header, so we should change as below:

-               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
-               >> $new_groups
+               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" | \
+               sort -ug >> $new_groups

Hi Dave, I'd like to fix this issue in this week. If you'd like to fix it
in this patch today, I'll wait half day. Or as it's a separated issue, I
can write another patch to get review, then merge with this patchset together.
Until now, I've merged below patches from this patchset:

38015b3d fstests: remove xfs deprecated test
59f840bf xfs/191: remove broken test
1de1ff66 xfs/148: fix failure from bad shortform size assumptions
df84ba04 xfs/148: make test debuggable
24ac437f xfs/348: golden output is not correct
e3a4d0e8 xfs/122: add attribute log formats to test output.
e121de4a fstests: filter quota warnings

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > [1]
> > -               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > -               >> $new_groups
> > +               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" | \
> > +               sort -ug > $new_groups
> > 
> > I'm wondering why we use
> > 
> > > > >  
> > > > >  	# Create the list of unique groups for existence checking
> > > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > > -- 
> > > > > 2.35.1
> > > > > 
> > 
>
diff mbox series

Patch

diff --git a/tools/mkgroupfile b/tools/mkgroupfile
index 24435898..414cb538 100755
--- a/tools/mkgroupfile
+++ b/tools/mkgroupfile
@@ -19,6 +19,8 @@  if [ ! -x ../../check ]; then
 	exit 1
 fi
 
+. ../../common/test_names
+
 cleanup()
 {
 	rm -f $new_groups.check
@@ -60,7 +62,8 @@  ENDL
 
 	# Aggregate the groups each test belongs to for the group file
 	grep -I -R "^_begin_fstest" $test_dir/ | \
-		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
+		sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
+		>> $new_groups
 
 	# Create the list of unique groups for existence checking
 	grep -I -R "^_begin_fstest" $test_dir/ | \