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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx -- 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
