On Sat, Jul 12, 2008 at 12:37 PM, Nikolay Sivov bunglehead@gmail.com wrote:
James Hawkins wrote:
On Sat, Jul 12, 2008 at 11:56 AM, Nikolay Sivov bunglehead@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.