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.
>> > -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.
--
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
