[PATCH v4 1/4] windows.media.speech/tests: Add test to check if IAsyncInfo_Close non is blocking.

[Rémi Bernon]

On 4/19/22 11:57, Bernhard Kölbl wrote:
>>> -        WaitForSingleObject(compilation_handler.event_finished,
>> INFINITE);
>>> +        todo_wine
>> ok(!WaitForSingleObject(compilation_handler.event_finished, 1000), "Wait
>> for event_finished failed.\n");
>>>            CloseHandle(compilation_handler.event_finished);
>>>
>>>            hr =
>> IAsyncOperation_SpeechRecognitionCompilationResult_put_Completed(operation,
>> NULL);
>>
>>
>> I don't understand why you need to change this now? If it is going to
>> fail and timeout after you stubbed some method, maybe it'd be better to
>> make the change with the stub.
>>
> 
> The reason for this change is so the tests can't be blocked forever and you
> get a message if it failed. 1000ms is long enough for the finished event to
> fire. Makes debugging the tests easier imho.
> 


I think having a non-INFINITE timeout makes the intent less clear. If 
the wait is never supposed to timeout, INFINITE makes it obvious imho. 
It also makes sure you will never get spurious failures even if for some 
reason a run gets particularly slow.

If for some reason the test breaks and the wait starts blocking, the 
testbot timeout is there to catch it, although yes, it's a bit longer.


>>
>>> @@ -1057,23 +1095,10 @@ static void test_SpeechRecognizer(void)
>>>            hr =
>> IAsyncOperation_SpeechRecognitionCompilationResult_QueryInterface(operation,
>> &IID_IAsyncInfo, (void **)&info);
>>>            todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
>>>
>>> -        /* This one can fail, if the async operation had already
>> finished */
>>> -        compilation_result = (void*)0xdeadbeef;
>>> -        hr =
>> IAsyncOperation_SpeechRecognitionCompilationResult_GetResults(operation,
>> &compilation_result);
>>> -        todo_wine ok(hr == E_ILLEGAL_METHOD_CALL, "Got unexpected hr
>> %#lx.\n", hr);
>>> -        todo_wine ok(compilation_result == (void*)0xdeadbeef,
>> "Compilation result had value %p.\n", compilation_result);
>>> -
>>> -        async_status = 0xdeadbeef;
>>> -        hr = IAsyncInfo_get_Status(info, &async_status);
>>> -        todo_wine ok(hr == S_OK, "IAsyncInfo_get_Status failed, hr
>> %#lx.\n", hr);
>>> -        todo_wine ok(async_status == Started || async_status ==
>> Completed, "Status was %#x.\n", async_status);
>>> -
>>> -        IAsyncInfo_Release(info);
>>> -
>>
>>
>> Why remove these tests?
>>
> 
> The tests were basically testing races and only caused timing issues.
> 


Okay, a dedicated patch would make it clearer (or with the broken result 
changes).


>>
>>>            async_status = 0xdeadbeef;
>>> @@ -1081,10 +1106,73 @@ static void test_SpeechRecognizer(void)
>>>            todo_wine ok(hr == S_OK, "IAsyncInfo_get_Status failed, hr
>> %#lx.\n", hr);
>>>            todo_wine ok(async_status == Completed, "Status was %#x.\n",
>> async_status);
>>>
>>> +        ref = IAsyncInfo_Release(info);
>>> +        todo_wine ok(ref == 1, "Got unexpected ref %lu.\n", ref);
>>> +        ref =
>> IAsyncOperation_SpeechRecognitionCompilationResult_Release(operation);
>>> +        todo_wine ok(!ref, "Got unexpected ref %lu.\n", ref);
>>> +
>>> +        /* Test if AsyncInfo_Close waits for the handler to finish. */
>>> +        hr = RoActivateInstance(hstr, &inspectable_2);
>>> +        todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
>>> +
>>> +        hr = IInspectable_QueryInterface(inspectable_2,
>> &IID_ISpeechRecognizer, (void **)&recognizer_2);
>>> +        todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
>>> +
>>> +        compilation_handler_create_static(&compilation_handler3);
>>> +        compilation_handler3.event_block = CreateEventW(NULL, FALSE,
>> FALSE, NULL);
>>> +        compilation_handler3.event_finished = CreateEventW(NULL, FALSE,
>> FALSE, NULL);
>>> +        compilation_handler3.thread_id = GetCurrentThreadId();
>>> +
>>
>>
>> Do you really need new variables here? I think you should just re-use
>> the existing ones, eventually cleaning them up and re-creating if really
>> necessary.
>>
> 
> 
> I guess you mean the inspectables. Yeah I can remove them.
> 


The handlers and recognizer too, no? I don't think you use the other 
variables after this except to clean them up, which you could do there, 
before re-creating them.


> 
>>
>>
>>> +        if (thread_status == WAIT_TIMEOUT)
>>> +        {
>>> +            todo_wine ok(compilation_handler3.ref == 3, "Got unexpected
>> ref %lu.\n", compilation_handler3.ref);
>>> +            todo_wine check_refcount(operation, 3);
>>> +
>>> +            hr =
>> IAsyncOperation_SpeechRecognitionCompilationResult_QueryInterface(operation,
>> &IID_IAsyncInfo, (void **)&info);
>>> +            todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
>>> +
>>> +            close_param.info = info;
>>> +            close_thread = CreateThread(NULL, 0,
>> async_operation_close_thread, &close_param, 0, NULL);
>>> +            todo_wine ok(!WaitForSingleObject(close_thread, 100), "Wait
>> for close_thread failed.\n");
>>> +
>>
>>
>> If IAsyncInfo_Close doesn't actually block then you don't need a thread
>> to call it, right?
>>
> 
> The idea is to have WaitForSingleObject fail if it ever was to block,
> either if someone changes the code or sth on Windows changes.
> 


Sure, though you can think the same for every function call. It's 
currently unnecessary so I think you shouldn't be forward thinking too 
much, and change it later iff it becomes necessary (probably never).


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