diff mbox

fstests: Tests can use any name now, not 3 digits only.

Message ID 1427290055-32647-1-git-send-email-jtulak@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Tulak March 25, 2015, 1:27 p.m. UTC
Tests can use any name now, not 3 digits only.
(e.g. a test can be named "tests/generic/some-name")

The only limitation on a test name is no whitespace and no dot.

Signed-off-by: Jan ?ulák <jtulak@redhat.com>
---
 README |  2 +-
 check  | 11 ++++++-----
 new    | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 54 insertions(+), 9 deletions(-)

Comments

Jan Tulak March 25, 2015, 1:32 p.m. UTC | #1
Eryu,
the sorting now puts the named tests at the end of group file and dots can't be in test names any more.

I hope this version looks finally ok.
Anyway, thank you for the feedback. :-)

Cheers,
Jan

----- Original Message -----
> From: "Jan ?ulák" <jtulak@redhat.com>
> To: eguan@redhat.com
> Cc: fstests@vger.kernel.org
> Sent: Wednesday, 25 March, 2015 2:27:35 PM
> Subject: [PATCH] fstests: Tests can use any name now, not 3 digits only.
> 
> Tests can use any name now, not 3 digits only.
> (e.g. a test can be named "tests/generic/some-name")
> 
> The only limitation on a test name is no whitespace and no dot.
> 
> Signed-off-by: Jan ?ulák <jtulak@redhat.com>
> ---
>  README |  2 +-
>  check  | 11 ++++++-----
>  new    | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 54 insertions(+), 9 deletions(-)
> 
> diff --git a/README b/README
> index 0c9449a..2376674 100644
> --- a/README
> +++ b/README
> @@ -205,7 +205,7 @@ Test script environment:
>  
>  Verified output:
>  
> -    Each test script has a numerical name, e.g. 007, and an associated
> +    Each test script has a name, e.g. 007, and an associated
>      verified output, e.g. 007.out.
>  
>      It is important that the verified output is deterministic, and
> diff --git a/check b/check
> index 0830e0c..da0bc31 100755
> --- a/check
> +++ b/check
> @@ -58,7 +58,7 @@ then
>      exit 1
>  fi
>  
> -SUPPORTED_TESTS="[0-9][0-9][0-9] [0-9][0-9][0-9][0-9]"
> +SUPPORTED_TESTS="[^\s.]\+"
>  SRC_GROUPS="generic shared"
>  export SRC_DIR="tests"
>  
> @@ -96,21 +96,22 @@ get_group_list()
>  		l=$(sed -n < $SRC_DIR/$d/group \
>  			-e 's/#.*//' \
>  			-e 's/$/ /' \
> -			-e "s;\(^[0-9][0-9][0-9]\).* $grp .*;$SRC_DIR/$d/\1;p")
> +			-e "s;^\($SUPPORTED_TESTS\).* $grp .*;$SRC_DIR/$d/\1;p")
>  		grpl="$grpl $l"
>  	done
>  	echo $grpl
>  }
>  
> -# find all tests, excluding files that are test metadata such as group
> files.
> -# This assumes that tests are defined purely by alphanumeric filenames with
> no
> -# ".xyz" extensions in the name.
> +# Find all tests, excluding files that are test metadata such as group
> files.
> +# It matches test names against $SUPPORTED_TESTS defined at the top of this
> +# file.
>  get_all_tests()
>  {
>  	touch $tmp.list
>  	for d in $SRC_GROUPS $FSTYP; do
>  		ls $SRC_DIR/$d/* | \
>  			grep -v "\..*" | \
> +			grep "^$SRC_DIR/$d/$SUPPORTED_TESTS"| \
>  			grep -v "group\|Makefile" >> $tmp.list 2>/dev/null
>  	done
>  }
> diff --git a/new b/new
> index d1f8939..6cf67a7 100755
> --- a/new
> +++ b/new
> @@ -84,8 +84,11 @@ eof=1
>  for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
>  do
>      line=$((line+1))
> -    if [ -z "$found" ] || [ "$found" == "#" ];then
> -	continue
> +    if [ -z "$found" ] || [ "$found" == "#" ]; then
> +        continue
> +    elif ! echo "$found" | grep -q "^[0-9][0-9][0-9]$"; then
> +        # this one is for tests not named by a number
> +        continue
>      fi
>      i=$((i+1))
>      id=`printf "%03d" $i`
> @@ -99,9 +102,50 @@ if [ $eof -eq 1 ]; then
>     i=$((i+1))
>     id=`printf "%03d" $i`
>  fi
> +auto_id=$id
>  
>  echo "Next test is $id"
>  
> +read -p "Do you want to use ANOTHER name? y,[n]: " -r
> +if [[ "$REPLY" =~ ^[Yy]$ ]]; then
> +    # get the new name from user
> +    id=""
> +    while [ "$id" = "" ]; do
> +        read -p "Enter the new name: "
> +        if [ "$REPLY" = "" ]; then
> +            echo "Can't use empty name. For canceling, use ctrl+c."
> +        elif [ -e "$tdir/$REPLY" ]; then
> +            echo "File '$REPLY' already exists, use another one."
> +            echo #
> +        elif echo "$REPLY" | grep -q "^[^\\s.]\+$"; then
> +            id="$REPLY"
> +        else
> +            echo "Filename must not contain whitespaces and dots!"
> +            echo
> +        fi
> +    done
> +
> +    # now find where to insert this name
> +    eof=1
> +    line=0
> +    for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
> +    do
> +        line=$((line+1))
> +        if [ -z "$found" ] || [ "$found" == "#" ]; then
> +            continue
> +        elif [[ "$found" > "$id" ]]; then
> +            eof=0
> +            break
> +        fi
> +    done
> +    if [ $eof -eq 1 ]; then
> +        # If place wasn't found, let $line be the end of the file
> +        line=$((line+1))
> +    fi
> +
> +fi
> +echo "Using '$id'."
> +
>  if [ -f $tdir/$id ]
>  then
>      echo "Error: test $id already exists!"
> @@ -115,7 +159,7 @@ year=`date +%Y`
>  
>  cat <<End-of-File >$tdir/$id
>  #! /bin/bash
> -# FS QA Test No. $id
> +# FS QA Test $id
>  #
>  # what am I here for?
>  #
> --
> 2.1.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
David Sterba March 25, 2015, 2:44 p.m. UTC | #2
On Wed, Mar 25, 2015 at 02:27:35PM +0100, Jan ?ulák wrote:
> Tests can use any name now, not 3 digits only.
> (e.g. a test can be named "tests/generic/some-name")

Good idea.

> The only limitation on a test name is no whitespace and no dot.

IMHO we don't need to be too creative, the limitations make sense.

I have a proposal for slight modification to the naming scheme:

  NNN-free-text

where NNN is a unique number among all tests in the same directory.

Why? Convenience, a shortcut for the long test descriptions. We usually
say that test 123 fails and some other does not, I personally find it
very handy and would like to keep that.

I've enforced this naming scheme for btrfs-progs userspace tests:
https://github.com/kdave/btrfs-progs/tree/master/tests/fsck-tests

The preference might be different for others though, but we can still
try to follow the scheme inside the tests/btrfs/ directory.

Otherwise the patch looks ok to me.
--
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
Lukas Czerner March 25, 2015, 3:20 p.m. UTC | #3
On Wed, 25 Mar 2015, David Sterba wrote:

> Date: Wed, 25 Mar 2015 15:44:52 +0100
> From: David Sterba <dsterba@suse.cz>
> To: Jan ?ulák <jtulak@redhat.com>
> Cc: eguan@redhat.com, fstests@vger.kernel.org
> Subject: Re: [PATCH] fstests: Tests can use any name now, not 3 digits only.
> 
> On Wed, Mar 25, 2015 at 02:27:35PM +0100, Jan ?ulák wrote:
> > Tests can use any name now, not 3 digits only.
> > (e.g. a test can be named "tests/generic/some-name")
> 
> Good idea.
> 
> > The only limitation on a test name is no whitespace and no dot.
> 
> IMHO we don't need to be too creative, the limitations make sense.
> 
> I have a proposal for slight modification to the naming scheme:
> 
>   NNN-free-text
> 
> where NNN is a unique number among all tests in the same directory.
> 
> Why? Convenience, a shortcut for the long test descriptions. We usually
> say that test 123 fails and some other does not, I personally find it
> very handy and would like to keep that.

Yes, I like that, but then we want to make sure that we do not have
tests with the same numbers, but different name. Also having more more
constrains on the names is a good thing especially when people feel like
being creative with test names.

So we can make it

NNN-test-name

where we only allow numbers in the first three characters, and only
alphabetic ASCII characters and a dash afterwards (or underscore,
whichever you prefer).

Thanks!
-Lukas

> 
> I've enforced this naming scheme for btrfs-progs userspace tests:
> https://github.com/kdave/btrfs-progs/tree/master/tests/fsck-tests
> 
> The preference might be different for others though, but we can still
> try to follow the scheme inside the tests/btrfs/ directory.
> 
> Otherwise the patch looks ok to me.
> --
> 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
>
Jan Tulak March 25, 2015, 3:27 p.m. UTC | #4
----- Original Message -----
> From: "David Sterba" <dsterba@suse.cz>
> 
> I have a proposal for slight modification to the naming scheme:
> 
>   NNN-free-text
> 
> where NNN is a unique number among all tests in the same directory.
> 
> Why? Convenience, a shortcut for the long test descriptions. We usually
> say that test 123 fails and some other does not, I personally find it
> very handy and would like to keep that.
> 
> I've enforced this naming scheme for btrfs-progs userspace tests:
> https://github.com/kdave/btrfs-progs/tree/master/tests/fsck-tests
> 
> The preference might be different for others though, but we can still
> try to follow the scheme inside the tests/btrfs/ directory.
> 

I see the reason, but I have a note. This format breaks alphabetic ordering, so if we use names for grouping tests together, they are not listed that way. There is an example of what I mean by the grouping:

performance/group:
fsmark-small-files-001                fsmark small_files rw sequential
fsmark-small-files-002                fsmark small_files rw random
fsmark-small-files-003                fsmark small_files traverse
fsmark-small-files-004                fsmark small_files unlink
fsmark-large-files-001                fsmark large_files rw
fsmark-large-files-002                fsmark large_files unlink
fsmark-1m-empty-files-001        fsmark metadata scale create
fsmark-10m-empty-files-001        fsmark metadata scale create
fsmark-100m-empty-files-001        fsmark metadata scale create
fsmark-100m-empty-files-002        fsmark metadata scale traverse
fsmark-100m-empty-files-003        fsmark metadata scale unlink
.....

If we put the unique number at the end (some-name-NNN), then this issue is eliminated. Of course, with this you can't do NNN<tab> for completion, but it keeps the number reference. But this way it makes harder to find the test by number...


----- Original Message -----
> From: "Lukáš Czerner" <lczerner@redhat.com>
> Sent: Wednesday, 25 March, 2015 4:20:24 PM
> 
> 
> Yes, I like that, but then we want to make sure that we do not have
> tests with the same numbers, but different name. Also having more more
> constrains on the names is a good thing especially when people feel like
> being creative with test names.
> 
> So we can make it
> 
> NNN-test-name
> 
> where we only allow numbers in the first three characters, and only
> alphabetic ASCII characters and a dash afterwards (or underscore,
> whichever you prefer).
> 
> Thanks!
> -Lukas

The stricter rules are all right, I agree with that too.

Jan
--
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
Lukas Czerner March 25, 2015, 3:43 p.m. UTC | #5
On Wed, 25 Mar 2015, Jan Tulak wrote:

> Date: Wed, 25 Mar 2015 11:27:13 -0400 (EDT)
> From: Jan Tulak <jtulak@redhat.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: David Sterba <dsterba@suse.cz>, eguan@redhat.com, fstests@vger.kernel.org
> Subject: Re: [PATCH] fstests: Tests can use any name now, not 3 digits only.
> 
> ----- Original Message -----
> > From: "David Sterba" <dsterba@suse.cz>
> > 
> > I have a proposal for slight modification to the naming scheme:
> > 
> >   NNN-free-text
> > 
> > where NNN is a unique number among all tests in the same directory.
> > 
> > Why? Convenience, a shortcut for the long test descriptions. We usually
> > say that test 123 fails and some other does not, I personally find it
> > very handy and would like to keep that.
> > 
> > I've enforced this naming scheme for btrfs-progs userspace tests:
> > https://github.com/kdave/btrfs-progs/tree/master/tests/fsck-tests
> > 
> > The preference might be different for others though, but we can still
> > try to follow the scheme inside the tests/btrfs/ directory.
> > 
> 
> I see the reason, but I have a note. This format breaks alphabetic ordering, so if we use names for grouping tests together, they are not listed that way. There is an example of what I mean by the grouping:
> 
> performance/group:
> fsmark-small-files-001                fsmark small_files rw sequential
> fsmark-small-files-002                fsmark small_files rw random
> fsmark-small-files-003                fsmark small_files traverse
> fsmark-small-files-004                fsmark small_files unlink
> fsmark-large-files-001                fsmark large_files rw
> fsmark-large-files-002                fsmark large_files unlink
> fsmark-1m-empty-files-001        fsmark metadata scale create
> fsmark-10m-empty-files-001        fsmark metadata scale create
> fsmark-100m-empty-files-001        fsmark metadata scale create
> fsmark-100m-empty-files-002        fsmark metadata scale traverse
> fsmark-100m-empty-files-003        fsmark metadata scale unlink
> .....
> 
> If we put the unique number at the end (some-name-NNN), then this issue is eliminated. Of course, with this you can't do NNN<tab> for completion, but it keeps the number reference. But this way it makes harder to find the test by number...
> 

First of all, what's the point of the names if they are the same ?
And secondly what's the point of numbers if they repeat so often ?

This is probably only relevant to your performance patches, but can
you elaborate a bit more on how you plan to name the tests ? Because I am
not sure the example you've just shown is the best approach.

Also is there a reason for you to see it grouped by the name when
you do ls ? It's not like it'll help you run a group of the tests at
once.

-Lukas


> 
> ----- Original Message -----
> > From: "Lukáš Czerner" <lczerner@redhat.com>
> > Sent: Wednesday, 25 March, 2015 4:20:24 PM
> > 
> > 
> > Yes, I like that, but then we want to make sure that we do not have
> > tests with the same numbers, but different name. Also having more more
> > constrains on the names is a good thing especially when people feel like
> > being creative with test names.
> > 
> > So we can make it
> > 
> > NNN-test-name
> > 
> > where we only allow numbers in the first three characters, and only
> > alphabetic ASCII characters and a dash afterwards (or underscore,
> > whichever you prefer).
> > 
> > Thanks!
> > -Lukas
> 
> The stricter rules are all right, I agree with that too.
> 
> Jan
>
Jan Tulak March 26, 2015, 1:32 p.m. UTC | #6
----- Original Message -----
> From: "Lukáš Czerner" <lczerner@redhat.com>
> To: "Jan Tulak" <jtulak@redhat.com>
> Cc: "David Sterba" <dsterba@suse.cz>, eguan@redhat.com, fstests@vger.kernel.org
> Sent: Wednesday, 25 March, 2015 4:43:22 PM
> Subject: Re: [PATCH] fstests: Tests can use any name now, not 3 digits only.
> 
> First of all, what's the point of the names if they are the same ?
> And secondly what's the point of numbers if they repeat so often ?
> 
> This is probably only relevant to your performance patches, but can
> you elaborate a bit more on how you plan to name the tests ? Because I am
> not sure the example you've just shown is the best approach.
> 
> Also is there a reason for you to see it grouped by the name when
> you do ls ? It's not like it'll help you run a group of the tests at
> once.
> 
> -Lukas
> 

Rather than in "ls", I thought about seeing them grouped in the group file. But when I thought about it more, I found it pretty much useless for most of cases (as long as the test has all groups it should have). So I'm taking back my objections about it. 
You can call it a small scope creep with groups. :-)

I'm sending a patch with the unique IDs in front of the name.

Jan
--
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
diff mbox

Patch

diff --git a/README b/README
index 0c9449a..2376674 100644
--- a/README
+++ b/README
@@ -205,7 +205,7 @@  Test script environment:
 
 Verified output:
 
-    Each test script has a numerical name, e.g. 007, and an associated
+    Each test script has a name, e.g. 007, and an associated
     verified output, e.g. 007.out.
 
     It is important that the verified output is deterministic, and
diff --git a/check b/check
index 0830e0c..da0bc31 100755
--- a/check
+++ b/check
@@ -58,7 +58,7 @@  then
     exit 1
 fi
 
-SUPPORTED_TESTS="[0-9][0-9][0-9] [0-9][0-9][0-9][0-9]"
+SUPPORTED_TESTS="[^\s.]\+"
 SRC_GROUPS="generic shared"
 export SRC_DIR="tests"
 
@@ -96,21 +96,22 @@  get_group_list()
 		l=$(sed -n < $SRC_DIR/$d/group \
 			-e 's/#.*//' \
 			-e 's/$/ /' \
-			-e "s;\(^[0-9][0-9][0-9]\).* $grp .*;$SRC_DIR/$d/\1;p")
+			-e "s;^\($SUPPORTED_TESTS\).* $grp .*;$SRC_DIR/$d/\1;p")
 		grpl="$grpl $l"
 	done
 	echo $grpl
 }
 
-# find all tests, excluding files that are test metadata such as group files.
-# This assumes that tests are defined purely by alphanumeric filenames with no
-# ".xyz" extensions in the name.
+# Find all tests, excluding files that are test metadata such as group files.
+# It matches test names against $SUPPORTED_TESTS defined at the top of this
+# file.
 get_all_tests()
 {
 	touch $tmp.list
 	for d in $SRC_GROUPS $FSTYP; do
 		ls $SRC_DIR/$d/* | \
 			grep -v "\..*" | \
+			grep "^$SRC_DIR/$d/$SUPPORTED_TESTS"| \
 			grep -v "group\|Makefile" >> $tmp.list 2>/dev/null
 	done
 }
diff --git a/new b/new
index d1f8939..6cf67a7 100755
--- a/new
+++ b/new
@@ -84,8 +84,11 @@  eof=1
 for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
 do
     line=$((line+1))
-    if [ -z "$found" ] || [ "$found" == "#" ];then
-	continue
+    if [ -z "$found" ] || [ "$found" == "#" ]; then
+        continue
+    elif ! echo "$found" | grep -q "^[0-9][0-9][0-9]$"; then
+        # this one is for tests not named by a number
+        continue
     fi
     i=$((i+1))
     id=`printf "%03d" $i`
@@ -99,9 +102,50 @@  if [ $eof -eq 1 ]; then
    i=$((i+1))
    id=`printf "%03d" $i`
 fi
+auto_id=$id
 
 echo "Next test is $id"
 
+read -p "Do you want to use ANOTHER name? y,[n]: " -r
+if [[ "$REPLY" =~ ^[Yy]$ ]]; then
+    # get the new name from user
+    id=""
+    while [ "$id" = "" ]; do
+        read -p "Enter the new name: "
+        if [ "$REPLY" = "" ]; then
+            echo "Can't use empty name. For canceling, use ctrl+c."
+        elif [ -e "$tdir/$REPLY" ]; then
+            echo "File '$REPLY' already exists, use another one."
+            echo #
+        elif echo "$REPLY" | grep -q "^[^\\s.]\+$"; then
+            id="$REPLY"
+        else
+            echo "Filename must not contain whitespaces and dots!"
+            echo 
+        fi
+    done
+
+    # now find where to insert this name
+    eof=1
+    line=0
+    for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
+    do
+        line=$((line+1))
+        if [ -z "$found" ] || [ "$found" == "#" ]; then
+            continue
+        elif [[ "$found" > "$id" ]]; then
+            eof=0
+            break
+        fi
+    done
+    if [ $eof -eq 1 ]; then
+        # If place wasn't found, let $line be the end of the file
+        line=$((line+1))
+    fi
+
+fi
+echo "Using '$id'."
+
 if [ -f $tdir/$id ]
 then
     echo "Error: test $id already exists!"
@@ -115,7 +159,7 @@  year=`date +%Y`
 
 cat <<End-of-File >$tdir/$id
 #! /bin/bash
-# FS QA Test No. $id
+# FS QA Test $id
 #
 # what am I here for?
 #