[PATCH] generic: fix cleanup function for test 490

[PATCH] generic: fix cleanup function for test 490

[Theodore Ts'o]

generic/490 fails because cleanup tries to delete . and .. since $tmp
is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
delete temp files created by seek_sanity_test.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/generic/490 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/490 b/tests/generic/490
index b5042c2e..c0335ca0 100755
--- a/tests/generic/490
+++ b/tests/generic/490
@@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
 _cleanup()
 {
 	cd /
-	rm -f $tmp.*
+	rm -f $base_test_file*
 }
 
 $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
-- 
2.16.1.72.g5be1f00a9a

Re: [PATCH] generic: fix cleanup function for test 490

[Eryu Guan]

On Sun, May 20, 2018 at 02:45:17PM -0400, Theodore Ts'o wrote:
> generic/490 fails because cleanup tries to delete . and .. since $tmp
> is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
> delete temp files created by seek_sanity_test.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Sorry, this is my bad, Jan's original patch did just remove the
test files, I intended to use lower case variable name but did wrong
update in _cleanup(). Thanks for the fix!

Eryu
> ---
>  tests/generic/490 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/490 b/tests/generic/490
> index b5042c2e..c0335ca0 100755
> --- a/tests/generic/490
> +++ b/tests/generic/490
> @@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
>  _cleanup()
>  {
>  	cd /
> -	rm -f $tmp.*
> +	rm -f $base_test_file*
>  }
>  
>  $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> -- 
> 2.16.1.72.g5be1f00a9a
> 

Re: [PATCH] generic: fix cleanup function for test 490

[Dave Chinner]

On Sun, May 20, 2018 at 02:45:17PM -0400, Theodore Ts'o wrote:
> generic/490 fails because cleanup tries to delete . and .. since $tmp
> is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
> delete temp files created by seek_sanity_test.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  tests/generic/490 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/490 b/tests/generic/490
> index b5042c2e..c0335ca0 100755
> --- a/tests/generic/490
> +++ b/tests/generic/490
> @@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
>  _cleanup()
>  {
>  	cd /
> -	rm -f $tmp.*
> +	rm -f $base_test_file*
>  }
>  
>  $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||

This is wrong. $tmp must always be set in a test, as the
infrastructure uses it. Failing to set $tmp will result in stray
files being spewed around the place by the test and not cleaned up.
Hence the "rm -f $tmp.*" line must stay, the normal tmp=/tmp/$$ line
added to the test preamble, and then you can remove the base test
file...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic: fix cleanup function for test 490

[Eryu Guan]

On Mon, May 21, 2018 at 12:41:44PM +1000, Dave Chinner wrote:
> On Sun, May 20, 2018 at 02:45:17PM -0400, Theodore Ts'o wrote:
> > generic/490 fails because cleanup tries to delete . and .. since $tmp
> > is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
> > delete temp files created by seek_sanity_test.
> > 
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> >  tests/generic/490 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/generic/490 b/tests/generic/490
> > index b5042c2e..c0335ca0 100755
> > --- a/tests/generic/490
> > +++ b/tests/generic/490
> > @@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
> >  _cleanup()
> >  {
> >  	cd /
> > -	rm -f $tmp.*
> > +	rm -f $base_test_file*
> >  }
> >  
> >  $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> 
> This is wrong. $tmp must always be set in a test, as the
> infrastructure uses it. Failing to set $tmp will result in stray
> files being spewed around the place by the test and not cleaned up.
> Hence the "rm -f $tmp.*" line must stay, the normal tmp=/tmp/$$ line
> added to the test preamble, and then you can remove the base test
> file...

Yeah, I wanted to point that out too. So always using the './new' script
is the recommended way to write new test, it already puts all the common
setups (including the 'tmp=/tmp/$$' definition and 'rm -f $tmp.*'
cleanup) in the test template.

Thanks,
Eryu

Re: [PATCH] generic: fix cleanup function for test 490

[Dave Chinner]

On Mon, May 21, 2018 at 10:54:00AM +0800, Eryu Guan wrote:
> On Mon, May 21, 2018 at 12:41:44PM +1000, Dave Chinner wrote:
> > On Sun, May 20, 2018 at 02:45:17PM -0400, Theodore Ts'o wrote:
> > > generic/490 fails because cleanup tries to delete . and .. since $tmp
> > > is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
> > > delete temp files created by seek_sanity_test.
> > > 
> > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > ---
> > >  tests/generic/490 | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/generic/490 b/tests/generic/490
> > > index b5042c2e..c0335ca0 100755
> > > --- a/tests/generic/490
> > > +++ b/tests/generic/490
> > > @@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
> > >  _cleanup()
> > >  {
> > >  	cd /
> > > -	rm -f $tmp.*
> > > +	rm -f $base_test_file*
> > >  }
> > >  
> > >  $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> > 
> > This is wrong. $tmp must always be set in a test, as the
> > infrastructure uses it. Failing to set $tmp will result in stray
> > files being spewed around the place by the test and not cleaned up.
> > Hence the "rm -f $tmp.*" line must stay, the normal tmp=/tmp/$$ line
> > added to the test preamble, and then you can remove the base test
> > file...
> 
> Yeah, I wanted to point that out too. So always using the './new' script
> is the recommended way to write new test, it already puts all the common
> setups (including the 'tmp=/tmp/$$' definition and 'rm -f $tmp.*'
> cleanup) in the test template.

Is there a fix committed for this yet? This bug causes the test to
run rm -f .* in the root directory of the machine....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Dave Chinner]

On Fri, May 25, 2018 at 10:29:14AM +1000, Dave Chinner wrote:
> On Mon, May 21, 2018 at 10:54:00AM +0800, Eryu Guan wrote:
> > On Mon, May 21, 2018 at 12:41:44PM +1000, Dave Chinner wrote:
> > > On Sun, May 20, 2018 at 02:45:17PM -0400, Theodore Ts'o wrote:
> > > > generic/490 fails because cleanup tries to delete . and .. since $tmp
> > > > is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
> > > > delete temp files created by seek_sanity_test.
> > > > 
> > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > ---
> > > >  tests/generic/490 | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tests/generic/490 b/tests/generic/490
> > > > index b5042c2e..c0335ca0 100755
> > > > --- a/tests/generic/490
> > > > +++ b/tests/generic/490
> > > > @@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
> > > >  _cleanup()
> > > >  {
> > > >  	cd /
> > > > -	rm -f $tmp.*
> > > > +	rm -f $base_test_file*
> > > >  }
> > > >  
> > > >  $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> > > 
> > > This is wrong. $tmp must always be set in a test, as the
> > > infrastructure uses it. Failing to set $tmp will result in stray
> > > files being spewed around the place by the test and not cleaned up.
> > > Hence the "rm -f $tmp.*" line must stay, the normal tmp=/tmp/$$ line
> > > added to the test preamble, and then you can remove the base test
> > > file...
> > 
> > Yeah, I wanted to point that out too. So always using the './new' script
> > is the recommended way to write new test, it already puts all the common
> > setups (including the 'tmp=/tmp/$$' definition and 'rm -f $tmp.*'
> > cleanup) in the test template.
> 
> Is there a fix committed for this yet? This bug causes the test to
> run rm -f .* in the root directory of the machine....

BTW, in looking at what was missing from the test setup template in
this test, I noticed that it was also missing the setup of $here,
removal of $seqres.full, etc

SO I checked on how many tests fail to set $here, which is used in
the common/rc test infrastructure:

$ git grep -l 'here=`pwd`' tests/ > t.t
$ ls -1 tests/*/*[0-9] |grep -v out > tt.t
$ diff -u t.t tt.t |grep ^+ |wc -l
137
$

And, well, this is potentially very dangerous:

$ git grep -l 'tmp=/tmp/\$\$' tests/ > t.t
$ ls -1 tests/*/*[0-9] |grep -v out > tt.t
$ diff -u t.t tt.t |grep ^+ |wc -l
49
$

A bunch of test use different tmp setups (e.g. mktemp -d, some put
them in $TEST_DIR) but many omit it completely. They really all
should use the same base location.

IOWs there's 137 tests that have $here incorrectly/not set up, and
49 tests that don't initialise $tmp like they are supposed to. A
large number of these are relatively new tests, and the setup of
this variable is done by the "new" test script.

So why are so many new tests missing stuff that the 'new' template
sets up? Do we have a copy-n-paste process problem?  Should we
abstract out this test setup preamble so people don't keep screwing
it up?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Dave Chinner]

On Fri, May 25, 2018 at 10:59:40AM +1000, Dave Chinner wrote:
> On Fri, May 25, 2018 at 10:29:14AM +1000, Dave Chinner wrote:
> > On Mon, May 21, 2018 at 10:54:00AM +0800, Eryu Guan wrote:
> > > On Mon, May 21, 2018 at 12:41:44PM +1000, Dave Chinner wrote:
> > > > On Sun, May 20, 2018 at 02:45:17PM -0400, Theodore Ts'o wrote:
> > > > > generic/490 fails because cleanup tries to delete . and .. since $tmp
> > > > > is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
> > > > > delete temp files created by seek_sanity_test.
> > > > > 
> > > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > > ---
> > > > >  tests/generic/490 | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tests/generic/490 b/tests/generic/490
> > > > > index b5042c2e..c0335ca0 100755
> > > > > --- a/tests/generic/490
> > > > > +++ b/tests/generic/490
> > > > > @@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
> > > > >  _cleanup()
> > > > >  {
> > > > >  	cd /
> > > > > -	rm -f $tmp.*
> > > > > +	rm -f $base_test_file*
> > > > >  }
> > > > >  
> > > > >  $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> > > > 
> > > > This is wrong. $tmp must always be set in a test, as the
> > > > infrastructure uses it. Failing to set $tmp will result in stray
> > > > files being spewed around the place by the test and not cleaned up.
> > > > Hence the "rm -f $tmp.*" line must stay, the normal tmp=/tmp/$$ line
> > > > added to the test preamble, and then you can remove the base test
> > > > file...
> > > 
> > > Yeah, I wanted to point that out too. So always using the './new' script
> > > is the recommended way to write new test, it already puts all the common
> > > setups (including the 'tmp=/tmp/$$' definition and 'rm -f $tmp.*'
> > > cleanup) in the test template.
> > 
> > Is there a fix committed for this yet? This bug causes the test to
> > run rm -f .* in the root directory of the machine....
> 
> BTW, in looking at what was missing from the test setup template in
> this test, I noticed that it was also missing the setup of $here,
> removal of $seqres.full, etc
> 
> SO I checked on how many tests fail to set $here, which is used in
> the common/rc test infrastructure:
> 
> $ git grep -l 'here=`pwd`' tests/ > t.t
> $ ls -1 tests/*/*[0-9] |grep -v out > tt.t
> $ diff -u t.t tt.t |grep ^+ |wc -l
> 137
> $
> 
> And, well, this is potentially very dangerous:
> 
> $ git grep -l 'tmp=/tmp/\$\$' tests/ > t.t
> $ ls -1 tests/*/*[0-9] |grep -v out > tt.t
> $ diff -u t.t tt.t |grep ^+ |wc -l
> 49
> $
> 
> A bunch of test use different tmp setups (e.g. mktemp -d, some put
> them in $TEST_DIR) but many omit it completely. They really all
> should use the same base location.
> 
> IOWs there's 137 tests that have $here incorrectly/not set up, and
> 49 tests that don't initialise $tmp like they are supposed to. A
> large number of these are relatively new tests, and the setup of
> this variable is done by the "new" test script.
> 
> So why are so many new tests missing stuff that the 'new' template
> sets up? Do we have a copy-n-paste process problem?  Should we
> abstract out this test setup preamble so people don't keep screwing
> it up?

Abstracting it out would look like the patch below. That, and moving
to SPDX tags for the licensing text, will greatly clean up the test
setup preamble. It will also draw a line in the sand for new tests -
if they don't match the new setup preamble, they don't pass
review...

Thoughts?

-Dave.
-- 
Dave Chinner
david@fromorbit.com


fstests: generic test setup preamble

From: Dave Chinner <dchinner@redhat.com>

So many tests chop random bits out of the mandaotry test setup
preamble which leads to subtle problems and stray files being dumped
all over the place. Create a common test preamble with hooks for
local cleanup functions so we can prevent this in future.

Signed-off-by: Dave Chinner <dchinner@redhat.com>

---
 common/setup_test | 22 ++++++++++++++++++++++
 new               | 24 ++++--------------------
 tests/generic/001 | 19 +++++++------------
 3 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/common/setup_test b/common/setup_test
new file mode 100644
index 000000000000..6359ab53a8d9
--- /dev/null
+++ b/common/setup_test
@@ -0,0 +1,22 @@
+# common test setup preamble
+# test specific cleanup is done via the local_cleanup() function now.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+    local_cleanup
+    cd /
+    rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
diff --git a/new b/new
index 4eacccd3bf8b..5e7f6a3c4198 100755
--- a/new
+++ b/new
@@ -181,31 +181,15 @@ cat <<End-of-File >$tdir/$id
 #-----------------------------------------------------------------------
 #
 
-seq=\`basename \$0\`
-seqres=\$RESULT_DIR/\$seq
-echo "QA output created by \$seq"
+. common/setup_test
 
-here=\`pwd\`
-tmp=/tmp/\$\$
-status=1	# failure is the default!
-trap "_cleanup; exit \\\$status" 0 1 2 3 15
-
-_cleanup()
-{
-	cd /
-	rm -f \$tmp.*
-}
-
-# get standard environment, filters and checks
-. ./common/rc
-. ./common/filter
+# test exit cleanup goes here
+local_cleanup() { true }
 
 # remove previous \$seqres.full before test
 rm -f \$seqres.full
 
-# real QA test starts here
-
-# Modify as appropriate.
+# include test specific environments here
 _supported_fs generic
 _supported_os Linux
 _require_test
diff --git a/tests/generic/001 b/tests/generic/001
index 0edd41f1e2cc..e1dd2e3202e3 100755
--- a/tests/generic/001
+++ b/tests/generic/001
@@ -31,25 +31,20 @@
 #-----------------------------------------------------------------------
 #
 
-seq=`basename $0`
-seqres=$RESULT_DIR/$seq
-echo "QA output created by $seq"
+. common/setup_test
 
-# get standard environment, filters and checks
-. ./common/rc
-. ./common/filter
+# test exit cleanup goes here
+local_cleanup() { true }
 
-tmp=/tmp/$$
-here=`pwd`
-status=1
-done_cleanup=false
-trap "_cleanup; rm -f $tmp.*; exit \$status" 0 1 2 3 15
+# remove previous $seqres.full before test
+rm -f $seqres.full
 
-# real QA test starts here
+# include test specific environments here
 _supported_fs generic
 _supported_os Linux
 _require_test
 
+done_cleanup=false
 verbose=true
 verify=$here/verify_fill
 

Re: Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Eric Biggers]

Hi Dave,

On Fri, May 25, 2018 at 11:23:36AM +1000, Dave Chinner wrote:
> On Fri, May 25, 2018 at 10:59:40AM +1000, Dave Chinner wrote:
> > On Fri, May 25, 2018 at 10:29:14AM +1000, Dave Chinner wrote:
> > > On Mon, May 21, 2018 at 10:54:00AM +0800, Eryu Guan wrote:
> > > > On Mon, May 21, 2018 at 12:41:44PM +1000, Dave Chinner wrote:
> > > > > On Sun, May 20, 2018 at 02:45:17PM -0400, Theodore Ts'o wrote:
> > > > > > generic/490 fails because cleanup tries to delete . and .. since $tmp
> > > > > > is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
> > > > > > delete temp files created by seek_sanity_test.
> > > > > > 
> > > > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > > > ---
> > > > > >  tests/generic/490 | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/tests/generic/490 b/tests/generic/490
> > > > > > index b5042c2e..c0335ca0 100755
> > > > > > --- a/tests/generic/490
> > > > > > +++ b/tests/generic/490
> > > > > > @@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
> > > > > >  _cleanup()
> > > > > >  {
> > > > > >  	cd /
> > > > > > -	rm -f $tmp.*
> > > > > > +	rm -f $base_test_file*
> > > > > >  }
> > > > > >  
> > > > > >  $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> > > > > 
> > > > > This is wrong. $tmp must always be set in a test, as the
> > > > > infrastructure uses it. Failing to set $tmp will result in stray
> > > > > files being spewed around the place by the test and not cleaned up.
> > > > > Hence the "rm -f $tmp.*" line must stay, the normal tmp=/tmp/$$ line
> > > > > added to the test preamble, and then you can remove the base test
> > > > > file...
> > > > 
> > > > Yeah, I wanted to point that out too. So always using the './new' script
> > > > is the recommended way to write new test, it already puts all the common
> > > > setups (including the 'tmp=/tmp/$$' definition and 'rm -f $tmp.*'
> > > > cleanup) in the test template.
> > > 
> > > Is there a fix committed for this yet? This bug causes the test to
> > > run rm -f .* in the root directory of the machine....
> > 
> > BTW, in looking at what was missing from the test setup template in
> > this test, I noticed that it was also missing the setup of $here,
> > removal of $seqres.full, etc
> > 
> > SO I checked on how many tests fail to set $here, which is used in
> > the common/rc test infrastructure:
> > 
> > $ git grep -l 'here=`pwd`' tests/ > t.t
> > $ ls -1 tests/*/*[0-9] |grep -v out > tt.t
> > $ diff -u t.t tt.t |grep ^+ |wc -l
> > 137
> > $
> > 
> > And, well, this is potentially very dangerous:
> > 
> > $ git grep -l 'tmp=/tmp/\$\$' tests/ > t.t
> > $ ls -1 tests/*/*[0-9] |grep -v out > tt.t
> > $ diff -u t.t tt.t |grep ^+ |wc -l
> > 49
> > $
> > 
> > A bunch of test use different tmp setups (e.g. mktemp -d, some put
> > them in $TEST_DIR) but many omit it completely. They really all
> > should use the same base location.
> > 
> > IOWs there's 137 tests that have $here incorrectly/not set up, and
> > 49 tests that don't initialise $tmp like they are supposed to. A
> > large number of these are relatively new tests, and the setup of
> > this variable is done by the "new" test script.
> > 
> > So why are so many new tests missing stuff that the 'new' template
> > sets up? Do we have a copy-n-paste process problem?  Should we
> > abstract out this test setup preamble so people don't keep screwing
> > it up?
> 
> Abstracting it out would look like the patch below. That, and moving
> to SPDX tags for the licensing text, will greatly clean up the test
> setup preamble. It will also draw a line in the sand for new tests -
> if they don't match the new setup preamble, they don't pass
> review...
> 
> Thoughts?
> 

I agree with adding a common test preamble.  The current boilerplate is way too
verbose and easy to get wrong.  Even if technically you are supposed to use the
'new' script which does it right... the fact that you need to generate the code
using a script at all, instead of being able to write something simple and
obviously correct, is problematic.

> fstests: generic test setup preamble
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> So many tests chop random bits out of the mandaotry test setup
> preamble which leads to subtle problems and stray files being dumped
> all over the place. Create a common test preamble with hooks for
> local cleanup functions so we can prevent this in future.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> ---
>  common/setup_test | 22 ++++++++++++++++++++++
>  new               | 24 ++++--------------------
>  tests/generic/001 | 19 +++++++------------
>  3 files changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/common/setup_test b/common/setup_test
> new file mode 100644
> index 000000000000..6359ab53a8d9
> --- /dev/null
> +++ b/common/setup_test
> @@ -0,0 +1,22 @@
> +# common test setup preamble
> +# test specific cleanup is done via the local_cleanup() function now.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +    local_cleanup
> +    cd /
> +    rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> diff --git a/new b/new
> index 4eacccd3bf8b..5e7f6a3c4198 100755
> --- a/new
> +++ b/new
> @@ -181,31 +181,15 @@ cat <<End-of-File >$tdir/$id
>  #-----------------------------------------------------------------------
>  #
>  
> -seq=\`basename \$0\`
> -seqres=\$RESULT_DIR/\$seq
> -echo "QA output created by \$seq"
> +. common/setup_test
>  
> -here=\`pwd\`
> -tmp=/tmp/\$\$
> -status=1	# failure is the default!
> -trap "_cleanup; exit \\\$status" 0 1 2 3 15
> -
> -_cleanup()
> -{
> -	cd /
> -	rm -f \$tmp.*
> -}
> -
> -# get standard environment, filters and checks
> -. ./common/rc
> -. ./common/filter
> +# test exit cleanup goes here
> +local_cleanup() { true }

Did you consider defining the no-op local_cleanup() in the common preamble, and
having tests override it if needed by redefining local_cleanup()?  Or
alternatively, the preamble could use use 'type -t' to check if local_cleanup is
defined before calling it.  Either is slightly error prone as a typo in the
function name may go unnoticed, but it would save boilerplate from most tests.

> +# remove previous $seqres.full before test
> +rm -f $seqres.full

Why can't the $seqres.full removal be done in the common preamble?

Thanks,

- Eric

Re: Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Dave Chinner]

On Thu, May 24, 2018 at 09:17:59PM -0700, Eric Biggers wrote:
> On Fri, May 25, 2018 at 11:23:36AM +1000, Dave Chinner wrote:
> > Abstracting it out would look like the patch below. That, and moving
> > to SPDX tags for the licensing text, will greatly clean up the test
> > setup preamble. It will also draw a line in the sand for new tests -
> > if they don't match the new setup preamble, they don't pass
> > review...
> > 
> > Thoughts?
> > 
> 
> I agree with adding a common test preamble.  The current boilerplate is way too
> verbose and easy to get wrong.  Even if technically you are supposed to use the
> 'new' script which does it right... the fact that you need to generate the code
> using a script at all, instead of being able to write something simple and
> obviously correct, is problematic.

Using code to write code is a much more efficient way of generating
new code than having people copy-and-waste. The problem is that
people still copy and paste, despite the documentation saying "don't
do that, use the script" and the script being fast and easy to use.

The moral of the story: no matter what we do, people will not follow
the rules and they'll do whatever they damn well want.

> > -here=\`pwd\`
> > -tmp=/tmp/\$\$
> > -status=1	# failure is the default!
> > -trap "_cleanup; exit \\\$status" 0 1 2 3 15
> > -
> > -_cleanup()
> > -{
> > -	cd /
> > -	rm -f \$tmp.*
> > -}
> > -
> > -# get standard environment, filters and checks
> > -. ./common/rc
> > -. ./common/filter
> > +# test exit cleanup goes here
> > +local_cleanup() { true }
> 
> Did you consider defining the no-op local_cleanup() in the common preamble, and
> having tests override it if needed by redefining local_cleanup()?

Yes.

> Or
> alternatively, the preamble could use use 'type -t' to check if local_cleanup is
> defined before calling it. 

Didn't know that trick, but it has the same problem as above.

> Either is slightly error prone as a typo in the
> function name may go unnoticed, but it would save boilerplate from most tests.

Yup, far too likely to have people do their own/wrong thing because
we know they won't/don't follow documented instructions for creating
new tests.

> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> 
> Why can't the $seqres.full removal be done in the common preamble?

Because it's common during test failure debugging to comment out the
removal of this file. I do it all the time, and I'm sure other
people do too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Amir Goldstein]

On Fri, May 25, 2018 at 9:25 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, May 24, 2018 at 09:17:59PM -0700, Eric Biggers wrote:
>> On Fri, May 25, 2018 at 11:23:36AM +1000, Dave Chinner wrote:
>> > Abstracting it out would look like the patch below. That, and moving
>> > to SPDX tags for the licensing text, will greatly clean up the test
>> > setup preamble. It will also draw a line in the sand for new tests -
>> > if they don't match the new setup preamble, they don't pass
>> > review...
>> >
>> > Thoughts?
>> >
>>
>> I agree with adding a common test preamble.  The current boilerplate is way too
>> verbose and easy to get wrong.  Even if technically you are supposed to use the
>> 'new' script which does it right... the fact that you need to generate the code
>> using a script at all, instead of being able to write something simple and
>> obviously correct, is problematic.
>
> Using code to write code is a much more efficient way of generating
> new code than having people copy-and-waste. The problem is that
> people still copy and paste, despite the documentation saying "don't
> do that, use the script" and the script being fast and easy to use.
>
> The moral of the story: no matter what we do, people will not follow
> the rules and they'll do whatever they damn well want.
>

As a frequent copy&paste offender, I welcome this change very much.
Keep in mind that it is a very common practice in xfstests to create
a variant of an existing test, so copy & pasting will likely not stop.

>> > -here=\`pwd\`
>> > -tmp=/tmp/\$\$
>> > -status=1   # failure is the default!
>> > -trap "_cleanup; exit \\\$status" 0 1 2 3 15
>> > -
>> > -_cleanup()
>> > -{
>> > -   cd /
>> > -   rm -f \$tmp.*
>> > -}
>> > -
>> > -# get standard environment, filters and checks
>> > -. ./common/rc
>> > -. ./common/filter
>> > +# test exit cleanup goes here
>> > +local_cleanup() { true }
>>
>> Did you consider defining the no-op local_cleanup() in the common preamble, and
>> having tests override it if needed by redefining local_cleanup()?
>
> Yes.
>

Maybe just cleanup() for local cleanup as is the convention in all the tests
that _funcs() are global and funcs() are local.

Did you notice that your patch left an overriding _cleanup() function
in generic/001
that does not call local_cleanup(), but that does do local cleanups.
I don't think that is what you intended do to.
Also, as an example you picked the only test that actually calls _cleanup()
at start of test as well - not that it matters much, but the cleanup
and handling
of $TEST_DIR/$$ is quite bizarre and needs to be killed, so you may want
to pick another poster child ;-)

BTW, there are only 3 other tests that call _cleanup directly, but they
do it just before calling exit... (generic/{124,127,236}).

>> Or
>> alternatively, the preamble could use use 'type -t' to check if local_cleanup is
>> defined before calling it.
>
> Didn't know that trick, but it has the same problem as above.
>
>> Either is slightly error prone as a typo in the
>> function name may go unnoticed, but it would save boilerplate from most tests.
>
> Yup, far too likely to have people do their own/wrong thing because
> we know they won't/don't follow documented instructions for creating
> new tests.
>
>> > +# remove previous $seqres.full before test
>> > +rm -f $seqres.full
>>
>> Why can't the $seqres.full removal be done in the common preamble?
>
> Because it's common during test failure debugging to comment out the
> removal of this file. I do it all the time, and I'm sure other
> people do too.
>

IDGI, this is removing the full log of previous test run.
Why is it useful to comment it out?
And if it is useful, better control keeping old log with env var and
still keep the default code in setup_test, e.g.:
KEEP_FULL_LOG=1 ./check ...


Thanks,
Amir.

Re: Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Dave Chinner]

On Fri, May 25, 2018 at 11:06:23AM +0300, Amir Goldstein wrote:
> On Fri, May 25, 2018 at 9:25 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, May 24, 2018 at 09:17:59PM -0700, Eric Biggers wrote:
> >> On Fri, May 25, 2018 at 11:23:36AM +1000, Dave Chinner wrote:
> >> > Abstracting it out would look like the patch below. That, and moving
> >> > to SPDX tags for the licensing text, will greatly clean up the test
> >> > setup preamble. It will also draw a line in the sand for new tests -
> >> > if they don't match the new setup preamble, they don't pass
> >> > review...
> >> >
> >> > Thoughts?
> >> >
> >>
> >> I agree with adding a common test preamble.  The current boilerplate is way too
> >> verbose and easy to get wrong.  Even if technically you are supposed to use the
> >> 'new' script which does it right... the fact that you need to generate the code
> >> using a script at all, instead of being able to write something simple and
> >> obviously correct, is problematic.
> >
> > Using code to write code is a much more efficient way of generating
> > new code than having people copy-and-waste. The problem is that
> > people still copy and paste, despite the documentation saying "don't
> > do that, use the script" and the script being fast and easy to use.
> >
> > The moral of the story: no matter what we do, people will not follow
> > the rules and they'll do whatever they damn well want.
> >
> 
> As a frequent copy&paste offender, I welcome this change very much.
> Keep in mind that it is a very common practice in xfstests to create
> a variant of an existing test, so copy & pasting will likely not stop.

My thoughts exactly :P

> >> > -here=\`pwd\`
> >> > -tmp=/tmp/\$\$
> >> > -status=1   # failure is the default!
> >> > -trap "_cleanup; exit \\\$status" 0 1 2 3 15
> >> > -
> >> > -_cleanup()
> >> > -{
> >> > -   cd /
> >> > -   rm -f \$tmp.*
> >> > -}
> >> > -
> >> > -# get standard environment, filters and checks
> >> > -. ./common/rc
> >> > -. ./common/filter
> >> > +# test exit cleanup goes here
> >> > +local_cleanup() { true }
> >>
> >> Did you consider defining the no-op local_cleanup() in the common preamble, and
> >> having tests override it if needed by redefining local_cleanup()?
> >
> > Yes.
> >
> 
> Maybe just cleanup() for local cleanup as is the convention in all the tests
> that _funcs() are global and funcs() are local.

Yup, I wrote then deleted in my last reply that I've renamed it to
just "cleanup().

> 
> Did you notice that your patch left an overriding _cleanup() function
> in generic/001
> that does not call local_cleanup(), but that does do local cleanups.

Yup - I didn't change any of the code in that test as I was just
demonstrating the sort of cleanup needed. generic/001 uses _cleanup
as a local function called between two different parts of the test,
not as an error trap, so more work is needed to fix it completely.

> I don't think that is what you intended do to.  Also, as an
> example you picked the only test that actually calls _cleanup() at
> start of test as well - not that it matters much, but the cleanup
> and handling of $TEST_DIR/$$ is quite bizarre and needs to be
> killed, so you may want to pick another poster child ;-)

I'm not going to fix every problem that this specific test has - I
want to fix the wider, more general problem first.  I just picked it
as an example for an initial RFC, not because it is a complete,
fully working patch ready for inclusion. There's 800+ plus
files that need work as part of this cleanup, so I want to make sure
the cleanup I'm going to do is acceptable before spending any time
on it.


> BTW, there are only 3 other tests that call _cleanup directly, but they
> do it just before calling exit... (generic/{124,127,236}).
> 
> >> Or
> >> alternatively, the preamble could use use 'type -t' to check if local_cleanup is
> >> defined before calling it.
> >
> > Didn't know that trick, but it has the same problem as above.
> >
> >> Either is slightly error prone as a typo in the
> >> function name may go unnoticed, but it would save boilerplate from most tests.
> >
> > Yup, far too likely to have people do their own/wrong thing because
> > we know they won't/don't follow documented instructions for creating
> > new tests.
> >
> >> > +# remove previous $seqres.full before test
> >> > +rm -f $seqres.full
> >>
> >> Why can't the $seqres.full removal be done in the common preamble?
> >
> > Because it's common during test failure debugging to comment out the
> > removal of this file. I do it all the time, and I'm sure other
> > people do too.
> >
> 
> IDGI, this is removing the full log of previous test run.
> Why is it useful to comment it out?

Because when you have a test that doesn't fail every time and you
run it in a loop, you don't know which of the test runs is going to
fail, and often you get different failure signatures. Collecting
them all is easy this way - I've been using xfstests this way for the
last 15 years and I see no reason to make this harder to acheive.

> And if it is useful, better control keeping old log with env var and
> still keep the default code in setup_test, e.g.:
> KEEP_FULL_LOG=1 ./check ...

I don't like magic env settings that nobody (including me) will
remember or use and so will just bit rot. I'm trying to remove
technical debt from xfstests, not add more.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Eric Biggers]

On Fri, May 25, 2018 at 06:45:19PM +1000, Dave Chinner wrote:
> On Fri, May 25, 2018 at 11:06:23AM +0300, Amir Goldstein wrote:
> > On Fri, May 25, 2018 at 9:25 AM, Dave Chinner <david@fromorbit.com> wrote:
> > > On Thu, May 24, 2018 at 09:17:59PM -0700, Eric Biggers wrote:
> > >> On Fri, May 25, 2018 at 11:23:36AM +1000, Dave Chinner wrote:
> > >> > Abstracting it out would look like the patch below. That, and moving
> > >> > to SPDX tags for the licensing text, will greatly clean up the test
> > >> > setup preamble. It will also draw a line in the sand for new tests -
> > >> > if they don't match the new setup preamble, they don't pass
> > >> > review...
> > >> >
> > >> > Thoughts?
> > >> >
> > >>
> > >> I agree with adding a common test preamble.  The current boilerplate is way too
> > >> verbose and easy to get wrong.  Even if technically you are supposed to use the
> > >> 'new' script which does it right... the fact that you need to generate the code
> > >> using a script at all, instead of being able to write something simple and
> > >> obviously correct, is problematic.
> > >
> > > Using code to write code is a much more efficient way of generating
> > > new code than having people copy-and-waste. The problem is that
> > > people still copy and paste, despite the documentation saying "don't
> > > do that, use the script" and the script being fast and easy to use.
> > >
> > > The moral of the story: no matter what we do, people will not follow
> > > the rules and they'll do whatever they damn well want.
> > >
> > 
> > As a frequent copy&paste offender, I welcome this change very much.
> > Keep in mind that it is a very common practice in xfstests to create
> > a variant of an existing test, so copy & pasting will likely not stop.
> 
> My thoughts exactly :P
> 
> > >> > -here=\`pwd\`
> > >> > -tmp=/tmp/\$\$
> > >> > -status=1   # failure is the default!
> > >> > -trap "_cleanup; exit \\\$status" 0 1 2 3 15
> > >> > -
> > >> > -_cleanup()
> > >> > -{
> > >> > -   cd /
> > >> > -   rm -f \$tmp.*
> > >> > -}
> > >> > -
> > >> > -# get standard environment, filters and checks
> > >> > -. ./common/rc
> > >> > -. ./common/filter
> > >> > +# test exit cleanup goes here
> > >> > +local_cleanup() { true }
> > >>
> > >> Did you consider defining the no-op local_cleanup() in the common preamble, and
> > >> having tests override it if needed by redefining local_cleanup()?
> > >
> > > Yes.
> > >
> > 
> > Maybe just cleanup() for local cleanup as is the convention in all the tests
> > that _funcs() are global and funcs() are local.
> 
> Yup, I wrote then deleted in my last reply that I've renamed it to
> just "cleanup().
> 
> > 
> > Did you notice that your patch left an overriding _cleanup() function
> > in generic/001
> > that does not call local_cleanup(), but that does do local cleanups.
> 
> Yup - I didn't change any of the code in that test as I was just
> demonstrating the sort of cleanup needed. generic/001 uses _cleanup
> as a local function called between two different parts of the test,
> not as an error trap, so more work is needed to fix it completely.
> 
> > I don't think that is what you intended do to.  Also, as an
> > example you picked the only test that actually calls _cleanup() at
> > start of test as well - not that it matters much, but the cleanup
> > and handling of $TEST_DIR/$$ is quite bizarre and needs to be
> > killed, so you may want to pick another poster child ;-)
> 
> I'm not going to fix every problem that this specific test has - I
> want to fix the wider, more general problem first.  I just picked it
> as an example for an initial RFC, not because it is a complete,
> fully working patch ready for inclusion. There's 800+ plus
> files that need work as part of this cleanup, so I want to make sure
> the cleanup I'm going to do is acceptable before spending any time
> on it.
> 
> 
> > BTW, there are only 3 other tests that call _cleanup directly, but they
> > do it just before calling exit... (generic/{124,127,236}).
> > 
> > >> Or
> > >> alternatively, the preamble could use use 'type -t' to check if local_cleanup is
> > >> defined before calling it.
> > >
> > > Didn't know that trick, but it has the same problem as above.
> > >
> > >> Either is slightly error prone as a typo in the
> > >> function name may go unnoticed, but it would save boilerplate from most tests.
> > >
> > > Yup, far too likely to have people do their own/wrong thing because
> > > we know they won't/don't follow documented instructions for creating
> > > new tests.
> > >
> > >> > +# remove previous $seqres.full before test
> > >> > +rm -f $seqres.full
> > >>
> > >> Why can't the $seqres.full removal be done in the common preamble?
> > >
> > > Because it's common during test failure debugging to comment out the
> > > removal of this file. I do it all the time, and I'm sure other
> > > people do too.
> > >
> > 
> > IDGI, this is removing the full log of previous test run.
> > Why is it useful to comment it out?
> 
> Because when you have a test that doesn't fail every time and you
> run it in a loop, you don't know which of the test runs is going to
> fail, and often you get different failure signatures. Collecting
> them all is easy this way - I've been using xfstests this way for the
> last 15 years and I see no reason to make this harder to acheive.
> 
> > And if it is useful, better control keeping old log with env var and
> > still keep the default code in setup_test, e.g.:
> > KEEP_FULL_LOG=1 ./check ...
> 
> I don't like magic env settings that nobody (including me) will
> remember or use and so will just bit rot. I'm trying to remove
> technical debt from xfstests, not add more.
> 

It could alternatively be an option passed to setup_test upon sourcing, so for
your debugging workflow you would just replace

. common/setup_test

with

. common/setup_test --keep-full-log

Having the same boilerplate copy-and-pasted in 800 places solely for the purpose
of being able to comment it out is very ugly.

- Eric

Re: Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Dave Chinner]

On Fri, May 25, 2018 at 03:33:00PM -0700, Eric Biggers wrote:
> On Fri, May 25, 2018 at 06:45:19PM +1000, Dave Chinner wrote:
> > On Fri, May 25, 2018 at 11:06:23AM +0300, Amir Goldstein wrote:
> > > On Fri, May 25, 2018 at 9:25 AM, Dave Chinner <david@fromorbit.com> wrote:
> > > >> > +# remove previous $seqres.full before test
> > > >> > +rm -f $seqres.full
> > > >>
> > > >> Why can't the $seqres.full removal be done in the common preamble?
> > > >
> > > > Because it's common during test failure debugging to comment out the
> > > > removal of this file. I do it all the time, and I'm sure other
> > > > people do too.
> > > >
> > > 
> > > IDGI, this is removing the full log of previous test run.
> > > Why is it useful to comment it out?
> > 
> > Because when you have a test that doesn't fail every time and you
> > run it in a loop, you don't know which of the test runs is going to
> > fail, and often you get different failure signatures. Collecting
> > them all is easy this way - I've been using xfstests this way for the
> > last 15 years and I see no reason to make this harder to acheive.
> > 
> > > And if it is useful, better control keeping old log with env var and
> > > still keep the default code in setup_test, e.g.:
> > > KEEP_FULL_LOG=1 ./check ...
> > 
> > I don't like magic env settings that nobody (including me) will
> > remember or use and so will just bit rot. I'm trying to remove
> > technical debt from xfstests, not add more.
> > 
> 
> It could alternatively be an option passed to setup_test upon sourcing, so for
> your debugging workflow you would just replace
> 
> . common/setup_test
> 
> with
> 
> . common/setup_test --keep-full-log
> 
> Having the same boilerplate copy-and-pasted in 800 places solely for the purpose
> of being able to comment it out is very ugly.

Which conflicts with the goal of having a common fixed test setup
preamble that should not be modified by anyone.

i.e. I want to move towards a clean separation of the required test
harness setup parameters vs things that tests can change without
impacting the test harness or common/ test code. Things like $tmp,
$here, etc are controlled by the test harness and used in the common
code. OTOH, removal of $seqres.full output files is a per-test
option, not a test harness parameter - many tests don't use it and
never remove it. Hence it's /definition/ needs to be part of the
test harness setup preamble, but it's removal should not be as that
is a per-test and/or test-run context specific behaviour.


Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Widespread test setup template problems (was Re: [PATCH] generic: fix cleanup function for test 490)

[Eryu Guan]

On Fri, May 25, 2018 at 11:23:36AM +1000, Dave Chinner wrote:
> On Fri, May 25, 2018 at 10:59:40AM +1000, Dave Chinner wrote:
> > On Fri, May 25, 2018 at 10:29:14AM +1000, Dave Chinner wrote:
> > > On Mon, May 21, 2018 at 10:54:00AM +0800, Eryu Guan wrote:
> > > > On Mon, May 21, 2018 at 12:41:44PM +1000, Dave Chinner wrote:
> > > > > On Sun, May 20, 2018 at 02:45:17PM -0400, Theodore Ts'o wrote:
> > > > > > generic/490 fails because cleanup tries to delete . and .. since $tmp
> > > > > > is left unset, and so "rm -f $tmp.*" does nothing useful.  Instead
> > > > > > delete temp files created by seek_sanity_test.
> > > > > > 
> > > > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > > > ---
> > > > > >  tests/generic/490 | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/tests/generic/490 b/tests/generic/490
> > > > > > index b5042c2e..c0335ca0 100755
> > > > > > --- a/tests/generic/490
> > > > > > +++ b/tests/generic/490
> > > > > > @@ -49,7 +49,7 @@ _require_test_program "seek_sanity_test"
> > > > > >  _cleanup()
> > > > > >  {
> > > > > >  	cd /
> > > > > > -	rm -f $tmp.*
> > > > > > +	rm -f $base_test_file*
> > > > > >  }
> > > > > >  
> > > > > >  $here/src/seek_sanity_test -s 19 -e 20 $base_test_file > $seqres.full 2>&1 ||
> > > > > 
> > > > > This is wrong. $tmp must always be set in a test, as the
> > > > > infrastructure uses it. Failing to set $tmp will result in stray
> > > > > files being spewed around the place by the test and not cleaned up.
> > > > > Hence the "rm -f $tmp.*" line must stay, the normal tmp=/tmp/$$ line
> > > > > added to the test preamble, and then you can remove the base test
> > > > > file...
> > > > 
> > > > Yeah, I wanted to point that out too. So always using the './new' script
> > > > is the recommended way to write new test, it already puts all the common
> > > > setups (including the 'tmp=/tmp/$$' definition and 'rm -f $tmp.*'
> > > > cleanup) in the test template.
> > > 
> > > Is there a fix committed for this yet? This bug causes the test to
> > > run rm -f .* in the root directory of the machine....
> > 
> > BTW, in looking at what was missing from the test setup template in
> > this test, I noticed that it was also missing the setup of $here,
> > removal of $seqres.full, etc
> > 
> > SO I checked on how many tests fail to set $here, which is used in
> > the common/rc test infrastructure:
> > 
> > $ git grep -l 'here=`pwd`' tests/ > t.t
> > $ ls -1 tests/*/*[0-9] |grep -v out > tt.t
> > $ diff -u t.t tt.t |grep ^+ |wc -l
> > 137
> > $
> > 
> > And, well, this is potentially very dangerous:
> > 
> > $ git grep -l 'tmp=/tmp/\$\$' tests/ > t.t
> > $ ls -1 tests/*/*[0-9] |grep -v out > tt.t
> > $ diff -u t.t tt.t |grep ^+ |wc -l
> > 49
> > $
> > 
> > A bunch of test use different tmp setups (e.g. mktemp -d, some put
> > them in $TEST_DIR) but many omit it completely. They really all
> > should use the same base location.
> > 
> > IOWs there's 137 tests that have $here incorrectly/not set up, and
> > 49 tests that don't initialise $tmp like they are supposed to. A
> > large number of these are relatively new tests, and the setup of
> > this variable is done by the "new" test script.
> > 
> > So why are so many new tests missing stuff that the 'new' template
> > sets up? Do we have a copy-n-paste process problem?  Should we
> > abstract out this test setup preamble so people don't keep screwing
> > it up?

That's my bad, sorry. I didn't spend much review bandwidth on test setup
preamble but focused more on test correctness.

> 
> Abstracting it out would look like the patch below. That, and moving
> to SPDX tags for the licensing text, will greatly clean up the test
> setup preamble. It will also draw a line in the sand for new tests -
> if they don't match the new setup preamble, they don't pass
> review...

I did post a patch, maybe a year ago, that went through all tests and
setup $tmp correctly (but not $here), and IIRC Amir suggested a similar
solution to your RFC patch. But that discussion only resulted in commit
81aa85deac1f ("fstests: remove tmp files properly") in the end.

> 
> Thoughts?

I have no problem with going to that direction, the common setup
preamble also makes review easier, I don't have to search for "tmp" and
"here" etc but just common/setup_test.

Thanks a lot for doing it!

Eryu

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> 
> fstests: generic test setup preamble
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> So many tests chop random bits out of the mandaotry test setup
> preamble which leads to subtle problems and stray files being dumped
> all over the place. Create a common test preamble with hooks for
> local cleanup functions so we can prevent this in future.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> ---
>  common/setup_test | 22 ++++++++++++++++++++++
>  new               | 24 ++++--------------------
>  tests/generic/001 | 19 +++++++------------
>  3 files changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/common/setup_test b/common/setup_test
> new file mode 100644
> index 000000000000..6359ab53a8d9
> --- /dev/null
> +++ b/common/setup_test
> @@ -0,0 +1,22 @@
> +# common test setup preamble
> +# test specific cleanup is done via the local_cleanup() function now.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +    local_cleanup
> +    cd /
> +    rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> diff --git a/new b/new
> index 4eacccd3bf8b..5e7f6a3c4198 100755
> --- a/new
> +++ b/new
> @@ -181,31 +181,15 @@ cat <<End-of-File >$tdir/$id
>  #-----------------------------------------------------------------------
>  #
>  
> -seq=\`basename \$0\`
> -seqres=\$RESULT_DIR/\$seq
> -echo "QA output created by \$seq"
> +. common/setup_test
>  
> -here=\`pwd\`
> -tmp=/tmp/\$\$
> -status=1	# failure is the default!
> -trap "_cleanup; exit \\\$status" 0 1 2 3 15
> -
> -_cleanup()
> -{
> -	cd /
> -	rm -f \$tmp.*
> -}
> -
> -# get standard environment, filters and checks
> -. ./common/rc
> -. ./common/filter
> +# test exit cleanup goes here
> +local_cleanup() { true }
>  
>  # remove previous \$seqres.full before test
>  rm -f \$seqres.full
>  
> -# real QA test starts here
> -
> -# Modify as appropriate.
> +# include test specific environments here
>  _supported_fs generic
>  _supported_os Linux
>  _require_test
> diff --git a/tests/generic/001 b/tests/generic/001
> index 0edd41f1e2cc..e1dd2e3202e3 100755
> --- a/tests/generic/001
> +++ b/tests/generic/001
> @@ -31,25 +31,20 @@
>  #-----------------------------------------------------------------------
>  #
>  
> -seq=`basename $0`
> -seqres=$RESULT_DIR/$seq
> -echo "QA output created by $seq"
> +. common/setup_test
>  
> -# get standard environment, filters and checks
> -. ./common/rc
> -. ./common/filter
> +# test exit cleanup goes here
> +local_cleanup() { true }
>  
> -tmp=/tmp/$$
> -here=`pwd`
> -status=1
> -done_cleanup=false
> -trap "_cleanup; rm -f $tmp.*; exit \$status" 0 1 2 3 15
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
>  
> -# real QA test starts here
> +# include test specific environments here
>  _supported_fs generic
>  _supported_os Linux
>  _require_test
>  
> +done_cleanup=false
>  verbose=true
>  verify=$here/verify_fill
>