Re: gdiplus: Add test cases for GdipDrawCurve [corrected]

[Andrew Eikum]

Nikolay Sivov wrote:
> Andrew Eikum wrote:
> > Nikolay Sivov wrote:
> > > Paul Vriens wrote:
> > > > Andrew Eikum wrote:
> > > > > This patch was submitted back on Tuesday and I haven't received a 
> > > > > response one way or the other about it.  Does anyone see anything 
> > > > > immediately wrong with it?
> > > > >
> > > > > I more-or-less copied the functionality of test_GdipDrawBezier 
> > > > > right above, testing each of the different input possibilities for 
> > > > > correctness.  It passes 100% on WinXP SP3 and Win7 RC1, although 
> > > > > there are failures in Wine's GdipDrawCurve implementation.
> > > > >
> > > > > Thanks for taking a look,
> > > > > Andrew
> > > >
> > > > Hi Andrew,
> > > >
> > > > Test crashes on my box:
> > > >
> > > > Not sure if that was the reason for not being committed though as 
> > > > the tests could run fine on AJ's magic box of course.
> > > - /* InvalidParameter cases: invalid count */
> > > - status = GdipDrawCurve(graphics, pen, points, -1);
> > > - expect(InvalidParameter, status);
> > > -
> > > - status = GdipDrawCurve(graphics, pen, points, 0);
> > > - expect(InvalidParameter, status);
> > >
> > > This could be a problem. Count isn't checked on allocation in 
> > > GdipDrawCurve2(), and allocated buffer isn't checked for NULL too.
> >
> > Well, even if the tests cause it to crash, doesn't that make this a 
> > problem with GdipDrawCurve2 and not the tests patch?  The tests do 
> > not crash on Windows, which means there's nothing wrong with the 
> > tests themselves.  If tests are not ever supposed to crash Wine like 
> > this, wouldn't it be more appropriate to make a test harness that 
> > catches all exceptions and reports as failures?  I'm just not seeing 
> > how this crash is the _tests'_ fault.
>
> Sure, it isn't a test problem, it's a patch problem. Any patch (or 
> series) shouldn't break tests.
> > In any case, the problem is as you described.  The return of 
> > GdipAlloc isn't checked, causing a crash when it tries to access the 
> > structures it expects to be there.  Would these tests be accepted if 
> > there was an accompanying patch to fix this defect, eliminating the 
> > crash?
>
> Exactly, you should patch code too, not only tests. In this particular 
> case it's better to check count first and return InvalidParameter and 
> check
> for GdipAlloc result and return OutOfMemory if it's NULL.

Okay, excellent.  I'll make the patch for GdipDrawCurve2 and send that 
off.  Would it be a good idea to re-send the tests patch, or just refer 
to it in the patch email?

Thank you!
Andrew