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

[Arkadiusz Hiler]

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.
> > > 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);

We would also need a mouse variant.

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.
> > > @@ -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.