From: "Zebediah Figura (she/her)" <zfigura@codeweavers.com>
Subject: Re: [PATCH] ws2_32/tests: Add order-agnostic check in test_simultaneous_async_recv.
Message-Id: <c9a752d3-0e32-02d4-99fd-f29433bdf934@codeweavers.com>
Date: Mon, 10 Jan 2022 15:00:58 -0600
In-Reply-To: <ae5cade5-f7bc-6100-51f1-4923373c4440@gmail.com>
References: <684917dd-81d8-12a8-1f27-7ce8ed6bacbe@gmail.com> <d5995f17-0804-e53d-f766-1328b7b88f7d@codeweavers.com> <ae5cade5-f7bc-6100-51f1-4923373c4440@gmail.com>

On 1/9/22 06:26, Jinoh Kang wrote:
> On 1/9/22 03:37, Zebediah Figura (she/her) wrote:
>> On 1/7/22 12:15, Jinoh Kang wrote:
>>> Signed-off-by: Jinoh Kang <jinoh.kang.kr@gmail.com>
>>> ---
>>>    dlls/ws2_32/tests/sock.c | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c
>>> index 782b1e59729..966a681e809 100644
>>> --- a/dlls/ws2_32/tests/sock.c
>>> +++ b/dlls/ws2_32/tests/sock.c
>>> @@ -11520,6 +11520,8 @@ static void test_simultaneous_async_recv(void)
>>>            const void *expect = msgstr + i * stride;
>>>            const void *actual = resbuf + i * stride;
>>>            DWORD size;
>>> +        int allcmp;
>>> +        size_t j;
>>>              ret = WaitForSingleObject(events[i], 1000);
>>>            ok(!ret, "wait timed out\n");
>>> @@ -11528,6 +11530,20 @@ static void test_simultaneous_async_recv(void)
>>>            ret = GetOverlappedResult((HANDLE)client, &overlappeds[i], &size, FALSE);
>>>            ok(ret, "got error %u\n", GetLastError());
>>>            ok(size == stride, "got size %u\n", size);
>>> +
>>> +        allcmp = 0;
>>> +        for (j = 0; j <= num_io * stride - size; j++) allcmp |= !memcmp(msgstr + j, actual, size);
>>> +        ok(allcmp, "returned data shall be part of original message (got %s)\n", debugstr_an(actual, size));
>>> +
>>
>> I'm sorry, but this test makes no sense to me. What exactly are you trying to check here?
> 
> It's a redundant check meant to ease debugging in case a future regression causes wine to
> reorder overlapped I/O operations while still completing all of them.
> 
> It can be taken out if it is deemed unnecessary.  Actually, the point of this patch is in
> the added comment.
> 

I'm not sure I see the need for this additional test. If we complete the 
asyncs out of order we'll already see the original message dumped out of 
order; the existing test already uses debugstr_an().