[2/6] wininet/tests: Add tests for asynchronous InternetReadFileEx.

[Sebastian Lackner]

On 26.04.2016 22:47, Jacek Caban wrote:
> Hi Sebastian,
> 
> On 04/26/16 16:11, Sebastian Lackner wrote:
>> On 26.04.2016 14:30, Jacek Caban wrote:
>>> It's a nice way to force async read, but when you add tests with
>>> predictable reads it would be nice to take more advantage of that. What
>>> do you think about:
>>>
>>> - call InternetReadFileEx and check that it's done sync
>>> - call InternetReadFileEx and check that it's done async
>>> - set conn_wait_event, wait completion and make sure we read all
>>> remaining data
>>> - call InternetReadFileEx and make sure it synchronously hits the end of
>>> data
>>>
>>> Thanks,
>>> Jacek
>>>
>>>
>> I don't think its necessary to restructure the whole code, unrolling the
>> loop would require a lot of code duplication because Wine still behaves
>> different in many aspects, and even on Windows it depends on things like
>> buffer sizes.
> 
> I expected Wine to be closer to Windows in this case. I experimented a
> bit with your tests and I can see that those are less predictable than I
> thought.

Yes, thats also what I noticed when I played around with it. We can probably
make the tests more precise at some later point.

> You'd see Wine behaviour by using IRF_NO_WAIT, so handling
> flags 0 is a problem in Wine, but not the only one. Anyway, those are
> unrelated to your changes, so it's fine as is.
> 
> 
>> I've added more tests (some as todo_wine) for the things you
>> pointed out below though. Are you fine with the new patch, or any other
>> cases you would like to have tested?
> 
> Thanks for adding them. Just a few comments:
> 
> +            if (!pending_reads++)
> +            {
> +                res = WaitForSingleObject( hCompleteEvent, 200 );
> +                ok( res == WAIT_TIMEOUT, "expected WAIT_TIMEOUT, got %u\n", res );
> +                SetEvent( conn_wait_event );
> +            }
> 
> 
> I'm not sure what is this for and I'd like to avoid adding expected
> timeouts is possible. Checking last error ERROR_IO_PENDING seems enough
> to consider it being done asynchronously. Sorry if you misunderstood my
> comment about checking it for being async.

The idea of this additional check was to ensure that the request cannot be
fulfilled with the current buffer contents. Otherwise its not completely
clear that async handler is really waiting for new data, as it should be.
If you prefer, I can also remove this test again or set the timeout to 0 to
avoid blocking. As the tests show, this part is already working fine. ;)

> 
> +            todo_wine_if( ib.dwBufferLength == 0 )
> +            ok( ib.dwBufferLength != 0, "expected ib.dwBufferLength != 0\n" );
> 
> 
> todo_wine_if(pending_reads > 1) should do the trick here and won't
> completely disable the test on Wine.

Okay, I will change that.

> 
> Same comments apply to InternetReadFile tests.
> 
> Thanks,
> Jacek
>