[PATCH v4 0/3] MR3565: d3d9/tests: Work around test_reset_fullscreen failing on gitlab CI.
--- See the added comment for details what is going on. -- v4: d3d9/tests: Wait longer in test_occlusion_query for software renderers. d3d9/tests: The device window may restore behind our back in test_wndproc. d3d9/tests: Work around test_reset_fullscreen failing on gitlab CI. https://gitlab.winehq.org/wine/wine/-/merge_requests/3565
From: Stefan Dösinger <stefan(a)codeweavers.com> --- See the added comment for details what is going on. --- dlls/d3d9/tests/device.c | 68 ++++++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 20 deletions(-) diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c index 8ab52c7d5e4..0d38f68bf3f 100644 --- a/dlls/d3d9/tests/device.c +++ b/dlls/d3d9/tests/device.c @@ -4752,11 +4752,14 @@ done: static void test_reset_fullscreen(void) { struct device_desc device_desc; - D3DDISPLAYMODE d3ddm, d3ddm2; unsigned int mode_count, i; IDirect3DDevice9 *device; + D3DDISPLAYMODE d3ddm; WNDCLASSEXA wc = {0}; IDirect3D9 *d3d; + BOOL fvwm2_bad; + D3DFORMAT fmt; + RECT r1, r2; HRESULT hr; ATOM atom; static const struct message messages[] = @@ -4795,49 +4798,74 @@ static void test_reset_fullscreen(void) goto cleanup; } + IDirect3D9_GetAdapterDisplayMode(d3d, D3DADAPTER_DEFAULT, &d3ddm); + fmt = d3ddm.Format; + mode_count = IDirect3D9_GetAdapterModeCount(d3d, D3DADAPTER_DEFAULT, fmt); + for (i = 0; i < mode_count; ++i) + { + hr = IDirect3D9_EnumAdapterModes(d3d, D3DADAPTER_DEFAULT, fmt, i, &d3ddm); + ok(SUCCEEDED(hr), "Failed to enumerate display mode, hr %#lx.\n", hr); + + if (d3ddm.Width != registry_mode.dmPelsWidth || d3ddm.Height != registry_mode.dmPelsHeight) + break; + } + if (i == mode_count) + { + skip("Could not find a suitable display mode.\n"); + goto cleanup; + } + /* * Switch to fullscreen mode. * This will force the window to be shown and will cause the WM_ACTIVATEAPP * message to be sent. */ - device_desc.width = registry_mode.dmPelsWidth; - device_desc.height = registry_mode.dmPelsHeight; + device_desc.width = d3ddm.Width; + device_desc.height = d3ddm.Height; device_desc.device_window = device_window; device_desc.flags = CREATE_DEVICE_FULLSCREEN; ok(SUCCEEDED(reset_device(device, &device_desc)), "Failed to reset device.\n"); + /* FVWM2 doesn't update its knowledge about the screen size on mode changes. It + * will therefore try to resize our fullscreen window to the original size. We + * don't learn about this until we process messages, but then we receive a + * WM_SIZE message after d3d9 stopped filtering. To dodge the issue, switch to + * the original mode when testing for WM_SIZE, but double check that the window + * size was changed. */ + GetWindowRect(device_window, &r1); + flush_events(); ok(expect_messages->message == 0, "Expected to receive message %#x.\n", expect_messages->message); expect_messages = NULL; - IDirect3D9_GetAdapterDisplayMode(d3d, D3DADAPTER_DEFAULT, &d3ddm); - mode_count = IDirect3D9_GetAdapterModeCount(d3d, D3DADAPTER_DEFAULT, d3ddm.Format); - for (i = 0; i < mode_count; ++i) - { - hr = IDirect3D9_EnumAdapterModes(d3d, D3DADAPTER_DEFAULT, d3ddm.Format, i, &d3ddm2); - ok(SUCCEEDED(hr), "Failed to enumerate display mode, hr %#lx.\n", hr); - - if (d3ddm2.Width != d3ddm.Width || d3ddm2.Height != d3ddm.Height) - break; - } - if (i == mode_count) - { - skip("Could not find a suitable display mode.\n"); - goto cleanup; - } + /* This is what FVWM2 does to us: Resize the window behind our back. */ + GetWindowRect(device_window, &r2); + fvwm2_bad = !EqualRect(&r1, &r2); + todo_wine_if(fvwm2_bad) + ok(EqualRect(&r1, &r2), "Window rect changed during event flush: %s -> %s.\n", + wine_dbgstr_rect(&r1), wine_dbgstr_rect(&r2)); wm_size_received = 0; /* Fullscreen mode change. */ - device_desc.width = d3ddm2.Width; - device_desc.height = d3ddm2.Height; + device_desc.width = registry_mode.dmPelsWidth; + device_desc.height = registry_mode.dmPelsHeight; device_desc.device_window = device_window; device_desc.flags = CREATE_DEVICE_FULLSCREEN; ok(SUCCEEDED(reset_device(device, &device_desc)), "Failed to reset device.\n"); flush_events(); + + /* And some FVWM2 versions still insist on resizing in the flush_events() above - + * my fvwm 2.7.0 resizes the window back to the size before the mode change and + * then back to the new mode. This doesn't affect the gitlab machines afaics. */ + todo_wine_if (fvwm2_bad && wm_size_received) ok(!wm_size_received, "Received unexpected WM_SIZE message.\n"); + GetWindowRect(device_window, &r2); + ok(!EqualRect(&r1, &r2), "Window rectangles before and after mode change are equal: %s\n", + wine_dbgstr_rect(&r1)); + cleanup: if (device) IDirect3DDevice9_Release(device); IDirect3D9_Release(d3d); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3565
From: Stefan Dösinger <stefan(a)codeweavers.com> d3d8 is not affected in this way because it does not call flush_events() between focus loss and re-gain. --- dlls/d3d9/tests/device.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c index 0d38f68bf3f..27bc82b8933 100644 --- a/dlls/d3d9/tests/device.c +++ b/dlls/d3d9/tests/device.c @@ -4228,6 +4228,24 @@ static void test_wndproc(void) } flush_events(); + if (!(tests[i].create_flags & CREATE_DEVICE_NOWINDOWCHANGES)) + { + /* The window manager on Gitlab CI (FVWM2) insists on restoring the device window for some + * reason. In this case we might not receive WM_WINDOWPOSCHANGING on the device window + * because nothing is being changed on focus gain. Manually minimizing it again seems to + * resolve this. + * + * Wine's internal status is being updated while processing messages (the flush_events above). */ + ret = IsIconic(device_window); + todo_wine_if (!ret) + ok(ret, "Expected the device window to still be iconic.\n"); + if (!ret) + ShowWindow(device_window, SW_MINIMIZE); + flush_events(); + ret = IsIconic(device_window); + ok(ret, "Device window refuses to minimize.\n"); + } + /* I have to minimize and restore the focus window, otherwise native d3d9 fails * device::reset with D3DERR_DEVICELOST. This does not happen when the window * restore is triggered by the user. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3565
From: Stefan Dösinger <stefan(a)codeweavers.com> The issue can be reproduced with LIBGL_ALWAYS_SOFTWARE=1 on Mesa. The Gitlab machines are affected. --- dlls/d3d9/tests/device.c | 4 +++- dlls/d3d9/tests/utils.h | 8 +++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c index 27bc82b8933..42c5f5879e3 100644 --- a/dlls/d3d9/tests/device.c +++ b/dlls/d3d9/tests/device.c @@ -6626,7 +6626,9 @@ static void test_occlusion_query(void) hr = IDirect3DDevice9_BeginScene(device); ok(hr == D3D_OK, "Failed to begin scene, hr %#lx.\n", hr); - for (i = 0; i < 50000; ++i) + /* This loop can take some time on Mesa's software renderer. Don't push + * it too high or it might time out on Gitlab CI. */ + for (i = 0; i < 30000; ++i) { hr = IDirect3DQuery9_Issue(query, D3DISSUE_BEGIN); ok(hr == D3D_OK, "Got unexpected hr %#lx.\n", hr); diff --git a/dlls/d3d9/tests/utils.h b/dlls/d3d9/tests/utils.h index 248e07eb521..1cfa97d09b4 100644 --- a/dlls/d3d9/tests/utils.h +++ b/dlls/d3d9/tests/utils.h @@ -27,12 +27,18 @@ static inline void wait_query_(const char *file, unsigned int line, IDirect3DQue unsigned int i; HRESULT hr; - for (i = 0; i < 500; ++i) + /* Some of the larger loops in test_occlusion_query() can take quite some time on + * software Mesa (i.e., the gitlab CI machines). */ + for (i = 0; i < 1000; ++i) { if ((hr = IDirect3DQuery9_GetData(query, NULL, 0, D3DGETDATA_FLUSH)) == S_OK) break; Sleep(10); } + + if (hr == S_OK && i >= 500) + trace_(file, line)("Query took %u ms to finish.\n", i * 10); + ok_(file, line)(hr == S_OK, "Got unexpected hr %#lx.\n", hr); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3565
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=136746 Your paranoid android. === w10pro64 (64 bit report) === d3d9: device.c:4159: Test failed: Expected message 0x46 for window 0x1, but didn't receive it, i=0. device.c:4203: Test failed: Expected message 0x7e for window 0, but didn't receive it, i=0. device.c:4207: Test failed: The device window is active, i=0. device.c:4212: Test failed: Got unexpected hr 0x88760869. device.c:4216: Test failed: Got unexpected screen size 800x600. device.c:4241: Test failed: Expected the device window to still be iconic. device.c:4260: Test failed: Expected message 0x46 for window 0, but didn't receive it, i=0. device.c:4269: Test failed: Got unexpected screen size 800x600. device.c:4286: Test failed: Expected message 0x7e for window 0, but didn't receive it, i=0. device.c:4294: Test failed: Got unexpected width 640. device.c:4295: Test failed: Got unexpected height 480. device.c:4376: Test failed: Expected message 0x7e for window 0x1, but didn't receive it, i=0. device.c:4383: Test failed: Expected IsIconic 1, got 0, i=0. device.c:4387: Test failed: Got unexpected hr 0. device.c:4395: Test failed: Expected message 0x46 for window 0x1, but didn't receive it, i=0. device.c:4433: Test failed: Expected message 0x1c for window 0x1, but didn't receive it, i=0. device.c:4443: Test failed: Got unexpected hr 0. device.c:4501: Test failed: Expected message 0x18 for window 0, but didn't receive it, i=0. device.c:4507: Test failed: Got unexpected WINDOWPOS hwnd=0000000000000000, insertAfter=0000000000000000, x=0, y=0, cx=0, cy=0, flags=0 device.c:4524: Test failed: Expected the device window to be visible, i=0.
Updates in this newest push: - commit 1: unchanged - commit 2: re-minimize the device window. Continue the test normally afterwards - commit 3: todo_wine -> trace -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3565#note_43551
On Mon Aug 28 13:36:05 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=136746 Your paranoid android. === w10pro64 (64 bit report) === d3d9: device.c:4159: Test failed: Expected message 0x46 for window 0x1, but didn't receive it, i=0. device.c:4203: Test failed: Expected message 0x7e for window 0, but didn't receive it, i=0. device.c:4207: Test failed: The device window is active, i=0. device.c:4212: Test failed: Got unexpected hr 0x88760869. device.c:4216: Test failed: Got unexpected screen size 800x600. device.c:4241: Test failed: Expected the device window to still be iconic. device.c:4260: Test failed: Expected message 0x46 for window 0, but didn't receive it, i=0. device.c:4269: Test failed: Got unexpected screen size 800x600. device.c:4286: Test failed: Expected message 0x7e for window 0, but didn't receive it, i=0. device.c:4294: Test failed: Got unexpected width 640. device.c:4295: Test failed: Got unexpected height 480. device.c:4376: Test failed: Expected message 0x7e for window 0x1, but didn't receive it, i=0. device.c:4383: Test failed: Expected IsIconic 1, got 0, i=0. device.c:4387: Test failed: Got unexpected hr 0. device.c:4395: Test failed: Expected message 0x46 for window 0x1, but didn't receive it, i=0. device.c:4433: Test failed: Expected message 0x1c for window 0x1, but didn't receive it, i=0. device.c:4443: Test failed: Got unexpected hr 0. device.c:4501: Test failed: Expected message 0x18 for window 0, but didn't receive it, i=0. device.c:4507: Test failed: Got unexpected WINDOWPOS hwnd=0000000000000000, insertAfter=0000000000000000, x=0, y=0, cx=0, cy=0, flags=0 device.c:4524: Test failed: Expected the device window to be visible, i=0. ``` The full output of this has the following line:
`device.c:4142: Tests skipped: Not running in foreground, skip foreground window test` Which doesn't show up on the same machine on 32 bit, where the tests pass. The change to test_reset_fullscreen() runs before the failing test_wndproc here, so it has the potential to somehow convince windows that a different program has the user's attention. The failure did not show up on a manual re-run (https://testbot.winehq.org/JobDetails.pl?Key=136757#k318). Otoh this test seems to have been very stable in the past on Windows, so it isn't something I want to wave away easily. (http://test.winehq.org/data/tests/d3d9:device.html) Also cause for concern, the Linux testbot is still failing test_wndproc despite manually re-minifying the window: https://testbot.winehq.org/JobDetails.pl?Key=136757#k401 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3565#note_43574
On Sat Aug 26 08:19:37 2023 +0000, Alexandre Julliard wrote:
Yeah. I don't know why we didn't do this right from the beginning. Or apply any of the other accumulated knowledge from the TestBot, for that matter. We can certainly tweak things, but I see no evidence that the Testbot runs consistently have better results than the Gitlab ones. And honestly, if the tests can only succeed in a carefully tuned environment, that's not much better than when they only succeeded on my box. We need to accept that Wine runs in various environments with various upstream bugs, and deal with that. If upstream bugs have a high impact we need to work around them in the code; if we decide that the impact is low enough not to bother, then it should be OK to ignore the failures in the tests. There may be some unusual configurations that we decide not to support, and maybe fvwm focus is one of them, but that should be the last resort. We need to stop using upstream bugs as an excuse to not fix the tests. For a radically different direction: the opposite of running the tests 'in a carefully tuned environment' would be to run them in either GNOME or KDE (see below). This is not to say that we should only fix bugs for these DEs, but prioritizing them could make sense. But then most Wine developers probably already use them so maybe we can consider GNOME and KDE to already be covered and dedicate the CIs to testing other environments.
Also to complement Alexandre's comment and something not necessarily related to this MR, fixing the tests can be different from fixing Wine (bad uses of flaky, adding workarounds in the tests). The goal is to fix Wine of course so that's something to keep in mind when fixing the tests. Market share information is hard to come by but based on Debian's popularity contest [1] and GamingOnLinux stats [2] we have: | | [1] | [2] | |----------|-----|-----| | GNOME | 41% | 30% | | XFce | 21% | 8% | | KDE | 17% | 38% | | Cinnamon | 9% | 8% | 1. https://popcon.debian.org/by_inst Stats based on the task-xxx-desktop packages. The fvwm1 + fvwm + fvwm3 packages would be around 1%. 2. https://www.gamingonlinux.com/users/statistics/ -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3565#note_43674
This merge request was closed by Stefan Dösinger. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3565
I am closing this merge request in favor of two separate ones: !3680 fixes the timeout in test_occlusion_query with the software renderer !3719 fixes d3d9:d3d9ex on openbox I am still working on fvwm3. The problem is that unlike openbox it has a number of flaky failures. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3565#note_44151
participants (3)
-
Francois Gouget (＠fgouget) -
Marvin -
Stefan Dösinger