[libcamera-devel] [PATCH 3/3] test: v4l2_videodevice: buffer_cache: Fail the test if no buffer from cache is obtained

[Laurent Pinchart]

Hi Kieran,

On Wed, Apr 15, 2020 at 01:56:05PM +0100, Kieran Bingham wrote:
> On 15/04/2020 04:09, Umang Jain wrote:
> > On Wed, Apr 15, 2020 at 00:56, Laurent Pinchart wrote:
> >> Hi Umang, Thank you for the patch. On Tue, Apr 14, 2020 at 07:06:59AM
> >> +0000, Umang Jain wrote:
> >>
> >> Failing the test guards against negative index (-ENOENT in this
> >> case) being passed to V4L2BufferCache::put(). 
> >>
> >> Can this happens in practice ?
> 
> This is code which /tests/ library code.
> 
> Testing things that shouldn't necessarily happen in practice is part of
> that right? So that when someone changes V4L2BufferCache and breaks
> something - the tests should fire a failure?

But is this particular test testing that, or is it already caught by a
different test ? Each of the test cases cover a part of the API, I was
wondering if for this particular test such a check was needed. The
answer may be yes, I haven't read the code.

> In other words, shouldn't test be written defensively?
> 
> I don't think this can happen in practice with the *current*
> implementation, but that's the point - the implementation could change.
> 
> If we didn't ever expect to change implementations - then we don't need
> to keep any tests around at all :-)
> 
> > Well, V4L2BufferCache::put() accepts only unsigned int argument, so I
> > would say, it won't happen in practice.
> > Still the patches seems correct to me but commit message should be
> > tweaked, is that right?
> 
> So in this instance, the ASSERT() will fire, and bail out on
> V4L2BufferCache::put() ... but the unexpected fault (which should have
> failed the test) is in the get() ... ?
> 
> My point is - I would add this check in ..
> Laurent - are you suggesting we shouldn't?

-- 
Regards,

Laurent Pinchart