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@xxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
