[libcamera-devel] [PATCH] tests: call the derived Test class cleanup() function

[Jacopo Mondi]

Hi Laurent, Niklas,

On Sat, Dec 22, 2018 at 03:44:10PM +0100, Niklas S?derlund wrote:
> Hi Jacopo, Laurent,
>
> On 2018-12-22 12:34:22 +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Friday, 21 December 2018 18:26:42 EET jacopo mondi wrote:
> > > On Fri, Dec 21, 2018 at 01:53:29AM +0100, Niklas Söderlund wrote:
> > > > Calling the cleanup() function in the base class Test destructor only
> > > > calls the base class empty cleanup() function, not the overloaded one.
> > > > This results in tests not cleaning up after themself. Solve this by
> > > > explicitly calling the cleanup() function from execute().
> > > >
> > > > This was discovered while running valgrind on tests where objects where
> > > > allocated in init() and freed in cleanup().
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > ---
> > > >
> > > >  test/test.cpp | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/test/test.cpp b/test/test.cpp
> > > > index 4e7779e750d56687..1bb6ebcb9e8acf18 100644
> > > > --- a/test/test.cpp
> > > > +++ b/test/test.cpp
> > > > @@ -13,7 +13,6 @@ Test::Test()
> > > >  Test::~Test()
> > > >  {
> > > > -	cleanup();
> > > >  }
> > > >
> > > >  int Test::execute()
> > > > @@ -24,5 +23,9 @@ int Test::execute()
> > > >  	if (ret < 0)
> > >
> > > Shouldn't the same be done here?
> >
> > It's a good question. Shouldn't the init() method clean after itself if it
> > fails ? What's your opinion on this generally ?
>
> My view is that in a perfect world cleanup() should be called here and
> each test cases implementation of cleanup() should be prepared to handle
> partially initialized state. Even better in a super perfect world init()
> and run() would never fail so there would be no need to check the return
> codes or write the test to begin with as it would never fail anyhow ;-P
>
> As we live in an in-perfect world I see little point in calling or not
> calling cleanup() here, just bail out and make sure it's clear the test
> failed.  If the state created by a half run init()/cealnup() function
> crashes my machine it's a inconvenience but it would probably motivate
> me to find and fix the issue quicker :-)
>

I see, but as much as I would like to have init() to cleanup after
itself, that should might not happen.

What if the base class provided macro TEST_REGISTER does that in case
of both success and failures?

#define TEST_REGISTER(klass)						\
int main(int argc, char *argv[])					\
{									\
	int ret = klass().execute();					\
        cleanup();                                                      \
                                                                        \
        return ret;                                                     \
}

Thanks
   j


> >
> > > >  		return ret;
> > > >
> > > > -	return run();
> > > > +	ret = run();
> > > > +
> > > > +	cleanup();
> > > > +
> > > > +	return ret;
> > > >  }
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >
> >
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181223/cc950d1d/attachment.sig>