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

[James Hawkins]

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.

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 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.

-- 
James Hawkins