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

[Niklas Söderlund]

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 :-)

> 
> > >  		return ret;
> > > 
> > > -	return run();
> > > +	ret = run();
> > > +
> > > +	cleanup();
> > > +
> > > +	return ret;
> > >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas Söderlund