Re: [1/2] gdi32: Added PolyDraw tests to tests/path.c [try2]

[Koshelev, Misha Vladislavo]

On 6/27/07, Evan Stade <estade at gmail.com> wrote:
> Hi,
>
> [try2] The patch I sent yesterday was not properly todo_wined.  Also,
> this test is a bit more extensive (about twice as many points drawn).
> It uses various point-type combination (even non-sensical ones such as
> PT_LINETO | PT_MOVETO) to test the exact logic of PolyDraw with an
> open path.
>
> changelog:
> * added polydraw test to path testing (5 more todo_wines)
>
> dlls/gdi32/tests/path.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 70 insertions(+), 0 deletions(-)
>
> --
> Evan Stade

Also, I'd add a PT_MOVETO into your first PolyDraw call so you can tell whether
PT_CLOSEFIGURE adds a PT_MOVETO to orig_pos or lastmove, as the current version
does not differentiate this.

Misha

[1/2] gdi32: Added PolyDraw tests to tests/path.c [try2]

[Evan Stade]

On 6/29/07, Koshelev, Misha Vladislavo <mk144210(a)bcm.tmc.edu> wrote:
> On 6/27/07, Evan Stade <estade at gmail.com> wrote:
> > Hi,
> >
> > [try2] The patch I sent yesterday was not properly todo_wined.  Also,
> > this test is a bit more extensive (about twice as many points drawn).
> > It uses various point-type combination (even non-sensical ones such as
> > PT_LINETO | PT_MOVETO) to test the exact logic of PolyDraw with an
> > open path.
> >
> > changelog:
> > * added polydraw test to path testing (5 more todo_wines)
> >
> > dlls/gdi32/tests/path.c |   70
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 70 insertions(+), 0 deletions(-)
> >
> > --
> > Evan Stade
>
> Also, I'd add a PT_MOVETO into your first PolyDraw call so you can tell
> whether
>  PT_CLOSEFIGURE adds a PT_MOVETO to orig_pos or lastmove, as the current
> version
>  does not differentiate this.
>
> Misha

Thanks a lot for your help.

Your shorter diff may make the tests conform, but it incorrectly
assigns lastmove.x = dc->CursPosX.  This only makes it conform to the
tests because the tests happen to have a MoveToEx as the last path
point before the PolyDraw is called.  Also, your diff doesn't
correctly check the point types. The test if( lpbTypes[i] ==
PT_MOVETO) (which was there before your changes) is wrong because it
returns false for PT_MOVETO | PT_LINETO, when it should return true
(according to my testing which was not a part of the test I
submitted).  Also, your diff does not MoveToEx to orig_pos at the
second "return FALSE".  This is incorrect but again does not show up
in the tests.

So while a small and simple diff can make these specific tests
conform, it is not correct.  However I still don't understand how this
influences whether the test patch got accepted.  Your explanation of
why the fix patch didn't get accepted on the other hand does make
sense.

-- 
Evan Stade

[1/2] gdi32: Added PolyDraw tests to tests/path.c [try2]

[Misha Koshelev]

On Fri, 2007-06-29 at 14:49 -0700, Evan Stade wrote:
> Thanks a lot for your help.
> No prob.
> Your shorter diff may make the tests conform, but it incorrectly
> assigns lastmove.x = dc->CursPosX.  This only makes it conform to the
> tests because the tests happen to have a MoveToEx as the last path
> point before the PolyDraw is called.

You should include this in your tests, e.g., by calling LineTo after the
MoveToEx and before the first PolyDraw. This immediately makes it clear
why you need to have separate path and non-path cases. (I personally
think about conformance tests like this - you worked hard to figure out
how the function works on native and implemented your own version, but
someone might commit a patch sometime in the future to fix something
else that screws up all your hard work; that's why you need to put this
knowledge into conformance tests, so once that feature is implemented in
wine it will never break again).
> Also, your diff doesn't
> correctly check the point types. The test if( lpbTypes[i] ==
> PT_MOVETO) (which was there before your changes) is wrong because it
> returns false for PT_MOVETO | PT_LINETO, when it should return true
> (according to my testing which was not a part of the test I
> submitted).  Also, your diff does not MoveToEx to orig_pos at the
> second "return FALSE".  This is incorrect but again does not show up
> in the tests.
> Again, these are things that should ideally be a part of your
> conformance test. That would make the rationale for your extensive
> changes in your second patch clearer and would improve the likelihood of
> the patches going in.
> So while a small and simple diff can make these specific tests
> conform, it is not correct.  However I still don't understand how this
> influences whether the test patch got accepted.  Your explanation of
> why the fix patch didn't get accepted on the other hand does make
> sense.

I am not sure why the test patch was not accepted (unless maybe
Alexandre wants both patches ready to go before committing either, as if
you make any changes to the second one you might find you need to add
something to the tests... don't know), but having the whole patchset
complete and good to go certainly wouldn't hurt either patch.

Misha