On 4/19/22 11:57, Bernhard Kölbl wrote:
WaitForSingleObject(compilation_handler.event_finished,INFINITE);
todo_wineok(!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 alreadyfinished */
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 unexpectedref %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), "Waitfor 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,