Re: [PATCH 1/3] user32/tests: Sleep enough time to make sure all input message have been processed.

[Qian Hong]

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  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(+)

[PATCH 1/3] user32/tests: Sleep enough time to make sure all input message have been processed.

[Alexandre Julliard]

Qian Hong  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.

[PATCH 1/3] user32/tests: Sleep enough time to make sure all input message have been processed.

[Qian Hong]

Hello,

Thanks for the feedback.

On Fri, Jul 12, 2013 at 3:07 AM, Alexandre Julliard  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

[PATCH 1/3] user32/tests: Sleep enough time to make sure all input message have been processed.

[Qian Hong]

On Fri, Jul 12, 2013 at 7:11 PM, Qian Hong  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

[PATCH 1/3] user32/tests: Sleep enough time to make sure all input message have been processed.

[Qian Hong]

On Fri, Jul 12, 2013 at 3:07 AM, Alexandre Julliard  wrote:
> Adding sleeps to work around timing issues is usually suspicious,

Hello, how about this one?

--
Regards,
Qian Hong

-
http://www.codeweavers.com

[PATCH 1/3] user32/tests: Sleep enough time to make sure all input message have been processed.

[Alexandre Julliard]

Qian Hong  writes:
> On Fri, Jul 12, 2013 at 3:07 AM, Alexandre Julliard  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).

[PATCH 1/3] user32/tests: Sleep enough time to make sure all input message have been processed.

[Qian Hong]

On Mon, Jul 15, 2013 at 7:02 PM, Alexandre Julliard  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