June 2022 - Wine-Devel - list.winehq.org

Re: [PATCH v2 0/5] MR169: user32/tests: Cleanup some tests.

by Stefan Dösinger

On Wed Jun 1 18:31:09 2022 +0000, Rémi Bernon wrote: > Sure, `SetForegroundWindow` doesn't necessarily succeeds on windows, but > in the case of the tests and how they are run on windows, it is not > supposed to fail. > If it fails it means the tests will fail anyway. There have been cases > like this in testbot runs, for instance when there's an unexpected > topmost window (for instance the firewall warning), and it should be > fixed by fixing the environment, not by skipping tests. > On Linux there's some other issues, but they are mitigated by the > `flush_events` calls which makes sure X11 events stabilize in the state > we expect windows to be. It is not ideal, and Wine `user32` and > `winex11` are still racy w.r.t focus and foreground, but that's > something that also need to be fixed, not ignored. > > it should be fixed by fixing the environment, not by skipping tests. I'm not sure I agree. The testbot isn't the only environment where people possibly run the tests, and someone new to the entire user32 thing might find environment-induced test failures a bug and not a feature. That said, it would be sensible to apply different rules to d3d and user32. In d3d, handling focus loss/gain is only a relatively small part, whereas it is pretty central to user32. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/169#note_1472

June 1, 2022

Re: RFC: Allow marking unreliable tests as flaky.

by Esme Povirk (she/they)

It might be better to use something like broken() to encourage more precision in marking the failures. For example, if something fails with ACCESS_DENIED sometimes, we'd still like to know if it starts failing with FILE_NOT_FOUND. That does make it trickier to add flaky tests to the summary line, though. I guess you could set it up so that if flaky() is called with non-zero and the next ok() succeeds, it's marked as flaky. Having the failures in the summary line also opens up the option of handling them by retrying, and can be implemented without making that decision first. Of course, retrying leaves open the possibility that you will be unlucky and all the retries will also fail. Whether that's worth it would probably depend on how often a regression on a flaky test slips through and causes the tests to fail for Alexandre.

June 1, 2022

Re: [PATCH v2 0/5] MR169: user32/tests: Cleanup some tests.

by Rémi Bernon

On Wed Jun 1 18:31:09 2022 +0000, Stefan Dösinger wrote: > Afaiu SetForegroundWindow can fail on Windows if the terminal that is > running the tests wasn't the foreground window when the test was > started. I.e. it is some sort of focus stealing prevention. > In the d3d tests we have a few similar tests. Those are mostly motivated > by Linux WMs refusing to give us focus, not so much on Windows. There is > also the case of fvwm2 or other WMs with a focus-follows-mouse setting > that will make SetForegroundWindow not do what you want it to do. Sure, `SetForegroundWindow` doesn't necessarily succeeds on windows, but in the case of the tests and how they are run on windows, it is not supposed to fail. If it fails it means the tests will fail anyway. There have been cases like this in testbot runs, for instance when there's an unexpected topmost window (for instance the firewall warning), and it should be fixed by fixing the environment, not by skipping tests. On Linux there's some other issues, but they are mitigated by the `flush_events` calls which makes sure X11 events stabilize in the state we expect windows to be. It is not ideal, and Wine `user32` and `winex11` are still racy w.r.t focus and foreground, but that's something that also need to be fixed, not ignored. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/169#note_1471

June 1, 2022

Re: [PATCH v2 0/5] MR169: user32/tests: Cleanup some tests.

by Stefan Dösinger

Afaiu SetForegroundWindow can fail on Windows if the terminal that is running the tests wasn't the foreground window when the test was started. I.e. it is some sort of focus stealing prevention. In the d3d tests we have a few similar tests. Those are mostly motivated by Linux WMs refusing to give us focus, not so much on Windows. There is also the case of fvwm2 or other WMs with a focus-follows-mouse setting that will make SetForegroundWindow not do what you want it to do. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/169#note_1470

June 1, 2022

Re: [PATCH v2 0/5] MR169: user32/tests: Cleanup some tests.

by Rémi Bernon

v2: Check `SetForegroundWindow` return values. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/169#note_1469

June 1, 2022

[PATCH v2 5/5] user32/tests: Process messages while waiting in WindowFromPoint tests.

by Rémi Bernon

From: Rémi Bernon <rbernon(a)codeweavers.com> The test is randomly failing, processing messages while waiting for events and cleaning up child windows in the other process should fix the issue. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/win.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 0f07e42ae0d..2d9ff7cbc45 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -10211,21 +10211,29 @@ static void window_from_point_proc(HWND parent) flush_events(TRUE); SetEvent(start_event); - got_hittest = FALSE; got_click = FALSE; - while(!got_click && wait_for_message(&msg)) { - if(msg.message == WM_LBUTTONUP) { - ok(msg.hwnd == win, "msg.hwnd = %p, expected %p\n", msg.hwnd, win); - got_click = TRUE; + got_hittest = FALSE; + while ((ret = MsgWaitForMultipleObjects( 1, &end_event, FALSE, INFINITE, QS_ALLINPUT )) <= 1) + { + while (PeekMessageA( &msg, 0, 0, 0, PM_REMOVE )) + { + if (msg.message == WM_LBUTTONUP) + { + ok(msg.hwnd == win, "msg.hwnd = %p, expected %p\n", msg.hwnd, win); + got_click = TRUE; + } + TranslateMessage( &msg ); + DispatchMessageA( &msg ); } - DispatchMessageA(&msg); + if (ret == 0) break; } + ok(ret == 0, "MsgWaitForMultipleObjects returned %#lx\n", ret); ok(got_hittest, "transparent window didn't get WM_NCHITTEST message\n"); ok(got_click, "button under static window didn't get WM_LBUTTONUP\n"); - ret = WaitForSingleObject(end_event, 5000); - ok(ret == WAIT_OBJECT_0, "WaitForSingleObject returned %lx\n", ret); - + DestroyWindow(win); + DestroyWindow(child_button); + DestroyWindow(child_static); CloseHandle(start_event); CloseHandle(end_event); } @@ -10245,14 +10253,10 @@ static void test_window_from_point(const char *argv0) pt.x = pt.y = 150; win = WindowFromPoint(pt); + ok(win == hwnd, "WindowFromPoint returned %p, expected %p\n", win, hwnd); pt.x = 250; - if(win == hwnd) - win = WindowFromPoint(pt); - if(win != hwnd) { - skip("there's another window covering test window\n"); - DestroyWindow(hwnd); - return; - } + win = WindowFromPoint(pt); + ok(win == hwnd, "WindowFromPoint returned %p, expected %p\n", win, hwnd); child = CreateWindowExA(0, "static", "static", WS_CHILD | WS_VISIBLE, 0, 0, 100, 100, hwnd, 0, NULL, NULL); @@ -10279,7 +10283,7 @@ static void test_window_from_point(const char *argv0) startup.cb = sizeof(startup); ok(CreateProcessA(NULL, cmd, NULL, NULL, FALSE, 0, NULL, NULL, &startup, &info), "CreateProcess failed.\n"); - ok(wait_for_events(1, &start_event, 1000) == 0, "didn't get start_event\n"); + wait_for_events(1, &start_event, INFINITE); child = GetWindow(hwnd, GW_CHILD); win = WindowFromPoint(pt); @@ -10294,6 +10298,8 @@ static void test_window_from_point(const char *argv0) ok(win == child, "WindowFromPoint returned %p, expected %p\n", win, child); SetEvent(end_event); + wait_for_events(1, &info.hProcess, INFINITE); + wait_child_process(info.hProcess); CloseHandle(start_event); CloseHandle(end_event); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022

[PATCH v2 4/5] user32/tests: Check for window class registration failure normally.

by Rémi Bernon

From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/win.c | 48 ++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index ee347fd3e70..0f07e42ae0d 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -1161,10 +1161,11 @@ static LRESULT WINAPI tool_window_procA(HWND hwnd, UINT msg, WPARAM wparam, LPAR static const WCHAR mainclassW[] = {'M','a','i','n','W','i','n','d','o','w','C','l','a','s','s','W',0}; -static BOOL RegisterWindowClasses(void) +static void RegisterWindowClasses(void) { WNDCLASSW clsW; WNDCLASSA cls; + BOOL ret; cls.style = CS_DBLCLKS; cls.lpfnWndProc = main_window_procA; @@ -1177,7 +1178,8 @@ static BOOL RegisterWindowClasses(void) cls.lpszMenuName = NULL; cls.lpszClassName = "MainWindowClass"; - if(!RegisterClassA(&cls)) return FALSE; + ret = RegisterClassA(&cls); + ok(ret, "RegisterClassA failed, error %lu\n", GetLastError()); clsW.style = CS_DBLCLKS; clsW.lpfnWndProc = main_window_procW; @@ -1190,7 +1192,8 @@ static BOOL RegisterWindowClasses(void) clsW.lpszMenuName = NULL; clsW.lpszClassName = mainclassW; - if(!RegisterClassW(&clsW)) return FALSE; + ret = RegisterClassW(&clsW); + ok(ret, "RegisterClassW failed, error %lu\n", GetLastError()); cls.style = 0; cls.lpfnWndProc = tool_window_procA; @@ -1203,9 +1206,8 @@ static BOOL RegisterWindowClasses(void) cls.lpszMenuName = NULL; cls.lpszClassName = "ToolWindowClass"; - if(!RegisterClassA(&cls)) return FALSE; - - return TRUE; + ret = RegisterClassA(&cls); + ok(ret, "RegisterClassA failed, error %lu\n", GetLastError()); } static void verify_window_info(const char *hook, HWND hwnd, const WINDOWINFO *info) @@ -2499,9 +2501,10 @@ static LRESULT WINAPI mdi_main_wnd_procA(HWND hwnd, UINT msg, WPARAM wparam, LPA return DefFrameProcA(hwnd, mdi_client, msg, wparam, lparam); } -static BOOL mdi_RegisterWindowClasses(void) +static void mdi_RegisterWindowClasses(void) { WNDCLASSA cls; + BOOL ret; cls.style = 0; cls.lpfnWndProc = mdi_main_wnd_procA; @@ -2513,17 +2516,18 @@ static BOOL mdi_RegisterWindowClasses(void) cls.hbrBackground = GetStockObject(WHITE_BRUSH); cls.lpszMenuName = NULL; cls.lpszClassName = "MDI_parent_Class"; - if(!RegisterClassA(&cls)) return FALSE; + ret = RegisterClassA(&cls); + ok(ret, "RegisterClassA failed, error %lu\n", GetLastError()); cls.lpfnWndProc = mdi_child_wnd_proc_1; cls.lpszClassName = "MDI_child_Class_1"; - if(!RegisterClassA(&cls)) return FALSE; + ret = RegisterClassA(&cls); + ok(ret, "RegisterClassA failed, error %lu\n", GetLastError()); cls.lpfnWndProc = mdi_child_wnd_proc_2; cls.lpszClassName = "MDI_child_Class_2"; - if(!RegisterClassA(&cls)) return FALSE; - - return TRUE; + ret = RegisterClassA(&cls); + ok(ret, "RegisterClassA failed, error %lu\n", GetLastError()); } static void test_mdi(void) @@ -2536,7 +2540,7 @@ static void test_mdi(void) MSG msg; HMENU frame_menu, child_menu; - if (!mdi_RegisterWindowClasses()) assert(0); + mdi_RegisterWindowClasses(); mdi_hwndMain = CreateWindowExA(0, "MDI_parent_Class", "MDI parent window", WS_CAPTION | WS_SYSMENU | WS_MINIMIZEBOX | @@ -5994,9 +5998,10 @@ static void test_AWRwindow(LPCSTR class, LONG style, LONG exStyle, BOOL menu) DestroyWindow(hwnd); } -static BOOL AWR_init(void) +static void AWR_init(void) { WNDCLASSA class; + BOOL ret; class.style = CS_HREDRAW | CS_VREDRAW; class.lpfnWndProc = DefWindowProcA; @@ -6009,18 +6014,12 @@ static BOOL AWR_init(void) class.lpszMenuName = 0; class.lpszClassName = szAWRClass; - if (!RegisterClassA(&class)) { - ok(FALSE, "RegisterClass failed\n"); - return FALSE; - } + ret = RegisterClassA(&class); + ok(ret, "RegisterClassA failed, error %lu\n", GetLastError()); hmenu = CreateMenu(); - if (!hmenu) - return FALSE; ok(hmenu != 0, "Failed to create menu\n"); ok(AppendMenuA(hmenu, MF_STRING, 1, "Test!"), "Failed to create menu item\n"); - - return TRUE; } @@ -6087,8 +6086,7 @@ static void test_AWR_flags(void) static void test_AdjustWindowRect(void) { - if (!AWR_init()) - return; + AWR_init(); SHOWSYSMETRIC(SM_CYCAPTION); SHOWSYSMETRIC(SM_CYSMCAPTION); @@ -13017,7 +13015,7 @@ START_TEST(win) return; } - if (!RegisterWindowClasses()) assert(0); + RegisterWindowClasses(); hwndMain = CreateWindowExA(/*WS_EX_TOOLWINDOW*/ 0, "MainWindowClass", "Main window", WS_CAPTION | WS_SYSMENU | WS_MINIMIZEBOX | -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022

[PATCH v2 3/5] user32/tests: Remove SetForegroundWindow success checks.

by Rémi Bernon

From: Rémi Bernon <rbernon(a)codeweavers.com> It's not supposed to fail and skipping the tests simply creates false successful runs. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/win.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index c6667cafeff..ee347fd3e70 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -3489,13 +3489,11 @@ static void check_z_order_debug(HWND hwnd, HWND next, HWND prev, HWND owner, static void test_popup_zorder(HWND hwnd_D, HWND hwnd_E, DWORD style) { HWND hwnd_A, hwnd_B, hwnd_C, hwnd_F; + BOOL ret; /* Give current thread foreground state otherwise the tests may fail. */ - if (!SetForegroundWindow(hwnd_D)) - { - skip("SetForegroundWindow not working\n"); - return; - } + ret = SetForegroundWindow(hwnd_D); + ok(ret, "SetForegroundWindow failed, error %lu\n", GetLastError()); SetWindowPos(hwnd_E, hwnd_D, 0,0,0,0, SWP_NOSIZE|SWP_NOMOVE|SWP_NOACTIVATE); @@ -3955,11 +3953,8 @@ static void test_SetForegroundWindow(HWND hwnd) check_wnd_state(hwnd, hwnd, hwnd, 0); ret = SetForegroundWindow(hwnd); - if (!ret) - { - skip( "SetForegroundWindow not working\n" ); - return; - } + ok(ret, "SetForegroundWindow failed, error %lu\n", GetLastError()); + check_wnd_state(hwnd, hwnd, hwnd, 0); SetLastError(0xdeadbeef); @@ -11599,19 +11594,15 @@ static void reset_window_state(HWND *state, int n) static void test_topmost(void) { HWND owner, hwnd, hwnd2, hwnd_child, hwnd_child2, hwnd_grandchild, state[6] = { 0 }; - BOOL is_wine = !strcmp(winetest_platform, "wine"); + BOOL ret, is_wine = !strcmp(winetest_platform, "wine"); owner = create_tool_window(WS_CAPTION | WS_SYSMENU | WS_MINIMIZEBOX | WS_MAXIMIZEBOX | WS_POPUP | WS_VISIBLE, 0); ok(owner != 0, "Failed to create owner window (%ld)\n", GetLastError()); /* Give current thread foreground state otherwise the tests may fail. */ - if (!SetForegroundWindow(owner)) - { - DestroyWindow(owner); - skip("SetForegroundWindow not working\n"); - return; - } + ret = SetForegroundWindow(owner); + ok(ret, "SetForegroundWindow failed, error %lu\n", GetLastError()); hwnd = create_tool_window(WS_CAPTION | WS_SYSMENU | WS_MINIMIZEBOX | WS_MAXIMIZEBOX | WS_POPUP | WS_VISIBLE, owner); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022

[PATCH v2 2/5] user32/tests: Remove unnecessary test skipping checks.

by Rémi Bernon

From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/win.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 3727efb1b41..c6667cafeff 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -1453,10 +1453,6 @@ static void test_nonclient_area(HWND hwnd) GetWindowRect(hwnd, &rc_window); GetClientRect(hwnd, &rc_client); - /* avoid some cases when things go wrong */ - if (IsRectEmpty(&rc_window) || IsRectEmpty(&rc_client) || - rc_window.right > 32768 || rc_window.bottom > 32768) return; - rc = rc_client; MapWindowPoints(hwnd, 0, (LPPOINT)&rc, 2); FixedAdjustWindowRectEx(&rc, style, menu, exstyle); @@ -4314,7 +4310,7 @@ static void test_capture_4(void) WS_OVERLAPPEDWINDOW, CW_USEDEFAULT, 0, 400, 200, NULL, NULL, hInstance, NULL); ok(hwnd != NULL, "CreateWindowEx failed with error %ld\n", GetLastError()); - if (!hwnd) return; + hmenu = CreatePopupMenu(); ret = AppendMenuA( hmenu, MF_STRING, 1, "winetest2"); @@ -4379,11 +4375,8 @@ static void test_keyboard_input(HWND hwnd) ok(GetFocus() == hwnd, "wrong focus window %p\n", GetFocus()); keybd_event(VK_SPACE, 0, 0, 0); - if (!peek_message(&msg)) - { - skip( "keybd_event didn't work, skipping keyboard test\n" ); - return; - } + ret = peek_message(&msg); + ok( ret, "message %04x available\n", msg.message); ok(msg.hwnd == hwnd && msg.message == WM_KEYDOWN, "hwnd %p message %04x\n", msg.hwnd, msg.message); ret = peek_message(&msg); ok( !ret, "message %04x available\n", msg.message); @@ -11289,12 +11282,6 @@ static void test_LockWindowUpdate(HWND parent) {parent, parent, 1, 1} }; - if (!child) - { - skip("CreateWindow failed, skipping LockWindowUpdate tests\n"); - return; - } - ShowWindow(parent, SW_SHOW); UpdateWindow(parent); flush_events(TRUE); @@ -12181,11 +12168,6 @@ static void test_destroy_quit(void) "destroy test main", WS_OVERLAPPED | WS_CAPTION, 100, 100, 100, 100, 0, 0, GetModuleHandleA(NULL), NULL); ok(destroy_data.main_wnd != NULL, "CreateWindowEx failed with error %ld\n", GetLastError()); - if (!destroy_data.main_wnd) - { - CloseHandle(destroy_data.evt); - return; - } thread1 = CreateThread(NULL, 0, destroy_thread1, 0, 0, NULL); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022

[PATCH v2 1/5] user32/tests: Fix indentation to silence a warning.

by Rémi Bernon

From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/msg.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 2705914d5e5..235724a5ff9 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -14089,9 +14089,11 @@ static void test_ShowWindow(void) ok(wp.ptMaxPosition.x == sw[i].wp_max.x && wp.ptMaxPosition.y == sw[i].wp_max.y, "expected %ld,%ld got %ld,%ld\n", sw[i].wp_max.x, sw[i].wp_max.y, wp.ptMaxPosition.x, wp.ptMaxPosition.y); -if (0) /* FIXME: Wine behaves completely different here */ - ok(EqualRect(&win_rc, &wp.rcNormalPosition), "expected %s got %s\n", - wine_dbgstr_rect(&win_rc), wine_dbgstr_rect(&wp.rcNormalPosition)); + if (0) /* FIXME: Wine behaves completely different here */ + { + ok(EqualRect(&win_rc, &wp.rcNormalPosition), "expected %s got %s\n", + wine_dbgstr_rect(&win_rc), wine_dbgstr_rect(&wp.rcNormalPosition)); + } } DestroyWindow(hwnd); flush_events(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022