Re: [PATCH 2/5] wait until all data from earlier test has been written in test_waittxempty
Wolfgang Walter <wine(a)stwm.de> 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. -- Dmitry.
On Thursday 05 September 2013 10:37:22 you wrote:
Wolfgang Walter <wine(a)stwm.de> 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, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Recht Abteilungsleiter IT Leopoldstraße 15 80802 München
Wolfgang Walter <wine(a)stwm.de> 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. -- Dmitry.
Am Donnerstag, 5. September 2013, 17:54:58 schrieb Dmitry Timoshkov:
Wolfgang Walter <wine(a)stwm.de> 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, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Wolfgang Walter <wine(a)stwm.de> 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. -- Dmitry.
Am Donnerstag, 5. September 2013, 18:55:55 schrieben Sie:
Wolfgang Walter <wine(a)stwm.de> 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, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Wolfgang Walter <wine(a)stwm.de> 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? -- Dmitry.
Am Donnerstag, 5. September 2013, 19:40:13 schrieb Dmitry Timoshkov:
Wolfgang Walter <wine(a)stwm.de> 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, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Wolfgang Walter <wine(a)stwm.de> 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. -- Dmitry.
Am Donnerstag, 5. September 2013, 21:12:37 schrieb Dmitry Timoshkov:
Wolfgang Walter <wine(a)stwm.de> 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, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
Wolfgang Walter <wine(a)stwm.de> 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. -- Dmitry.
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. -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ Good judgment comes from experience, and experience comes from bad judgment -- Barry LePatner
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, -- Wolfgang Walter Studentenwerk München Anstalt des öffentlichen Rechts Abteilungsleiter IT Leopoldstraße 15 80802 München
participants (3)
-
Dmitry Timoshkov -
Francois Gouget -
Wolfgang Walter