[PATCH] dinput/tests: Make overlapped format tests more robust.

[Zebediah Figura (she/her)]

On 8/4/21 2:20 PM, Rémi Bernon wrote:
> Hi Arek,
> 
> It looks almost good, but a few changes would make it even better I 
> think (and taking the opportunity to nit-pick a bit).
> 
> On 8/4/21 5:26 PM, Arkadiusz Hiler wrote:
>> Both Acquire() and event processing with DirectInput seem to be
>> asynchronous. In most cases we can just keep hammering GetDeviceData()
>> until the event gets processed.
>>
>> Things get pretty racy around Acquire() though. If we fire event right 
>> after the
>> call we can find ourselves in one of the three situations:
>>
>> 1. Event happened after acquiring has completed - the wait will suffice.
>>
>> 2. Event happened before acquiring did any real work - the device will
>>     pick up the state as if the event was processed, but there's nothing
>>     in GetDeviceData(). Because of that we cannot fail on wait.
>>
>> 3. Event happened somewhere in the middle of acquiring - we ended up
>>     both missing the event for GetDeviceData() and we have outdated
>>     state. Firing the event again will register as if the button was not
>>     already pressed.
>>
>> This change covers all three scenarios.
>>
>> Signed-off-by: Arkadiusz Hiler <ahiler at codeweavers.com>
>> ---
>>
>> I've done some more testing and noticed that indeed event processing is
>> asynchronous - events can arrive a bit late but we never miss them.
>>
>> The situation gets a bit tricky around Acquire(). After trying many 
>> things to
>> make it behave nicely it turned out there are three possible states we 
>> can find
>> ourselves in.
>>
>> I've been spamming the testbot and two VMs locally with the tests and 
>> it seems
>> to be reliable now.
>>
>> If this solution gets accepted I'll give the same treatment to dinput 
>> mouse
>> tests (which are currently testing nothing on Windows) and dinput8 
>> device tests.
>>
>> Thanks Rémi for the suggestions!
>>
>> Cc: Rémi Bernon <rbernon at codeweavers.com>
>>
>>   dlls/dinput/tests/device.c | 54 +++++++++++++++++++++++++++++++++-----
>>   1 file changed, 48 insertions(+), 6 deletions(-)
>>
>> diff --git a/dlls/dinput/tests/device.c b/dlls/dinput/tests/device.c
>> index 6307957b6e2..d404c769953 100644
>> --- a/dlls/dinput/tests/device.c
>> +++ b/dlls/dinput/tests/device.c
>> @@ -343,11 +343,32 @@ static void pump_messages(void)
>>       }
>>   }
>> +BOOL wait_for_device_data_and_discard_(int line, IDirectInputDeviceA 
>> *device)
>> +{
>> +    DWORD cnt;
>> +    HRESULT hr;
>> +    DWORD start_time = GetTickCount();
>> +
>> +    do
>> +    {
>> +        cnt = 10;
>> +        hr = IDirectInputDevice_GetDeviceData(device, 
>> sizeof(DIDEVICEOBJECTDATA_DX3), NULL, &cnt, 0);
>> +        ok_(__FILE__, line)(SUCCEEDED(hr), 
>> "IDirectInputDevice_GetDeviceData() failed: %08x\n", hr);
>> +        ok_(__FILE__, line)(cnt == 0 || cnt == 1, "Unexpected number 
>> of events: %d\n", cnt);
>> +    } while (cnt != 1 && (GetTickCount() - start_time < 500));
>> +
>> +    return cnt == 1;
>> +}
>> +
>> +#define wait_for_device_data_and_discard(device) 
>> wait_for_device_data_and_discard_(__LINE__, device)
>> +
> 
> NP: These macros are usually declared right before the function itself.

I don't think it's that consistent; I've seen both organizations.

>>   void overlapped_format_tests(IDirectInputA *pDI, HWND hwnd)
>>   {
>>       HRESULT hr;
>>       struct overlapped_state state;
>>       IDirectInputDeviceA *keyboard = NULL;
>> +    DIPROPDWORD dp;
>> +    DWORD tries;
>>       hr = IDirectInput_CreateDevice(pDI, &GUID_SysKeyboard, 
>> &keyboard, NULL);
>>       ok(SUCCEEDED(hr), "IDirectInput_CreateDevice() failed: %08x\n", 
>> hr);
>> @@ -355,15 +376,28 @@ void overlapped_format_tests(IDirectInputA *pDI, 
>> HWND hwnd)
>>       /* test overlapped slider - default value 0 */
>>       hr = IDirectInputDevice_SetDataFormat(keyboard, 
>> &overlapped_slider_format);
>>       ok(SUCCEEDED(hr), "IDirectInputDevice_SetDataFormat() failed: 
>> %08x\n", hr);
>> +
>> +    dp.diph.dwSize = sizeof(DIPROPDWORD);
>> +    dp.diph.dwHeaderSize = sizeof(DIPROPHEADER);
>> +    dp.diph.dwHow = DIPH_DEVICE;
>> +    dp.diph.dwObj = 0;
>> +    dp.dwData = 10;
>> +    hr = IDirectInputDevice_SetProperty(keyboard, DIPROP_BUFFERSIZE, 
>> &dp.diph);
>> +    ok(SUCCEEDED(hr), "IDirectInputDevice_SetProperty() failed: 
>> %08x\n", hr);
>> +
>>       hr = IDirectInputDevice_Acquire(keyboard);
>>       ok(SUCCEEDED(hr), "IDirectInputDevice_Acquire() failed: %08x\n", 
>> hr);
>>       SetFocus(hwnd);
>>       pump_messages();
>> -    /* press D */
>> -    keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
>> -    pump_messages();
>> +    tries = 2;
>> +    do
>> +    {
>> +        /* press D */
>> +        keybd_event(0, DIK_D, KEYEVENTF_SCANCODE, 0);
>> +        pump_messages();
>> +    } while (!wait_for_device_data_and_discard(keyboard) && tries--);
>>       memset(&state, 0xFF, sizeof(state));
>>       hr = IDirectInputDevice_GetDeviceState(keyboard, sizeof(state), 
>> &state);
> 
> I think it could be worth having a dedicated helper to acquire and wait, 
> instead of inlining these loops. The same helper could then be (copied) 
> and used in the other tests.
> 
> The tries counter and its hardcoded value is a bit confusing at first, 
> and instead of keeping the key pressed the helper could also make sure 
> it's released on function exit (maybe use a dedicated unusual key for 
> that?).
> 
>> @@ -378,6 +412,8 @@ void overlapped_format_tests(IDirectInputA *pDI, 
>> HWND hwnd)
>>       /* release D */
>>       keybd_event(0, DIK_D, KEYEVENTF_SCANCODE|KEYEVENTF_KEYUP, 0);
>>       pump_messages();
>> +    ok(wait_for_device_data_and_discard(keyboard),
>> +            "Timed out while waiting for injected events to be picked 
>> up by DirectInput.\n");
>>       hr = IDirectInputDevice_Unacquire(keyboard);
>>       ok(SUCCEEDED(hr), "IDirectInputDevice_Unacquire() failed: 
>> %08x\n", hr);
> 
> NP: Yes, dinput code is heavily inconsistent, but I don't think it uses 
> the two level indent on line continuation style anywhere yet. Personally 
> I don't find it very readable and I would rather align on the paren.

The problem with aligning to the parenthesis (or other semantic unit) is 
that it makes things really ugly when the parenthesis falls late in the 
line. For whatever my opinion is worth, I'm distinctly in favor of a 
constant eight-space indent, including spreading that to more code.