diff mbox

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

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

Commit Message

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

Names are limited to alphanumeric characters and dash and are always prefixed
with an unique id for easier identification of a specific test.

Signed-off-by: Jan ?ulák <jtulak@redhat.com>
---
 Just a small fix in comment to reflect $SUPPORTED_TESTS is in common/rc.

 README    |  7 ++++++-
 check     |  9 ++++-----
 common/rc |  1 +
 new       | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 59 insertions(+), 11 deletions(-)

Comments

Eryu Guan March 27, 2015, 2:33 p.m. UTC | #1
On Fri, Mar 27, 2015 at 12:49:26PM +0100, Jan ?ulák wrote:
> Tests can use any name now, not 3 digits only.
> (e.g. a test can be named "tests/generic/001-some-name")
> 
> Names are limited to alphanumeric characters and dash and are always prefixed
> with an unique id for easier identification of a specific test.
> 
> Signed-off-by: Jan ?ulák <jtulak@redhat.com>

Reviewed-by: Eryu Guan <eguan@redhat.com>

Thanks!

> ---
>  Just a small fix in comment to reflect $SUPPORTED_TESTS is in common/rc.
> 
>  README    |  7 ++++++-
>  check     |  9 ++++-----
>  common/rc |  1 +
>  new       | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/README b/README
> index 0c9449a..1841052 100644
> --- a/README
> +++ b/README
> @@ -202,10 +202,15 @@ Test script environment:
>  	- When calling getfacl in a test, pass the "-n" argument so
>  	  that numeric rather than symbolic identifiers are used in
>  	  the output.
> +	- When creating a new test, it is possible to enter a custom name
> +	  for the file. Filenames are in form NNN-custom-name, where NNN
> +	  is automatically added by the ./new script as an unique ID,
> +	  and "custom-name" is the optional string entered into a prompt
> +	  in the ./new script. Note the "NNN-" part is added automatically.
>  
>  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..df9a5be 100755
> --- a/check
> +++ b/check
> @@ -58,7 +58,6 @@ then
>      exit 1
>  fi
>  
> -SUPPORTED_TESTS="[0-9][0-9][0-9] [0-9][0-9][0-9][0-9]"
>  SRC_GROUPS="generic shared"
>  export SRC_DIR="tests"
>  
> @@ -96,21 +95,21 @@ 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 in common/rc
>  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/common/rc b/common/rc
> index 857308a..cc9a894 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -21,6 +21,7 @@
>  #-----------------------------------------------------------------------
>  
>  BC=$(which bc 2> /dev/null) || BC=
> +SUPPORTED_TESTS="[a-zA-Z0-9-]\+"
>  
>  _require_math()
>  {
> diff --git a/new b/new
> index d1f8939..7e9ce2c 100755
> --- a/new
> +++ b/new
> @@ -81,11 +81,14 @@ line=0
>  eof=1
>  [ -f "$tdir/group" ] || usage
>  
> -for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
> +for found in `cat $tdir/group | tr - ' ' | $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`
> @@ -100,7 +103,47 @@ if [ $eof -eq 1 ]; then
>     id=`printf "%03d" $i`
>  fi
>  
> -echo "Next test is $id"
> +echo "Next test id is $id"
> +
> +read -p "Append a name to the ID? Test name will be $id-\$name. y,[n]: " -r
> +if [[ $REPLY = [Yy] ]]; then
> +	# get the new name from user
> +	name=""
> +	while [ "$name" = "" ]; do
> +		read -p "Enter the name: "
> +		if [ "$REPLY" = "" ]; then
> +			echo "For canceling, use ctrl+c."
> +		elif [ -e "$tdir/$id-$REPLY" ]; then
> +			echo "File '$id-$REPLY' already exists, use another one."
> +			echo #
> +		elif echo "$REPLY" | grep -q "^$SUPPORTED_TESTS$"; then
> +			name="$REPLY"
> +		else
> +			echo "Filename must contain only alphanumeric symbols and dash!"
> +			echo "(Used regex: ^$SUPPORTED_TESTS$)"
> +			echo
> +		fi
> +	done
> +
> +	# now find where to insert this name
> +	eof=1
> +	for found in `tail -n +$line $tdir/group | $AWK_PROG '{ print $1 }'`; do
> +		found_id=$(echo "$found" | tr - ' ' | $AWK_PROG '{ print $1 }')
> +		line=$((line+1))
> +		if [ -z "$found" ] || [ "$found" == "#" ]; then
> +			continue
> +		elif [[ $found > $name ]] || [ $found_id -gt $id ]; then
> +			eof=0
> +			break
> +		fi
> +	done
> +	if [ $eof -eq 0 ]; then
> +		# If place wasn't found, let $line be the end of the file
> +		line=$((line-1))
> +	fi
> +	id="$id-$name"
> +fi
> +echo "Creating test file '$id'"
>  
>  if [ -f $tdir/$id ]
>  then
> @@ -115,7 +158,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
David Sterba March 30, 2015, 1:44 p.m. UTC | #2
On Fri, Mar 27, 2015 at 12:49:26PM +0100, Jan ?ulák wrote:
> Tests can use any name now, not 3 digits only.
> (e.g. a test can be named "tests/generic/001-some-name")
> 
> Names are limited to alphanumeric characters and dash and are always prefixed
> with an unique id for easier identification of a specific test.
> 
> Signed-off-by: Jan ?ulák <jtulak@redhat.com>

Reviewed-by: David Sterba <dsterba@suse.cz>
--
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
Dave Chinner April 1, 2015, 4:35 a.m. UTC | #3
On Fri, Mar 27, 2015 at 12:49:26PM +0100, Jan ?ulák wrote:
> Tests can use any name now, not 3 digits only.
> (e.g. a test can be named "tests/generic/001-some-name")
> 
> Names are limited to alphanumeric characters and dash and are always prefixed
> with an unique id for easier identification of a specific test.
> 
> Signed-off-by: Jan ?ulák <jtulak@redhat.com>
> ---
>  Just a small fix in comment to reflect $SUPPORTED_TESTS is in common/rc.

Which I have a couple of nits about, seeing we've settled on an
acceptible convention. :)

> 
>  README    |  7 ++++++-
>  check     |  9 ++++-----
>  common/rc |  1 +
>  new       | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/README b/README
> index 0c9449a..1841052 100644
> --- a/README
> +++ b/README
> @@ -202,10 +202,15 @@ Test script environment:
>  	- When calling getfacl in a test, pass the "-n" argument so
>  	  that numeric rather than symbolic identifiers are used in
>  	  the output.
> +	- When creating a new test, it is possible to enter a custom name
> +	  for the file. Filenames are in form NNN-custom-name, where NNN
> +	  is automatically added by the ./new script as an unique ID,
> +	  and "custom-name" is the optional string entered into a prompt
> +	  in the ./new script. Note the "NNN-" part is added automatically.

We have a defined format for the test names, yet it does not have
the clear explanation of what constitutes a valid name i.e. from an
error message below:

> +	echo "Filename must contain only alphanumeric symbols and dash!"

If we have a defined format for the test names, we should enforce
it strictly and code directly to it. The description here needs to
spell this out directly.

> @@ -96,21 +95,21 @@ 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")

Here we filter by $SUPPORTED_TESTS for valid tests, but....

> --- a/common/rc
> +++ b/common/rc
> @@ -21,6 +21,7 @@
>  #-----------------------------------------------------------------------
>  
>  BC=$(which bc 2> /dev/null) || BC=
> +SUPPORTED_TESTS="[a-zA-Z0-9-]\+"

but $SUPPORTED_TESTS doesn't define the correct format for the test
names. It defines valid characters, but does not enforce the fact
that the first 3 characters must match [0-9] (i.e. the NNN bit), and
then the rest is optional but must be alphanumeric symbols and dash.
i.e it should be matching against:

# Valid test names start with 3 digits "NNN":
#  "[0-9]\{3\}"
# followed by an optional "-":
#  "-\?"
# followed by an optional combination of alphanumeric and "-" chars:
#  "[[:alnum:]-]*"
# e.g. 999-the-mark-of-fstests
#
VALID_TEST_NAME="[0-9]\{3\}-\?[[:alnum:]-]*"

>  _require_math()
>  {
> diff --git a/new b/new
> index d1f8939..7e9ce2c 100755
> --- a/new
> +++ b/new
> @@ -81,11 +81,14 @@ line=0
>  eof=1
>  [ -f "$tdir/group" ] || usage
>  
> -for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
> +for found in `cat $tdir/group | tr - ' ' | $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

This should check for "$VALID_TEST_NAME$", too, as it will match
[0-9]{3}$ test names just fine.

> +        continue
>      fi
>      i=$((i+1))
>      id=`printf "%03d" $i`
> @@ -100,7 +103,47 @@ if [ $eof -eq 1 ]; then
>     id=`printf "%03d" $i`
>  fi
>  
> -echo "Next test is $id"
> +echo "Next test id is $id"
> +
> +read -p "Append a name to the ID? Test name will be $id-\$name. y,[n]: " -r
> +if [[ $REPLY = [Yy] ]]; then
> +	# get the new name from user
> +	name=""
> +	while [ "$name" = "" ]; do
> +		read -p "Enter the name: "
> +		if [ "$REPLY" = "" ]; then
> +			echo "For canceling, use ctrl+c."
> +		elif [ -e "$tdir/$id-$REPLY" ]; then
> +			echo "File '$id-$REPLY' already exists, use another one."
> +			echo #

This check should happen *after* input validation, though it should
never be true because we have selected a unique, unused $id....

> +		elif echo "$REPLY" | grep -q "^$SUPPORTED_TESTS$"; then
> +			name="$REPLY"

i.e. this should be first, and given that we have a well defined
input validation, we should make that as clear as possible. e.g.

		elif echo $REPLY | grep -q "^[[:alnum:]-]$"; then

> +		else
> +			echo "Filename must contain only alphanumeric symbols and dash!"
> +			echo "(Used regex: ^$SUPPORTED_TESTS$)"

Don't confuse the poor users with a regex - make it clear what the
format is in the documentation.

> +			echo
> +		fi
> +	done
> +
> +	# now find where to insert this name
> +	eof=1
> +	for found in `tail -n +$line $tdir/group | $AWK_PROG '{ print $1 }'`; do
> +		found_id=$(echo "$found" | tr - ' ' | $AWK_PROG '{ print $1 }')

That's an interesting way of cutting up a string.

		found_id=$(echo "$found" | cut -d "-" -f 1 -)

> +		line=$((line+1))
> +		if [ -z "$found" ] || [ "$found" == "#" ]; then
> +			continue
> +		elif [[ $found > $name ]] || [ $found_id -gt $id ]; then

This doesn't work. $found will be "298-I-want-a-spoon" or just
"298", but $name has no number prefix i.e. "but-I-like-sporks". We
will always have number prefixes, and tests should always have unique
numbers, so there is no reason to check anything but $id against
$found_id.  Which means:

	for found_id in `tail -n +$line $tdir/group | cut -d "-" -f 1" -`; do
		line=$((line+1))
		if [ -z "$found" ] || [ "$found" == "#" ]; then
			continue
		elif [ $found_id -gt $id ]; then
			eof=0
			break
		fi
	done

And once we've done all this and decided on a test name, we should
finally validate it again against $VALID_TEST_NAME....

Cheers,

Dave.
Jan Tulak April 1, 2015, 12:09 p.m. UTC | #4
----- Original Message -----
> From: "Dave Chinner" <david@fromorbit.com>
> To: "Jan ?ulák" <jtulak@redhat.com>
> Cc: fstests@vger.kernel.org, eguan@redhat.com
> Sent: Wednesday, 1 April, 2015 6:35:42 AM
> Subject: Re: [PATCH v9] fstests: Tests can use any name now, not 3 digits only.
> 
> On Fri, Mar 27, 2015 at 12:49:26PM +0100, Jan ?ulák wrote:
> > Tests can use any name now, not 3 digits only.
> > (e.g. a test can be named "tests/generic/001-some-name")
> > 
> > Names are limited to alphanumeric characters and dash and are always
> > prefixed
> > with an unique id for easier identification of a specific test.
> > 
> > Signed-off-by: Jan ?ulák <jtulak@redhat.com>
> > ---
> >  Just a small fix in comment to reflect $SUPPORTED_TESTS is in common/rc.
> 
> Which I have a couple of nits about, seeing we've settled on an
> acceptible convention. :)
> 
> > 
> >  README    |  7 ++++++-
> >  check     |  9 ++++-----
> >  common/rc |  1 +
> >  new       | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  4 files changed, 59 insertions(+), 11 deletions(-)
> > 
> > diff --git a/README b/README
> > index 0c9449a..1841052 100644
> > --- a/README
> > +++ b/README
> > @@ -202,10 +202,15 @@ Test script environment:
> >  	- When calling getfacl in a test, pass the "-n" argument so
> >  	  that numeric rather than symbolic identifiers are used in
> >  	  the output.
> > +	- When creating a new test, it is possible to enter a custom name
> > +	  for the file. Filenames are in form NNN-custom-name, where NNN
> > +	  is automatically added by the ./new script as an unique ID,
> > +	  and "custom-name" is the optional string entered into a prompt
> > +	  in the ./new script. Note the "NNN-" part is added automatically.
> 
> We have a defined format for the test names, yet it does not have
> the clear explanation of what constitutes a valid name i.e. from an
> error message below:
> 
> > +	echo "Filename must contain only alphanumeric symbols and dash!"

Thanks, I'm adding a note into the readme.

> 
> If we have a defined format for the test names, we should enforce
> it strictly and code directly to it. The description here needs to
> spell this out directly.
> 
> > @@ -96,21 +95,21 @@ 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")
> 
> Here we filter by $SUPPORTED_TESTS for valid tests, but....
> 
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -21,6 +21,7 @@
> >  #-----------------------------------------------------------------------
> >  
> >  BC=$(which bc 2> /dev/null) || BC=
> > +SUPPORTED_TESTS="[a-zA-Z0-9-]\+"
> 
> but $SUPPORTED_TESTS doesn't define the correct format for the test
> names. It defines valid characters, but does not enforce the fact
> that the first 3 characters must match [0-9] (i.e. the NNN bit), and
> then the rest is optional but must be alphanumeric symbols and dash.
> i.e it should be matching against:
> 

I did it to be "generous on input, strict on output" - the ./new script always put 3 numbers at the beginning, but if someone for some reason needs to avoid it, it will still work. Enforcing it is easy, but is it needed?


> # Valid test names start with 3 digits "NNN":
> #  "[0-9]\{3\}"
> # followed by an optional "-":
> #  "-\?"
> # followed by an optional combination of alphanumeric and "-" chars:
> #  "[[:alnum:]-]*"
> # e.g. 999-the-mark-of-fstests
> #
> VALID_TEST_NAME="[0-9]\{3\}-\?[[:alnum:]-]*"
> 
> >  _require_math()
> >  {
> > diff --git a/new b/new
> > index d1f8939..7e9ce2c 100755
> > --- a/new
> > +++ b/new
> > @@ -81,11 +81,14 @@ line=0
> >  eof=1
> >  [ -f "$tdir/group" ] || usage
> >  
> > -for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
> > +for found in `cat $tdir/group | tr - ' ' | $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
> 
> This should check for "$VALID_TEST_NAME$", too, as it will match
> [0-9]{3}$ test names just fine.
> 
> > +        continue
> >      fi
> >      i=$((i+1))
> >      id=`printf "%03d" $i`
> > @@ -100,7 +103,47 @@ if [ $eof -eq 1 ]; then
> >     id=`printf "%03d" $i`
> >  fi
> >  
> > -echo "Next test is $id"
> > +echo "Next test id is $id"
> > +
> > +read -p "Append a name to the ID? Test name will be $id-\$name. y,[n]: "
> > -r
> > +if [[ $REPLY = [Yy] ]]; then
> > +	# get the new name from user
> > +	name=""
> > +	while [ "$name" = "" ]; do
> > +		read -p "Enter the name: "
> > +		if [ "$REPLY" = "" ]; then
> > +			echo "For canceling, use ctrl+c."
> > +		elif [ -e "$tdir/$id-$REPLY" ]; then
> > +			echo "File '$id-$REPLY' already exists, use another one."
> > +			echo #
> 
> This check should happen *after* input validation, though it should
> never be true because we have selected a unique, unused $id....

This check can be true - ID check is done in a group file, it does not reflects existence of files with such ID and name. So if you for example interrupt the ./new script after creating the file, but before editing groups, the file can exist without an entry in groups and the ID is used again.

I agree with validating the input first.

> 
> > +		elif echo "$REPLY" | grep -q "^$SUPPORTED_TESTS$"; then
> > +			name="$REPLY"
> 
> i.e. this should be first, and given that we have a well defined
> input validation, we should make that as clear as possible. e.g.
> 
> 		elif echo $REPLY | grep -q "^[[:alnum:]-]$"; then
> 

You mean to put the regex directly into the condition? I prefer to keep in the variable defined in common/rc. 

> > +		else
> > +			echo "Filename must contain only alphanumeric symbols and dash!"
> > +			echo "(Used regex: ^$SUPPORTED_TESTS$)"
> 
> Don't confuse the poor users with a regex - make it clear what the
> format is in the documentation.
>

Well, I think any user who will write tests for a filesystem knows what regex is. ;-)
But I take the point, removed.


> 
> > +			echo
> > +		fi
> > +	done
> > +
> > +	# now find where to insert this name
> > +	eof=1
> > +	for found in `tail -n +$line $tdir/group | $AWK_PROG '{ print $1 }'`; do
> > +		found_id=$(echo "$found" | tr - ' ' | $AWK_PROG '{ print $1 }')
> 
> That's an interesting way of cutting up a string.
> 
> 		found_id=$(echo "$found" | cut -d "-" -f 1 -)

:-)

> 
> > +		line=$((line+1))
> > +		if [ -z "$found" ] || [ "$found" == "#" ]; then
> > +			continue
> > +		elif [[ $found > $name ]] || [ $found_id -gt $id ]; then
> 
> This doesn't work. $found will be "298-I-want-a-spoon" or just
> "298", but $name has no number prefix i.e. "but-I-like-sporks". We
> will always have number prefixes, and tests should always have unique
> numbers, so there is no reason to check anything but $id against
> $found_id.  Which means:
> 
> 	for found_id in `tail -n +$line $tdir/group | cut -d "-" -f 1" -`; do
> 		line=$((line+1))
> 		if [ -z "$found" ] || [ "$found" == "#" ]; then
> 			continue
> 		elif [ $found_id -gt $id ]; then
> 			eof=0
> 			break
> 		fi
> 	done
> 

I see. Most probably, it is a relict from before unique IDs. 
Fixed, thank you.

Jan


> And once we've done all this and decided on a test name, we should
> finally validate it again against $VALID_TEST_NAME....
> 
> Cheers,
> 
> Dave.
> 
> 
> 
> --
> Dave Chinner
> david@fromorbit.com
> 
--
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 April 1, 2015, 12:15 p.m. UTC | #5
On Wed, 1 Apr 2015, Jan Tulak wrote:

> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -21,6 +21,7 @@
> > >  #-----------------------------------------------------------------------
> > >  
> > >  BC=$(which bc 2> /dev/null) || BC=
> > > +SUPPORTED_TESTS="[a-zA-Z0-9-]\+"
> > 
> > but $SUPPORTED_TESTS doesn't define the correct format for the test
> > names. It defines valid characters, but does not enforce the fact
> > that the first 3 characters must match [0-9] (i.e. the NNN bit), and
> > then the rest is optional but must be alphanumeric symbols and dash.
> > i.e it should be matching against:
> > 
> 
> I did it to be "generous on input, strict on output" - the ./new script always put 3 numbers at the beginning, but if someone for some reason needs to avoid it, it will still work. Enforcing it is easy, but is it needed?

Hi Jan,

I thought that we've already established that indeed yes, it is
needed. Having ./new script to create the right format is nice, but
it's not enough.

You can easily create new tests without ./new script and while I do
not know how many people are using it, I've never used it myself.

Please enforce the naming format.

Thanks!
-Lukas
--
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..1841052 100644
--- a/README
+++ b/README
@@ -202,10 +202,15 @@  Test script environment:
 	- When calling getfacl in a test, pass the "-n" argument so
 	  that numeric rather than symbolic identifiers are used in
 	  the output.
+	- When creating a new test, it is possible to enter a custom name
+	  for the file. Filenames are in form NNN-custom-name, where NNN
+	  is automatically added by the ./new script as an unique ID,
+	  and "custom-name" is the optional string entered into a prompt
+	  in the ./new script. Note the "NNN-" part is added automatically.
 
 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..df9a5be 100755
--- a/check
+++ b/check
@@ -58,7 +58,6 @@  then
     exit 1
 fi
 
-SUPPORTED_TESTS="[0-9][0-9][0-9] [0-9][0-9][0-9][0-9]"
 SRC_GROUPS="generic shared"
 export SRC_DIR="tests"
 
@@ -96,21 +95,21 @@  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 in common/rc
 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/common/rc b/common/rc
index 857308a..cc9a894 100644
--- a/common/rc
+++ b/common/rc
@@ -21,6 +21,7 @@ 
 #-----------------------------------------------------------------------
 
 BC=$(which bc 2> /dev/null) || BC=
+SUPPORTED_TESTS="[a-zA-Z0-9-]\+"
 
 _require_math()
 {
diff --git a/new b/new
index d1f8939..7e9ce2c 100755
--- a/new
+++ b/new
@@ -81,11 +81,14 @@  line=0
 eof=1
 [ -f "$tdir/group" ] || usage
 
-for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
+for found in `cat $tdir/group | tr - ' ' | $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`
@@ -100,7 +103,47 @@  if [ $eof -eq 1 ]; then
    id=`printf "%03d" $i`
 fi
 
-echo "Next test is $id"
+echo "Next test id is $id"
+
+read -p "Append a name to the ID? Test name will be $id-\$name. y,[n]: " -r
+if [[ $REPLY = [Yy] ]]; then
+	# get the new name from user
+	name=""
+	while [ "$name" = "" ]; do
+		read -p "Enter the name: "
+		if [ "$REPLY" = "" ]; then
+			echo "For canceling, use ctrl+c."
+		elif [ -e "$tdir/$id-$REPLY" ]; then
+			echo "File '$id-$REPLY' already exists, use another one."
+			echo #
+		elif echo "$REPLY" | grep -q "^$SUPPORTED_TESTS$"; then
+			name="$REPLY"
+		else
+			echo "Filename must contain only alphanumeric symbols and dash!"
+			echo "(Used regex: ^$SUPPORTED_TESTS$)"
+			echo
+		fi
+	done
+
+	# now find where to insert this name
+	eof=1
+	for found in `tail -n +$line $tdir/group | $AWK_PROG '{ print $1 }'`; do
+		found_id=$(echo "$found" | tr - ' ' | $AWK_PROG '{ print $1 }')
+		line=$((line+1))
+		if [ -z "$found" ] || [ "$found" == "#" ]; then
+			continue
+		elif [[ $found > $name ]] || [ $found_id -gt $id ]; then
+			eof=0
+			break
+		fi
+	done
+	if [ $eof -eq 0 ]; then
+		# If place wasn't found, let $line be the end of the file
+		line=$((line-1))
+	fi
+	id="$id-$name"
+fi
+echo "Creating test file '$id'"
 
 if [ -f $tdir/$id ]
 then
@@ -115,7 +158,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?
 #