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

[Dave Chinner <david@fromorbit.com>]

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