Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Dmitry Timoshkov]

Wolfgang Walter  wrote:
> - if (res || (!res && GetLastError() == ERROR_IO_PENDING))
> -
>    /* if data has been sent: wait for termination */
> -
>    Sleep(timeout);

I don't see such a problem with real COM-port and serial-USB cable
under Windows or Linux here and under testbot VMs.

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Wolfgang Walter]

On Thursday 05 September 2013 10:37:22 you wrote:
> Wolfgang Walter  wrote:
> > - if (res || (!res && GetLastError() == ERROR_IO_PENDING))
> > -
> >    /* if data has been sent: wait for termination */
> > -
> >    Sleep(timeout);
>
> I don't see such a problem with real COM-port and serial-USB cable
> under Windows or Linux here and under testbot VMs.

Wine does that here (vanilla). I added this so that the NEXT test does not 
depend what wine exactly does.

Regards,

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Dmitry Timoshkov]

Wolfgang Walter  wrote:
> > > - if (res || (!res && GetLastError() == ERROR_IO_PENDING))
> > > -
> > >    /* if data has been sent: wait for termination */
> > > -
> > >    Sleep(timeout);
> >
> > I don't see such a problem with real COM-port and serial-USB cable
> > under Windows or Linux here and under testbot VMs.
>
> Wine does that here (vanilla). I added this so that the NEXT test does not 
> depend what wine exactly does.

When Wine behaviour differs from Windows one the test results need to be
marked as todo_wine, and such places already have it.

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Wolfgang Walter]

Am Donnerstag, 5. September 2013, 17:54:58 schrieb Dmitry Timoshkov:
> Wolfgang Walter  wrote:
> > > > - if (res || (!res && GetLastError() == ERROR_IO_PENDING))
> > > > -
> > > >    /* if data has been sent: wait for termination */
> > > > -
> > > >    Sleep(timeout);
> > >
> > > I don't see such a problem with real COM-port and serial-USB cable
> > > under Windows or Linux here and under testbot VMs.
> >
> > Wine does that here (vanilla). I added this so that the NEXT test does not
> > depend what wine exactly does.
>
> When Wine behaviour differs from Windows one the test results need to be
> marked as todo_wine, and such places already have it.

I don't unterstand you. I didn't change the test case. It still above and is 
still marked with todo.

This change simply ensures that the next test is not distorted. If wine does 
not pass the test

"WriteFile on an overlapped handle without ovl structure should fail"

but instead sends the bytes we should wait long enough before starting the 
next test so that there are no remaining bytes in the tx buffer. Otherwise the 
next test will not test sending 17 bytes but much more.

Regards,

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Dmitry Timoshkov]

Wolfgang Walter  wrote:
> > > > > - if (res || (!res && GetLastError() == ERROR_IO_PENDING))
> > > > > -
> > > > >    /* if data has been sent: wait for termination */
> > > > > -
> > > > >    Sleep(timeout);
> > > >
> > > > I don't see such a problem with real COM-port and serial-USB cable
> > > > under Windows or Linux here and under testbot VMs.
> > >
> > > Wine does that here (vanilla). I added this so that the NEXT test does not
> > > depend what wine exactly does.
> >
> > When Wine behaviour differs from Windows one the test results need to be
> > marked as todo_wine, and such places already have it.
>
> I don't unterstand you. I didn't change the test case. It still above and is 
> still marked with todo.

There is no need to add any workarounds for Wine bugs, appropriate places
already have todo_wine statements.

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Wolfgang Walter]

Am Donnerstag, 5. September 2013, 18:55:55 schrieben Sie:
> Wolfgang Walter  wrote:
> > > > > > - if (res || (!res && GetLastError() == ERROR_IO_PENDING))
> > > > > > -
> > > > > >    /* if data has been sent: wait for termination */
> > > > > > -
> > > > > >    Sleep(timeout);
> > > > >
> > > > > I don't see such a problem with real COM-port and serial-USB cable
> > > > > under Windows or Linux here and under testbot VMs.
> > > >
> > > > Wine does that here (vanilla). I added this so that the NEXT test does
> > > > not
> > > > depend what wine exactly does.
> > >
> > > When Wine behaviour differs from Windows one the test results need to be
> > > marked as todo_wine, and such places already have it.
> >
> > I don't unterstand you. I didn't change the test case. It still above and
> > is still marked with todo.
>
> There is no need to add any workarounds for Wine bugs, appropriate places
> already have todo_wine statements.

This is not a work around.

todo_wine will not magically undo sending the bytes.
Please explain why you think that there will be now pending
bytes in the tx buffer if the tests fails.

And why it does not make the next tests fail even if wine would
behave correctly then.

res = WriteFile(hcom, tbuf, sizeof(tbuf), &bytes, NULL);
todo_wine
    ok(!res, "WriteFile on an overlapped handle without ovl structure should fail\n");
todo_wine
    ok(GetLastError() == ERROR_INVALID_PARAMETER, "expected ERROR_INVALID_PARAMETER, got %d\n", GetLastError());

both tests would fail with wine actually. Wine does write 17 bytes to
com port these bytes now sit in tx buffer and need some time sending.
This distorts the following tests which tests the EV_TXEMPTY behaviour:

Again 17 bytes are written and then the tests assume that one waits
for these 17 bytes (timeout value and messurement).
But really we wait for much more bytes being sent, up to 36.

So even if EV_TXEMPTY handling would work the test for it will fail with a timeout.

Regards,

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Dmitry Timoshkov]

Wolfgang Walter  wrote:
> This is not a work around.
>
> todo_wine will not magically undo sending the bytes.
> Please explain why you think that there will be now pending
> bytes in the tx buffer if the tests fails.
>
> And why it does not make the next tests fail even if wine would
> behave correctly then.
> res = WriteFile(hcom, tbuf, sizeof(tbuf), &bytes, NULL);
>
> todo_wine
>     ok(!res, "WriteFile on an overlapped handle without ovl structure should fail\n");
> todo_wine
>     ok(GetLastError() == ERROR_INVALID_PARAMETER, "expected ERROR_INVALID_PARAMETER, got %d\n", GetLastError());
>
> both tests would fail with wine actually. Wine does write 17 bytes to
> com port these bytes now sit in tx buffer and need some time sending.
> This distorts the following tests which tests the EV_TXEMPTY behaviour:
>
> Again 17 bytes are written and then the tests assume that one waits
> for these 17 bytes (timeout value and messurement).
> But really we wait for much more bytes being sent, up to 36.
>
> So even if EV_TXEMPTY handling would work the test for it will fail with a timeout.

Does 'make test' pass without failures for you?

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Wolfgang Walter]

Am Donnerstag, 5. September 2013, 19:40:13 schrieb Dmitry Timoshkov:
> Wolfgang Walter  wrote:
> > This is not a work around.
> >
> > todo_wine will not magically undo sending the bytes.
> > Please explain why you think that there will be now pending
> > bytes in the tx buffer if the tests fails.
> >
> > And why it does not make the next tests fail even if wine would
> > behave correctly then.
> > res = WriteFile(hcom, tbuf, sizeof(tbuf), &bytes, NULL);
> >
> > todo_wine
> > ok(!res, "WriteFile on an overlapped handle without ovl structure
> > should fail\n");> 
> >
> > todo_wine
> > ok(GetLastError() == ERROR_INVALID_PARAMETER, "expected
> > ERROR_INVALID_PARAMETER, got %d\n", GetLastError());> 
> >
> > both tests would fail with wine actually. Wine does write 17 bytes to
> > com port these bytes now sit in tx buffer and need some time sending.
> > This distorts the following tests which tests the EV_TXEMPTY behaviour:
> >
> > Again 17 bytes are written and then the tests assume that one waits
> > for these 17 bytes (timeout value and messurement).
> > But really we wait for much more bytes being sent, up to 36.
> >
> > So even if EV_TXEMPTY handling would work the test for it will fail with a
> > timeout.
>
> Does 'make test' pass without failures for you?

Without patch 1/2 and 2/2 I get for

$ ../../../tools/runtest -P wine -M kernel32.dll -T ../../.. -q -p kernel32_test.exe.so comm.c

fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub
err:comm:set_line_control ByteSize
fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0)
comm.c:2029: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1)

With my patches 1/2 and 2/2 I get:

fixme:iphlpapi:NotifyAddrChange (Handle 0x10ee8e0, overlapped 0x10ee8ec): stub
wine: configuration in '/home/walterw/.wine' has been updated.
fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub
comm.c:857: Test succeeded inside todo block: WaitCommEvent failed with a timeout
comm.c:881: Test succeeded inside todo block: WaitCommEvent error 997
comm.c:883: Test succeeded inside todo block: WaitCommEvent: expected EV_TXEMPTY, got 0x4
comm.c:889: Test succeeded inside todo block: WaitCommEvent used 1141 ms for waiting
err:comm:set_line_control ByteSize
fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0)
comm.c:2036: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1)

So EV_TXEMPTY handling already works partially.

To see that my later patches are needed you must modify the test a little bit:

- dlls/kernel32/tests/comm.c.old  2013-09-05 13:40:10.275757373 +0200
+++ dlls/kernel32/tests/comm.c      2013-09-05 13:40:06.779074398 +0200
@@ -844,6 +844,8 @@
        after - before, bytes, baud);
     /* don't wait for WriteFile completion */

+    Sleep(2000);
+
     S(U(ovl_wait)).Offset = 0;
     S(U(ovl_wait)).OffsetHigh = 0;
     ovl_wait.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);

Then the tests 857 etc. again fail. We already talked about why
that is the case.

Probably I should add that as an additional test.

Regards,

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Dmitry Timoshkov]

Wolfgang Walter  wrote:
> > Does 'make test' pass without failures for you?
>
> Without patch 1/2 and 2/2 I get for
>
> $ ../../../tools/runtest -P wine -M kernel32.dll -T ../../.. -q -p kernel32_test.exe.so comm.c
>
> fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub
> err:comm:set_line_control ByteSize
> fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0)
> comm.c:2029: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1)

The tests pass without any failures.
> With my patches 1/2 and 2/2 I get:
>
> fixme:iphlpapi:NotifyAddrChange (Handle 0x10ee8e0, overlapped 0x10ee8ec): stub
> wine: configuration in '/home/walterw/.wine' has been updated.
> fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub
> comm.c:857: Test succeeded inside todo block: WaitCommEvent failed with a timeout
> comm.c:881: Test succeeded inside todo block: WaitCommEvent error 997
> comm.c:883: Test succeeded inside todo block: WaitCommEvent: expected EV_TXEMPTY, got 0x4
> comm.c:889: Test succeeded inside todo block: WaitCommEvent used 1141 ms for waiting
> err:comm:set_line_control ByteSize
> fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0 func=3 method=0)
> comm.c:2036: Tests skipped: interactive tests (set WINETEST_INTERACTIVE=1)

Successes inside of todo_wine blocks are treated as failres.

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Wolfgang Walter]

Am Donnerstag, 5. September 2013, 21:12:37 schrieb Dmitry Timoshkov:
> Wolfgang Walter  wrote:
> > > Does 'make test' pass without failures for you?
> >
> > Without patch 1/2 and 2/2 I get for
> >
> > $ ../../../tools/runtest -P wine -M kernel32.dll -T ../../.. -q -p
> > kernel32_test.exe.so comm.c
> >
> > fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub
> > err:comm:set_line_control ByteSize
> > fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0
> > func=3 method=0) comm.c:2029: Tests skipped: interactive tests (set
> > WINETEST_INTERACTIVE=1)
>
> The tests pass without any failures.
> > With my patches 1/2 and 2/2 I get:
> >
> > fixme:iphlpapi:NotifyAddrChange (Handle 0x10ee8e0, overlapped 0x10ee8ec):
> > stub wine: configuration in '/home/walterw/.wine' has been updated.
> > fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub
> > comm.c:857: Test succeeded inside todo block: WaitCommEvent failed with a
> > timeout comm.c:881: Test succeeded inside todo block: WaitCommEvent error
> > 997 comm.c:883: Test succeeded inside todo block: WaitCommEvent: expected
> > EV_TXEMPTY, got 0x4 comm.c:889: Test succeeded inside todo block:
> > WaitCommEvent used 1141 ms for waiting err:comm:set_line_control ByteSize
> > fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0
> > func=3 method=0) comm.c:2036: Tests skipped: interactive tests (set
> > WINETEST_INTERACTIVE=1)
>
> Successes inside of todo_wine blocks are treated as failres.

So you think I should remove the wine_todos already here?

Regards,

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Dmitry Timoshkov]

Wolfgang Walter  wrote:
> > > With my patches 1/2 and 2/2 I get:
> > >
> > > fixme:iphlpapi:NotifyAddrChange (Handle 0x10ee8e0, overlapped 0x10ee8ec):
> > > stub wine: configuration in '/home/walterw/.wine' has been updated.
> > > fixme:comm:set_queue_size insize 1024 outsize 1024 unimplemented stub
> > > comm.c:857: Test succeeded inside todo block: WaitCommEvent failed with a
> > > timeout comm.c:881: Test succeeded inside todo block: WaitCommEvent error
> > > 997 comm.c:883: Test succeeded inside todo block: WaitCommEvent: expected
> > > EV_TXEMPTY, got 0x4 comm.c:889: Test succeeded inside todo block:
> > > WaitCommEvent used 1141 ms for waiting err:comm:set_line_control ByteSize
> > > fixme:ntdll:server_ioctl_file Unsupported ioctl 1b000c (device=1b access=0
> > > func=3 method=0) comm.c:2036: Tests skipped: interactive tests (set
> > > WINETEST_INTERACTIVE=1)
> >
> > Successes inside of todo_wine blocks are treated as failres.
>
> So you think I should remove the wine_todos already here?

No, the source of the failures is still there.

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Francois Gouget]

On Thu, 5 Sep 2013, Wolfgang Walter wrote:
[...]
> To see that my later patches are needed you must modify the test a little bit:
> - dlls/kernel32/tests/comm.c.old  2013-09-05 13:40:10.275757373 +0200
>
> +++ dlls/kernel32/tests/comm.c      2013-09-05 13:40:06.779074398 +0200
> @@ -844,6 +844,8 @@
>         after - before, bytes, baud);
>      /* don't wait for WriteFile completion */
> - Sleep(2000);
> - S(U(ovl_wait)).Offset = 0;
>    S(U(ovl_wait)).OffsetHigh = 0;
>    ovl_wait.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);

I did not look at the specifics of this case but I agree with the 
general principle of making tests reasonably independent from each 
other. This way we know exactly what works and what does not. It's not 
an absolute rule but we do try for that already when we reset LastError 
between tests for instance, or in some other cases when we delete some 
resource and recreate it for each test.

I guess the above is not really the patch that you propose but I'll 
still comment on it: we also want the tests to run as fast as possible 
so Sleep() is bad. However if we were to Sleep() only if the previous 
test failed I think that would be ok.

[PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty

[Wolfgang Walter]

Am Sonntag, 8. September 2013, 11:32:26 schrieb Francois Gouget:
> On Thu, 5 Sep 2013, Wolfgang Walter wrote:
> [...]
> > To see that my later patches are needed you must modify the test a little
> > bit:
> > - dlls/kernel32/tests/comm.c.old  2013-09-05 13:40:10.275757373 +0200
> >
> > +++ dlls/kernel32/tests/comm.c      2013-09-05 13:40:06.779074398 +0200
> > @@ -844,6 +844,8 @@
> >     after - before, bytes, baud);
> >
> >  /* don't wait for WriteFile completion */
> >
> > -
> >
> > Sleep(2000);
> > -
> >
> > S(U(ovl_wait)).Offset = 0;
> >    S(U(ovl_wait)).OffsetHigh = 0;
> >    ovl_wait.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
>
> I did not look at the specifics of this case but I agree with the
> general principle of making tests reasonably independent from each
> other. This way we know exactly what works and what does not. It's not
> an absolute rule but we do try for that already when we reset LastError
> between tests for instance, or in some other cases when we delete some
> resource and recreate it for each test.
>
> I guess the above is not really the patch that you propose but I'll
> still comment on it: we also want the tests to run as fast as possible
> so Sleep() is bad. However if we were to Sleep() only if the previous
> test failed I think that would be ok.

Indeed this patch is not the proposed one. It is a variation of a test to show 
a different problem.

My patch to make two tests more independed from each other only sleeps if the 
first test fails.

Regards,