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

[Jacek Caban]

Hi Sebastian,

On 04/25/16 21:41, Sebastian Lackner wrote:
> - memset( &ib, 0, sizeof(ib) );
> - memset( buffer, 0, sizeof(buffer) );
> - ib.dwStructSize = sizeof(ib);
> - for (count = 0; count < sizeof(buffer); count += ib.dwBufferLength)
> - {
> -
>    ib.lpvBuffer = buffer + count;
> -
>    ib.dwBufferLength = sizeof(buffer) - count;
> -
> -
>    SET_OPTIONAL( INTERNET_STATUS_RECEIVING_RESPONSE );
> -
>    SET_OPTIONAL( INTERNET_STATUS_RESPONSE_RECEIVED );
> -
> -
>    ret = InternetReadFileExA( req, &ib, 0, 0xdeadbeef );
> -
>    if (!ret)
> -
>    {
> -
>        ok( GetLastError() == ERROR_IO_PENDING, "expected ERROR_IO_PENDING, got %u\n", GetLastError() );
> -
>        SET_EXPECT( INTERNET_STATUS_REQUEST_COMPLETE );
> -
>        SetEvent( conn_wait_event );
> -
>        WaitForSingleObject( hCompleteEvent, INFINITE );
> -
>        ok( req_error == ERROR_SUCCESS, "expected ERROR_SUCCESS, got %u\n", req_error );
> -
>        CHECK_NOTIFIED( INTERNET_STATUS_REQUEST_COMPLETE );
> -
>    }

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

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

[Sebastian Lackner]

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'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?

Regards,
Sebastian

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

[Jacek Caban]

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. 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.

+            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.

Same comments apply to InternetReadFile tests.

Thanks,
Jacek

[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

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

[Jacek Caban]

On 04/26/16 23:00, Sebastian Lackner wrote:
> 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. ;)

Yes, please. Both timeout 0 and removing the test is fine with me.

BTW, other patches look good to me.

Thanks,
Jacek

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

[Sebastian Lackner]

On 26.04.2016 23:13, Jacek Caban wrote:
> On 04/26/16 23:00, Sebastian Lackner wrote:
> > 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. ;)
>
> Yes, please. Both timeout 0 and removing the test is fine with me.
>
> BTW, other patches look good to me.
>
> Thanks,
> Jacek

Thanks, I've sent a new version.