Re: [PATCH] fstests: cleanup tmp files in tests

[Eryu Guan <eguan@redhat.com>]

On Mon, Jan 09, 2017 at 12:46:23PM +0200, Amir Goldstein wrote:
> On Mon, Jan 9, 2017 at 12:17 PM, Eryu Guan <eguan@redhat.com> wrote:
> > Hi Eric and Amir,
> >
> > On Fri, Jan 06, 2017 at 08:57:31PM -0800, Eric Biggers wrote:
> >> Hi Eryu,
> >>
> >> On Fri, Jan 06, 2017 at 11:45:07AM +0800, Eryu Guan wrote:
> >> > $tmp.* files should be removed in _cleanup() even if the test is not
> >> > using any $tmp.* file explicitly, because common helper functions
> >> > may take use of them too.
> >> >
> >> > So cleanup tmp files properly in tests, and add a _cleanup()
> >> > function and trap it on exit if the test doesn't do so, to make all
> >> > tests consistent on the way they do cleanup.
> >> >
> >> > Also remove other tmp files used by the tests and the harness so
> >> > that we leave no new tmp files in /tmp dir after a full test run.
> >> >
> >>
> >> Why exactly can't the boilerplate go in a preamble file which is sourced by all
> >> the tests?  For example common/preamble containing something like:
> >
> > Thanks for your review! As you two are basically suggesting the same
> > solution, so I reply to you together.
> >
> > I agree that we can do a common/preamble for all tests to source, but I
> > don't think we need to do that and it's not worth the effort.
> >
> > IMO, we define and setup the tests in the tests themselves, not in a
> > common/preamble file, which makes the tests clear and easy to read. We
> > don't want to "hide" any setups in a common file and to be a "surprise"
> > to the reader, we don't have to look around for a certain setup.
> >
> 
> The motivation for your work is the fact that tmp files are being created
> in a way that is "hidden" from the test author  and therefore, IMO,
> the cleanup of those temp files should also be in a common function
> that is "hidden" from the test author.

[Sorry, I got back to this late.]

That's a good point, so I think what should be done is let common
functions clean up their tmp files, so there's no hidden usage of $tmp
and tests only need to remove $tmp.* used explicitly in the tests.

I checked all the common functions, there're only two that are not
cleaning up their tmp files properly, _do() and run_fsx().

I'll send v2 patch for review, which will be a much simpler patch.

Thanks,
Eryu