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

[Rémi Bernon]

On 8/4/21 11:00 PM, Arkadiusz Hiler wrote:
> On Wed, Aug 04, 2021 at 02:25:49PM -0500, Zebediah Figura (she/her) wrote:
>> On 8/4/21 2:20 PM, Rémi Bernon wrote:
> <SNIP>
>>>> +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.
> 
> Indeed. I've checked 3 instances that I've found by looking for `ok_`.
> All three were declared this way, so I went with that.
> 

It's not completely consistent yes, but I think it quite stands out that 
most of the time it's the case.

$ egrep 'define.*\W[a-z]\w+_\(' dlls/*/tests/* -A1

>>>>    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?).
> 
> "Unusual key" may be hard to pick as AFAIU it has to be present in the
> data format.
> 
> How about something like:
> 
> static void flush_acquire_keyboard_(int line, IDirectInputDeviceA *device, DWORD valid_dik)
> {
>      int tries = 2;
>      do
>      {
>          keybd_event(0, valid_dik, KEYEVENTF_SCANCODE, 0);
>      } while (!wait_for_device_data_and_discard(device) && tries--);
> 
>      keybd_event(0, valid_dik, KEYEVENTF_SCANCODE|KEYEVENTF_KEYUP, 0);
>      ok_(__FILE__, line)(wait_for_device_data_and_discard(device),
>              "Timed out while waiting for injected events to be picked up by DirectInput.\n");
> }
> 
> #define flush_acquire_keyboard(device, valid_dik) flush_acquire_keyboard_(__LINE__, device, valid_dik)
> 
> And then we can:
> 
> hr = IDirectInputDevice_Acquire(keyboard);
> ok(SUCCEEDED(hr), "IDirectInputDevice_Acquire() failed: %08x\n", hr);
> flush_acquire_keyboard(keyboard, DIK_F);
> 

Why not even call Acquire inside the helper (and name it 
acquire_and_wait or something like that)?

> We would also need a mouse variant.
> 

You could inject mouse events too, but if the helper is copied in other 
test sources then it's probably better to use a slightly different 
version with mouse events instead.

> I wonder if instead of copying it verbatim all over the place it would
> be okay to create dlls/dinput/tests/event_helpers.h and include it from
> the dinput8 tests.
> 

I'm not sure it's worth a header to avoid coping it to a few files 
(especially considering we may rather have two helpers for mouse / 
keyboard). And I don't think it's a good idea to make dinput8 tests 
include files from another test folder so at least it would be 
duplicated (also because the device interface type is different).

>>>> @@ -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.
> 
> I share the same preference as zf. I think I've even set my editor to do
> that after seeing this convention used a lot in msvcrt and a few other
> places.
> 

As much as I don't like arguing about style variations, this one is 
actually bothering me. Alignment (on the paren, on operators, etc) is a 
pretty common style, and I think it actually makes things more readable.

I feel like the fixed two level indentation only helps when writing 
code, by making the decision simpler, or possibly diffs by saving 
re-alignments, but it doesn't help reading the code at all, especially 
in cases with nested parentheses.

On function declaration, and because we often have a lot of declspec 
(and possibly long ones like STDMETHODCALLTYPE) it may be relevant but 
otherwise in the code I don't feel like it's an improvement.


Then again, although dinput style is inconsistent it's not using that 
style yet anywhere (except for function declarations and even only in a 
few locations), so unless we intend to rewrite it all I don't see reason 
to add another style variation.

In my recent contributions to dinput and as some of the existing code 
was already like this, I chose a style similar to the user32 / ntdll 
style, with space in parentheses (see ansi.c). And unless it really 
bothers someone I intended to use it for my HID joystick too.

Cheers,
-- 
Rémi Bernon <rbernon at codeweavers.com>