fix NetworkingModule NoContentType test now that didCompleteNetworkRe…

[FLGMwt]

Motivation: testing conditions of the NetworkingModule_Request_Content_String_NoContentType test are no longer valid as of #382, since didCompleteNetworkResponse now emits three arguments. Since the test failure caused a return before the EventWaitHandle was triggered, the test hung indefinitely. After correcting the test conditions, the test passes.

In addition to correcting the test condition, I reworked the use of the waitHandle to prevent the indefinite hang should the test fail in the future. I'm fairly certain we're safe to unconditionally trigger the handle as soon as we enter the MockInvocationHandler and only rely on the testing conditions for the passed flag. I tested this by adding a Task.Delay(5000).Wait() after the waitHandle.Set() as it is in this PR. I saw the invocation handler was allowed to execute to completion, with the passed flag being allowed to be set properly before continuation of the test.

Unless I'm missing something, I thing this could be an issue with many of the tests in the project where we're only triggering the wait handle for the happy path. For example, in NetworkingModule_Response_Content, if I change "didReceiveNetworkData"to "definitelyNotDidReceiveNetworkData" (reflecting a potential name change dispatched event), I see the same indefinite hang.

If I'm not missing something, I can create an issue to investigate this throughout the tests. At a minimum, I think I have enough context to look into #288

[FLGMwt]

Actually, if the EventEmitter and the InvocationHandler are synchronous, is there a need for EventWaitHandles at all? All the tests in NetworkingModuleTests run fine without them, even with Task.Delays. Sorry if I'm missing something obvious ><

[rozele]

Thanks @FLGMwt. Currently, the EventEmitter and the InvocationHandler are synchronous in test because the InvocationHandler doesn't actually push to the JavaScript thread. I think we left in the EventWaitHandle in case the base class ever changed to auto-switch to the JS thread for instance, but that actually does seem unlikely. We could probably delete them.

[rozele]

Maybe we should fix the root issue here, and only send the canceled flag if the value is true. It will save the extra cycles from having to serialize a boolean.

[FLGMwt]

Do you mean changing NetworkingModule.OnRequestError to be something like this?

private void OnRequestError(int requestId, string message, bool timeout)
{
    if (timeout)
    {
        EventEmitter.emit("didCompleteNetworkResponse", new JArray
        {
            requestId,
            message,
            true
        });
    }
    else
    {
        EventEmitter.emit("didCompleteNetworkResponse", new JArray
        {
            requestId,
            message
        });
    }
}

[rozele]

Might be cleaner to just have:

private void OnRequestError(int requestId, string message)
{
...
}

private void OnRequestError(int requestId, string message, bool timeout)
{
...
}

Same number of lines, one less conditional check :-).

[FLGMwt]

In that case, how about

private void OnRequestError(int requestId, string message)
{
    EventEmitter.emit("didCompleteNetworkResponse", new JArray
    {
        requestId,
        message
    });
}

private void OnRequestErrorDueToTimeout(int requestId, string message)
{
    EventEmitter.emit("didCompleteNetworkResponse", new JArray
    {
        requestId,
        message,
        true
    });
}

to save the bool check and the unnecessary bool arg? : ) Open to suggestions for naming OnRequestErrorDueToTimeout.

[rozele]

Sure, or even just OnRequestTimeout ;-).

[rozele]

I'm going to leave this closed for now. I took your original suggestion and just updated the unit test. May make sense still to create a new PR for the "optimization".