[2/3] gdiplus: fix GdipPathIterNextMarker behaviour on path without markers. fix tests.

[Nikolay Sivov]

James Hawkins wrote:
> On Sat, Jul 12, 2008 at 12:37 PM, Nikolay Sivov <bunglehead at gmail.com> wrote:
>   
>> James Hawkins wrote:
>>     
>>> On Sat, Jul 12, 2008 at 11:56 AM, Nikolay Sivov <bunglehead at gmail.com>
>>> wrote:
>>>
>>>       
>>>> Changelog:
>>>>   -  Fix GdipPathIterNextMarker behaviour on path without markers. Make
>>>> tests pass on native.
>>>>
>>>> ---
>>>>  dlls/gdiplus/pathiterator.c       |    3 ++-
>>>>  dlls/gdiplus/tests/pathiterator.c |    6 +++---
>>>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/dlls/gdiplus/pathiterator.c b/dlls/gdiplus/pathiterator.c
>>>> index 55b0782..3d3b1dc 100644
>>>> --- a/dlls/gdiplus/pathiterator.c
>>>> +++ b/dlls/gdiplus/pathiterator.c
>>>> @@ -140,7 +140,8 @@ GpStatus WINGDIPAPI
>>>> GdipPathIterNextMarker(GpPathIterator* iterator, INT *result
>>>>    /* first call could start with second point as all subsequent, cause
>>>>       path couldn't contain only one */
>>>>    for(i = iterator->marker_pos + 1; i < iterator->pathdata.Count; i++){
>>>> -        if(iterator->pathdata.Types[i] & PathPointTypePathMarker){
>>>> +        if((iterator->pathdata.Types[i] & PathPointTypePathMarker) ||
>>>> +           (i == iterator->pathdata.Count - 1)){
>>>>            *startIndex = iterator->marker_pos;
>>>>            if(iterator->marker_pos > 0) (*startIndex)++;
>>>>            *endIndex   = iterator->marker_pos = i;
>>>> diff --git a/dlls/gdiplus/tests/pathiterator.c
>>>> b/dlls/gdiplus/tests/pathiterator.c
>>>> index 071c1d5..6498bb3 100644
>>>> --- a/dlls/gdiplus/tests/pathiterator.c
>>>> +++ b/dlls/gdiplus/tests/pathiterator.c
>>>> @@ -114,7 +114,7 @@ static void test_nextmarker(void)
>>>>    GdipCreatePathIter(&iter, path);
>>>>    stat = GdipPathIterNextMarker(iter, &result, &start, &end);
>>>>    expect(Ok, stat);
>>>> -    expect(0, result);
>>>> +    if(stat == Ok) expect(TRUE, (result == 4) && (start == 0) && (end ==
>>>> 3));
>>>>
>>>>         
>>> Why are you checking if stat == Ok?  You're linking what should be
>>> separate tests together.  You also need to put each of these checks
>>> into separate tests.  If the test fails, you have no idea (without
>>> debugging further) which one of those checks fails.  They're called
>>> unit tests for a reason.
>>>
>>>       
>> I don't think so. If the call fails testing return value doesn't make any
>> sense. In this case 'result' is uninitialized and remains uninitialized
>> after a call if it fails (here I mean a native too). So why should we check
>> something which has unpredictable value?
>> By the way first time I saw this here
>> 8fd6f0e26ae28f2ba4938e2fbcc4851f47baa53c..
>>
>>     
>
> An API defines a unique output* for the set of all possible inputs.
> When you write unit tests, you verify that the implementation of the
> API conforms to the definition.  In the case of Wine, our definition
> of the Win32 API is the test results from running the Wine test suite
> in Windows.  While msdn is a good reference, it's not always correct
> and we can't test our implementation against it.
>   
If you mean that an API's defined by a mapping of all possible inputs to 
a set of outputs I'm agreed.
> In all the cases where you've added checks for (stat != Ok), stat does
> equal Ok in Windows.  Thus, the definition of the API for those inputs
> is that the functions return Ok.  If, for a hypothetical example, one
> version of gdiplus did not return Ok, we would probably mark that as
> broken.  That leads me to my next point.
>
> You say you added the checks because it's wrong to check the value of
> an uninitialized variable.  You are correct in that it is wrong to
> check the value of an uninitialized variable.  Where you are mistaken
> is that, by the definition of this API, none of these out variables
> will be uninitialized in these cases.  One area where your tests are
> lacking is that you don't control all of the input.  Even in the
> failure case, you should initialize all out variables to some magic
> value and then check the values of those parameters after the failed
> call.  Either the values were unchanged, and thus still hold the magic
> value, or they are set to some other value.  One bonus of this
> practice is that, even if the test fails in Wine when it should
> succeed, you're guaranteed to never read the value of an uninitialized
> variable.
>   
So you mean here that in this case testing return value should be 
unlinked to testing outputs, that these tests are  independent. Maybe it 
really helps when behaviour changes (API changes in fact) from version 
to version. So maybe it's the better way to split this and let it both 
test fail.
> So to answer your question, there should be no unpredictable values in
> a well-written test.
>
> * or a unique set of acceptable outputs, e.g., multiple GetLastError values.
Yes, I thought about some special 'magic' values for output arguments. 
Some deadbeef should be acceptable.
Thanks for comments I'll repost these fixes.