[PATCH 1/2] dinput/tests: Do not test that reports are identical when polling.
This is dependent on timing, and currently fails occasionally both on Windows and Wine. Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- For examples of failures: http://test.winehq.org/data/4ec67b7a6447dfc4af8c03c141c600b41b90ef53/win81_n... http://test.winehq.org/data/64b96eec7d0aea470f897a3ed0ac9e1b3a680cc5/linux_f... dlls/dinput/tests/hid.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c index 4e107625064..1162f154f8e 100644 --- a/dlls/dinput/tests/hid.c +++ b/dlls/dinput/tests/hid.c @@ -2312,14 +2312,13 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - /* first report should be ready and the same */ + /* first report should be ready */ ret = GetOverlappedResult( async_file, &overlapped, &value, FALSE ); ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); ok( memcmp( report, buffer + caps.InputReportByteLength, caps.InputReportByteLength ), "expected different report\n" ); - ok( !memcmp( report, buffer, caps.InputReportByteLength ), "expected identical reports\n" ); value = 10; SetLastError( 0xdeadbeef ); @@ -2343,12 +2342,11 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - /* first report should be ready and the same */ + /* first report should be ready */ ret = GetOverlappedResult( async_file, &overlapped, &value, FALSE ); ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - ok( !memcmp( report, buffer, caps.InputReportByteLength ), "expected identical reports\n" ); send_hid_input( file, expect_small, sizeof(expect_small) ); -- 2.35.1
Fixes an intermittent test failure. Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/dinput/tests/hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/dinput/tests/hotplug.c b/dlls/dinput/tests/hotplug.c index 2d000317c9f..97e9e69fa9f 100644 --- a/dlls/dinput/tests/hotplug.c +++ b/dlls/dinput/tests/hotplug.c @@ -526,8 +526,8 @@ static HRESULT WINAPI controller_handler_Invoke( IEventHandler_RawGameController trace( "iface %p, sender %p, controller %p\n", iface, sender, controller ); ok( sender == NULL, "got sender %p\n", sender ); - SetEvent( impl->event ); impl->invoked = TRUE; + SetEvent( impl->event ); return S_OK; } -- 2.35.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113957 Your paranoid android. === w1064_tsign (64 bit report) === dinput: hotplug.c:926: Test failed: 1: controller removed handler not invoked hotplug.c:926: Test failed: 1: controller removed handler not invoked
On 5/2/22 23:48, Zebediah Figura wrote:
This is dependent on timing, and currently fails occasionally both on Windows and Wine.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- For examples of failures:
http://test.winehq.org/data/4ec67b7a6447dfc4af8c03c141c600b41b90ef53/win81_n... http://test.winehq.org/data/64b96eec7d0aea470f897a3ed0ac9e1b3a680cc5/linux_f...
dlls/dinput/tests/hid.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c index 4e107625064..1162f154f8e 100644 --- a/dlls/dinput/tests/hid.c +++ b/dlls/dinput/tests/hid.c @@ -2312,14 +2312,13 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - /* first report should be ready and the same */ + /* first report should be ready */ ret = GetOverlappedResult( async_file, &overlapped, &value, FALSE ); ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); ok( memcmp( report, buffer + caps.InputReportByteLength, caps.InputReportByteLength ), "expected different report\n" ); - ok( !memcmp( report, buffer, caps.InputReportByteLength ), "expected identical reports\n" );
value = 10; SetLastError( 0xdeadbeef );
This defeats a bit the purpose of the test, which is to validate that in polled mode all pending reads should be completed at once when device poll completes, whereas in non-polled mode, only one pending read should complete for each received report. I don't think this one is failing so often.
@@ -2343,12 +2342,11 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - /* first report should be ready and the same */ + /* first report should be ready */ ret = GetOverlappedResult( async_file, &overlapped, &value, FALSE ); ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - ok( !memcmp( report, buffer, caps.InputReportByteLength ), "expected identical reports\n" );
send_hid_input( file, expect_small, sizeof(expect_small) );
I'm not completely sure what this second one is trying to achieve, but probably comparing the reports there makes indeed little sense, as the polling frequency is increased a lot, and they will likely often differ. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 5/2/22 17:06, Rémi Bernon wrote:
On 5/2/22 23:48, Zebediah Figura wrote:
This is dependent on timing, and currently fails occasionally both on Windows and Wine.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- For examples of failures:
http://test.winehq.org/data/4ec67b7a6447dfc4af8c03c141c600b41b90ef53/win81_n...
http://test.winehq.org/data/64b96eec7d0aea470f897a3ed0ac9e1b3a680cc5/linux_f...
dlls/dinput/tests/hid.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c index 4e107625064..1162f154f8e 100644 --- a/dlls/dinput/tests/hid.c +++ b/dlls/dinput/tests/hid.c @@ -2312,14 +2312,13 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - /* first report should be ready and the same */ + /* first report should be ready */ ret = GetOverlappedResult( async_file, &overlapped, &value, FALSE ); ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); ok( memcmp( report, buffer + caps.InputReportByteLength, caps.InputReportByteLength ), "expected different report\n" ); - ok( !memcmp( report, buffer, caps.InputReportByteLength ), "expected identical reports\n" ); value = 10; SetLastError( 0xdeadbeef );
This defeats a bit the purpose of the test, which is to validate that in polled mode all pending reads should be completed at once when device poll completes, whereas in non-polled mode, only one pending read should complete for each received report.
I figured as much, and my inclination is that the test just isn't worth keeping around in that case. Perhaps it can be marked interactive instead. Alternatively, as I'm sitting here trying to brainstorm, maybe we could, say, set the poll interval to 100 ms, queue 10 reads, and verify that they all complete in less than 200 ms. That'd also allow us to avoid infinite waits.
I don't think this one is failing so often.
It's not often, but we really shouldn't be letting tests fail at all.
@@ -2343,12 +2342,11 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - /* first report should be ready and the same */ + /* first report should be ready */ ret = GetOverlappedResult( async_file, &overlapped, &value, FALSE ); ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - ok( !memcmp( report, buffer, caps.InputReportByteLength ), "expected identical reports\n" ); send_hid_input( file, expect_small, sizeof(expect_small) );
I'm not completely sure what this second one is trying to achieve, but probably comparing the reports there makes indeed little sense, as the polling frequency is increased a lot, and they will likely often differ.
Given 12dc52cd4a, it does strike me as somewhat unnecessary to test more than one report.
On 5/3/22 01:09, Zebediah Figura (she/her) wrote:
On 5/2/22 17:06, Rémi Bernon wrote:
On 5/2/22 23:48, Zebediah Figura wrote:
This is dependent on timing, and currently fails occasionally both on Windows and Wine.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- For examples of failures:
http://test.winehq.org/data/4ec67b7a6447dfc4af8c03c141c600b41b90ef53/win81_n...
http://test.winehq.org/data/64b96eec7d0aea470f897a3ed0ac9e1b3a680cc5/linux_f...
dlls/dinput/tests/hid.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c index 4e107625064..1162f154f8e 100644 --- a/dlls/dinput/tests/hid.c +++ b/dlls/dinput/tests/hid.c @@ -2312,14 +2312,13 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - /* first report should be ready and the same */ + /* first report should be ready */ ret = GetOverlappedResult( async_file, &overlapped, &value, FALSE ); ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); ok( memcmp( report, buffer + caps.InputReportByteLength, caps.InputReportByteLength ), "expected different report\n" ); - ok( !memcmp( report, buffer, caps.InputReportByteLength ), "expected identical reports\n" ); value = 10; SetLastError( 0xdeadbeef );
This defeats a bit the purpose of the test, which is to validate that in polled mode all pending reads should be completed at once when device poll completes, whereas in non-polled mode, only one pending read should complete for each received report.
I figured as much, and my inclination is that the test just isn't worth keeping around in that case.
Perhaps it can be marked interactive instead.
I don't think interactive tests are useful, especially not here where there's no need for user input.
Alternatively, as I'm sitting here trying to brainstorm, maybe we could, say, set the poll interval to 100 ms, queue 10 reads, and verify that they all complete in less than 200 ms. That'd also allow us to avoid infinite waits.
I don't think this one is failing so often.
It's not often, but we really shouldn't be letting tests fail at all.
It actually never failed, only the second one does. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 5/2/22 18:14, Rémi Bernon wrote:
On 5/3/22 01:09, Zebediah Figura (she/her) wrote:
On 5/2/22 17:06, Rémi Bernon wrote:
On 5/2/22 23:48, Zebediah Figura wrote:
This is dependent on timing, and currently fails occasionally both on Windows and Wine.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- For examples of failures:
http://test.winehq.org/data/4ec67b7a6447dfc4af8c03c141c600b41b90ef53/win81_n...
http://test.winehq.org/data/64b96eec7d0aea470f897a3ed0ac9e1b3a680cc5/linux_f...
dlls/dinput/tests/hid.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c index 4e107625064..1162f154f8e 100644 --- a/dlls/dinput/tests/hid.c +++ b/dlls/dinput/tests/hid.c @@ -2312,14 +2312,13 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); - /* first report should be ready and the same */ + /* first report should be ready */ ret = GetOverlappedResult( async_file, &overlapped, &value, FALSE ); ok( ret, "GetOverlappedResult failed, last error %lu\n", GetLastError() ); ok( value == (report_id ? 3 : 4), "GetOverlappedResult returned length %lu, expected %u\n", value, (report_id ? 3 : 4) ); ok( memcmp( report, buffer + caps.InputReportByteLength, caps.InputReportByteLength ), "expected different report\n" ); - ok( !memcmp( report, buffer, caps.InputReportByteLength ), "expected identical reports\n" ); value = 10; SetLastError( 0xdeadbeef );
This defeats a bit the purpose of the test, which is to validate that in polled mode all pending reads should be completed at once when device poll completes, whereas in non-polled mode, only one pending read should complete for each received report.
I figured as much, and my inclination is that the test just isn't worth keeping around in that case.
Perhaps it can be marked interactive instead.
I don't think interactive tests are useful, especially not here where there's no need for user input.
I've found interactive tests to be useful in DirectShow and ws2_32, for cases where it's difficult to impossible to write reliable tests, but where a "mostly reliable" test is still valuable. In such cases the test can't be automated, but it's already usually the case that signing off on a patch implies running the appropriate tests for it manually. Obviously "interactive" is a bit of a misnomer in this case, except inasmuch as it means "requires some attention which is not automated".
Alternatively, as I'm sitting here trying to brainstorm, maybe we could, say, set the poll interval to 100 ms, queue 10 reads, and verify that they all complete in less than 200 ms. That'd also allow us to avoid infinite waits.
I don't think this one is failing so often.
It's not often, but we really shouldn't be letting tests fail at all.
It actually never failed, only the second one does.
Hmm, I thought I encountered the first one failing locally, but I could be wrong. I'll just send a patch to remove the second part for now.
participants (4)
-
Marvin -
Rémi Bernon -
Zebediah Figura -
Zebediah Figura (she/her)