Re: [PATCH 1/3] user32/tests: Sleep enough time to make sure all input message have been processed.
Hello, This patch is marked as pending, is there anything I can improve? Did the test results in [1] and [2] help? Thanks for any feedback! [1] https://testbot.winehq.org/JobDetails.pl?Key=26191 [2] https://testbot.winehq.org/JobDetails.pl?Key=26190 On Wed, Jul 3, 2013 at 2:45 AM, Qian Hong <qhong(a)codeweavers.com> wrote:
Hello,
Current menu test code may hide some failure tests [1], with the attached patch, all failure tests could be detected [2].
[1] https://testbot.winehq.org/JobDetails.pl?Key=26191 [2] https://testbot.winehq.org/JobDetails.pl?Key=26190
--- dlls/user32/tests/menu.c | 1 + 1 file changed, 1 insertion(+)
-- Regards, Qian Hong - http://www.codeweavers.com
Qian Hong <qhong(a)codeweavers.com> writes:
Hello,
This patch is marked as pending, is there anything I can improve? Did the test results in [1] and [2] help?
Adding sleeps to work around timing issues is usually suspicious, particularly since the code is already supposed to wait. You'd have to explain why that's not sufficient. -- Alexandre Julliard julliard(a)winehq.org
Hello, Thanks for the feedback. On Fri, Jul 12, 2013 at 3:07 AM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Adding sleeps to work around timing issues is usually suspicious, particularly since the code is already supposed to wait. You'd have to explain why that's not sufficient.
Trying to explain why the sleeping in the while loop is not sufficient: The attached patch do two things: 1. - { INPUT_KEYBOARD, {{0}}, {VK_F10, 0}, FALSE, FALSE }, + { INPUT_KEYBOARD, {{0}}, {VK_F10, 0}, TRUE, FALSE }, Change the expected bMenuVisible from FALSE to TRUE in a passed test ( test 19 ) If the original test is correct, then after I change FALSE to TRUE, it should fail now. 2. Added some trace on sleeping time. The test result is here: https://testbot.winehq.org/JobDetails.pl?Key=26260 Looking at the test result, some times test 19 failed as expected, some times test 19 passed, such as https://testbot.winehq.org/JobDetails.pl?Key=26260&log_207=1#k207 --- snip --- test 19: Are we going to sleep? test 19: total sleeped time: 0 WARNING! sleeped time is zero! --- snip --- The reason that total sleeping time equal to zero is, (menu_tests[19].bMenuVisible == bMenuVisible) is true at the beginning, so the while loop is totally skipped, and we get a fake passed test result. Due to the above reason, a better while loop is as below: do { if (elapsed > 200) break; elapsed += 20; Sleep(20); } while (menu_tests[i].bMenuVisible != bMenuVisible) However, even that is not sufficient. For some complex test, (menu_tests[19].bMenuVisible == bMenuVisible) could become true after receiving the first input message but before receiving the last input message, so we get a fake passed test result as well. That is why I add a Sleep(100) after the while loop. Does my explaining make sense? I have another idea: 1. counting the amount of the received messages in the WndProc callback: case WM_KEYDOWN: case WM_KEYDOWN: case WM_SYSKEYDOWN: /* case WM_MOUSEMOVE: */ // Don't count WM_MOUSEMOVE case WM_LBUTTONDOWN: case XXX: case YYY: ... received_msg++; 2. and counting the amount of sent messages in send_key and click_menu, such like: static void send_key(WORD wVk) { .... sent_msg++; } 3. finally change the while loop to: do { sleeping; } while (sent_msg match received_msg or slept too long) Would you want me submit this version? Thanks for reviewing. -- Regards, Qian Hong - http://www.codeweavers.com
On Fri, Jul 12, 2013 at 7:11 PM, Qian Hong <qhong(a)codeweavers.com> wrote:
do { sleeping; } while (sent_msg match received_msg or slept too long)
typo, should be: while (sent_msg does not match received_msg && haven't sleep too long) -- Regards, Qian Hong - http://www.codeweavers.com
On Fri, Jul 12, 2013 at 3:07 AM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Adding sleeps to work around timing issues is usually suspicious,
Hello, how about this one? -- Regards, Qian Hong - http://www.codeweavers.com
Qian Hong <qhong(a)codeweavers.com> writes:
On Fri, Jul 12, 2013 at 3:07 AM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Adding sleeps to work around timing issues is usually suspicious,
Hello, how about this one?
This looks like a better approach, yes (make sure to fix your tab size before submitting though, the indentation is messed up). -- Alexandre Julliard julliard(a)winehq.org
On Mon, Jul 15, 2013 at 7:02 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
This looks like a better approach, yes
Sorry, this approach doesn't work as I expected, further investigation show that posted messages would be processed before input messages, msdn confirms it: --- quote --- If no filter is specified, messages are processed in the following order: Sent messages Posted messages Input (hardware) messages and system internal events Sent messages (again) WM_PAINT messages WM_TIMER messages --- quote --- msdn also said: To retrieve input messages before posted messages, use the wMsgFilterMin and wMsgFilterMaxparameters. However, even we can change the filter in our test, native Popup WndProc still receive posted messages before input message because it has its own message loop and we can't (and of cause we shouldn't) change it. This situation run out of my head, I can't find any easy way to fix it, I doubt if there is any way better than the original patch (patch 97161, pending). I'm open to any suggestion, thanks! If no better solution I hope patch 97161 is acceptable. -- Regards, Qian Hong - http://www.codeweavers.com
participants (2)
-
Alexandre Julliard -
Qian Hong