[PATCH 03/12] test: object-delete: Test deferred delete just before thread stops

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 10:50:51AM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> > On Mon, Jan 22, 2024 at 08:22:10PM +0100, Milan Zamazal wrote:
> >> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> >> 
> >> > The Object::deleterLater() function is expected to not race with
> >>                    ^^^
> >> deleteLater
> >> 
> >> > stopping the thread the object is bound to. Add a test for this.
> >> >
> >> > The test currently fails, demonstrating a bug in libcamera.
> >> 
> >> Shouldn't it then be marked as an expected failure?
> >
> > I don't think so. should_fail in meson indicates that the test
> > executable returning an error (including terminating due to an uncaught
> > signal, such as SIGSEGV) indicates success. In this case, the test
> > should pass, but a there's bug in libcamera that makes it fail. Once the
> > bug gets fixed, the test will then pass.
> 
> My point was whether it is OK to have a commit with a failing test, whether it
> can disturb anything.  Right, marking it as an expected failure would be wrong,
> but maybe disabling it's automatic run and enabling it in the fix commit would
> be a better idea?

At the moment I'm not aware of any workflow that would be broken by
this. We "break" tests regularly in this way when we find a bug, first
adding a unit test that fails, and then fixing it on top, in the same
patch series. It's important to see the test failing otherwise you can't
be sure the fix is right, it could be that the test is incorrectly
implemented.

> >> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Anyway, I don't have anything else to add, so:
> 
> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
> 
> >> > ---
> >> >  test/object-delete.cpp | 30 +++++++++++++++++++++++++-----
> >> >  1 file changed, 25 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/test/object-delete.cpp b/test/object-delete.cpp
> >> > index eabefe935974..80b7dc41cd37 100644
> >> > --- a/test/object-delete.cpp
> >> > +++ b/test/object-delete.cpp
> >> > @@ -33,10 +33,10 @@ public:
> >> >  	unsigned int *deleteCount_;
> >> >  };
> >> >  
> >> > -class NewThread : public Thread
> >> > +class DeleterThread : public Thread
> >> >  {
> >> >  public:
> >> > -	NewThread(Object *obj)
> >> > +	DeleterThread(Object *obj)
> >> >  		: object_(obj)
> >> >  	{
> >> >  	}
> >> > @@ -63,9 +63,9 @@ protected:
> >> >  		unsigned int count = 0;
> >> >  		TestObject *obj = new TestObject(&count);
> >> >  
> >> > -		NewThread thread(obj);
> >> > -		thread.start();
> >> > -		thread.wait();
> >> > +		DeleterThread delThread(obj);
> >> > +		delThread.start();
> >> > +		delThread.wait();
> >> >  
> >> >  		Thread::current()->dispatchMessages(Message::Type::DeferredDelete);
> >> >  
> >> > @@ -89,6 +89,26 @@ protected:
> >> >  			return TestFail;
> >> >  		}
> >> >  
> >> > +		/*
> >> > +		 * Test that deleteLater() works properly when called just
> >> > +		 * before the object's thread exits.
> >> > +		 */
> >> > +		Thread boundThread;
> >> > +		boundThread.start();
> >> > +
> >> > +		count = 0;
> >> > +		obj = new TestObject(&count);
> >> > +		obj->moveToThread(&boundThread);
> >> > +
> >> > +		obj->deleteLater();
> >> > +		boundThread.exit();
> >> > +		boundThread.wait();
> >> > +
> >> > +		if (count != 1) {
> >> > +			cout << "Object deletion right before thread exit failed (" << count << ")" << endl;
> >> > +			return TestFail;
> >> > +		}
> >> > +
> >> >  		return TestPass;
> >> >  	}
> >> >  };

-- 
Regards,

Laurent Pinchart