[PATCH v2] gdi32/tests: MoveToEx() does not return the old point.
This behavior goes back to at least Windows XP. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: Added the todo_wine since it does not match the Windows behavior. An alternative would be to mark the Windows behavior as broken. There's a good argument to be made about it since it does not match the documentation. But then the documentation is known to often be wrong and Windows' behavior has not wavered since Windows XP so it's not like this behavior is a bug that will be fixed. This is why I opted for documenting the Windows behavior and showing that Wine diverges. dlls/gdi32/tests/metafile.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/dlls/gdi32/tests/metafile.c b/dlls/gdi32/tests/metafile.c index 6adafa37b82..3d0e45e10a6 100644 --- a/dlls/gdi32/tests/metafile.c +++ b/dlls/gdi32/tests/metafile.c @@ -2262,16 +2262,15 @@ static void test_mf_Graphics(void) ok( ret, "MoveToEx error %d.\n", GetLastError()); ret = LineTo(hdcMetafile, 2, 2); ok( ret, "LineTo error %d.\n", GetLastError()); + oldpoint.x = oldpoint.y = 11235; ret = MoveToEx(hdcMetafile, 1, 1, &oldpoint); ok( ret, "MoveToEx error %d.\n", GetLastError()); - -/* oldpoint gets garbage under Win XP, so the following test would - * work under Wine but fails under Windows: - * - * ok((oldpoint.x == 2) && (oldpoint.y == 2), - * "MoveToEx: (x, y) = (%ld, %ld), should be (2, 2).\n", - * oldpoint.x, oldpoint.y); - */ + /* Windows 9x and older may have returned oldpoint as per the + * documentation. But Windows >= XP does not. + */ + todo_wine ok(broken(oldpoint.x == 11235 && oldpoint.y == 11235), + "MoveToEx: oldpoint = (%d, %d), should be (11235, 11235).\n", + oldpoint.x, oldpoint.y); ret = Ellipse(hdcMetafile, 0, 0, 2, 2); ok( ret, "Ellipse error %d.\n", GetLastError()); -- 2.20.1
On Thu, Nov 21, 2019 at 05:22:29PM +0100, Francois Gouget wrote:
This behavior goes back to at least Windows XP.
Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> ---
v2: Added the todo_wine since it does not match the Windows behavior. An alternative would be to mark the Windows behavior as broken. There's a good argument to be made about it since it does not match the documentation. But then the documentation is known to often be wrong and Windows' behavior has not wavered since Windows XP so it's not like this behavior is a bug that will be fixed. This is why I opted for documenting the Windows behavior and showing that Wine diverges.
dlls/gdi32/tests/metafile.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/dlls/gdi32/tests/metafile.c b/dlls/gdi32/tests/metafile.c index 6adafa37b82..3d0e45e10a6 100644 --- a/dlls/gdi32/tests/metafile.c +++ b/dlls/gdi32/tests/metafile.c @@ -2262,16 +2262,15 @@ static void test_mf_Graphics(void) ok( ret, "MoveToEx error %d.\n", GetLastError()); ret = LineTo(hdcMetafile, 2, 2); ok( ret, "LineTo error %d.\n", GetLastError()); + oldpoint.x = oldpoint.y = 11235; ret = MoveToEx(hdcMetafile, 1, 1, &oldpoint); ok( ret, "MoveToEx error %d.\n", GetLastError()); - -/* oldpoint gets garbage under Win XP, so the following test would - * work under Wine but fails under Windows: - * - * ok((oldpoint.x == 2) && (oldpoint.y == 2), - * "MoveToEx: (x, y) = (%ld, %ld), should be (2, 2).\n", - * oldpoint.x, oldpoint.y); - */ + /* Windows 9x and older may have returned oldpoint as per the + * documentation. But Windows >= XP does not. + */ + todo_wine ok(broken(oldpoint.x == 11235 && oldpoint.y == 11235), + "MoveToEx: oldpoint = (%d, %d), should be (11235, 11235).\n", + oldpoint.x, oldpoint.y);
ret = Ellipse(hdcMetafile, 0, 0, 2, 2); ok( ret, "Ellipse error %d.\n", GetLastError());
I don't think this makes much sense - the condition won't be tested under Wine because of the broken(). It seems unlikely an app will ever depend on this, so leaving the original comment there is probably the right thing to do. Huw.
On Mon, 25 Nov 2019, Huw Davies wrote: [...]
@@ -2262,16 +2262,15 @@ static void test_mf_Graphics(void) ok( ret, "MoveToEx error %d.\n", GetLastError()); ret = LineTo(hdcMetafile, 2, 2); ok( ret, "LineTo error %d.\n", GetLastError()); + oldpoint.x = oldpoint.y = 11235; ret = MoveToEx(hdcMetafile, 1, 1, &oldpoint); ok( ret, "MoveToEx error %d.\n", GetLastError()); - -/* oldpoint gets garbage under Win XP, so the following test would - * work under Wine but fails under Windows: - * - * ok((oldpoint.x == 2) && (oldpoint.y == 2), - * "MoveToEx: (x, y) = (%ld, %ld), should be (2, 2).\n", - * oldpoint.x, oldpoint.y); - */ + /* Windows 9x and older may have returned oldpoint as per the + * documentation. But Windows >= XP does not. + */ + todo_wine ok(broken(oldpoint.x == 11235 && oldpoint.y == 11235), + "MoveToEx: oldpoint = (%d, %d), should be (11235, 11235).\n", + oldpoint.x, oldpoint.y);
ret = Ellipse(hdcMetafile, 0, 0, 2, 2); ok( ret, "Ellipse error %d.\n", GetLastError());
I don't think this makes much sense - the condition won't be tested under Wine because of the broken().
Actually broken() only returns true on Windows (my commit message was misleading if not flat out wrong on that count). On Wine it always returns false. This is why the todo_wine is needed.
It seems unlikely an app will ever depend on this, so leaving the original comment there is probably the right thing to do.
I really don't like commented out code. * It bitrots easily. * One has to run the code to get any information out of it, and that supposes you have the right Windows version available. For instance it's pretty hard to know how Windows 2000 behaves now. A simple non-code comment may have captured that information back in the day. * The non-code part of this particular comment is wrong: Windows does not return garbage; it leaves the point unmodified. Windows apps are one case where this could be an issue. The other is running a Wine application or dll that depends on this behavior on Windows. Fortunately none of the Wine programs depends on this and for the dlls it's just comdlg32, riched20, user32 and uxtheme (I'd say unlikely for each). So I can understand not wanting to change the behavior of Wine and in that case having a todo_wine we don't intend to fix is a problem. So then I would propose this: ok((oldpoint.x == 2 && oldpoint.y == 2) || broken(oldpoint.x == 11235 && oldpoint.y == 11235), "MoveToEx: oldpoint = (%d, %d), should be (2, 2).\n", oldpoint.x, oldpoint.y); I'll resubmit with this option fleshed out. -- Francois Gouget <fgouget(a)codeweavers.com>
On Wed, Nov 27, 2019 at 07:55:28PM +0100, Francois Gouget wrote:
On Mon, 25 Nov 2019, Huw Davies wrote: [...]
@@ -2262,16 +2262,15 @@ static void test_mf_Graphics(void) ok( ret, "MoveToEx error %d.\n", GetLastError()); ret = LineTo(hdcMetafile, 2, 2); ok( ret, "LineTo error %d.\n", GetLastError()); + oldpoint.x = oldpoint.y = 11235; ret = MoveToEx(hdcMetafile, 1, 1, &oldpoint); ok( ret, "MoveToEx error %d.\n", GetLastError()); - -/* oldpoint gets garbage under Win XP, so the following test would - * work under Wine but fails under Windows: - * - * ok((oldpoint.x == 2) && (oldpoint.y == 2), - * "MoveToEx: (x, y) = (%ld, %ld), should be (2, 2).\n", - * oldpoint.x, oldpoint.y); - */ + /* Windows 9x and older may have returned oldpoint as per the + * documentation. But Windows >= XP does not. + */ + todo_wine ok(broken(oldpoint.x == 11235 && oldpoint.y == 11235), + "MoveToEx: oldpoint = (%d, %d), should be (11235, 11235).\n", + oldpoint.x, oldpoint.y);
ret = Ellipse(hdcMetafile, 0, 0, 2, 2); ok( ret, "Ellipse error %d.\n", GetLastError());
I don't think this makes much sense - the condition won't be tested under Wine because of the broken().
Actually broken() only returns true on Windows (my commit message was misleading if not flat out wrong on that count). On Wine it always returns false. This is why the todo_wine is needed.
Exactly, that was my point, broken() immediately returns 0 on Wine and the condition is not evaluated.
So I can understand not wanting to change the behavior of Wine and in that case having a todo_wine we don't intend to fix is a problem. So then I would propose this:
ok((oldpoint.x == 2 && oldpoint.y == 2) || broken(oldpoint.x == 11235 && oldpoint.y == 11235), "MoveToEx: oldpoint = (%d, %d), should be (2, 2).\n", oldpoint.x, oldpoint.y);
I'll resubmit with this option fleshed out.
This would be ok. Huw.
participants (2)
-
Francois Gouget -
Huw Davies