Re: [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.
>
> What do you mean with that? The tests indeed do succeed now and there is a 
> reason they do: when you call WaitCommEvent() while the tx buffer is not empty 
> yet the wine code will detect that EV_TXEMPTY correctly:

The tests must pass under Wine without any additional "fixes" as they do
currently under Windows. If you add some code to the tests which suddenly
makes them pass under Wine - that's not a fix, Wine implementation should
be fixed instead.

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

[Jonas Maebe]

On 05 Sep 2013, at 16:32, Dmitry Timoshkov wrote:
> The tests must pass under Wine without any additional "fixes" as they do
> currently under Windows. If you add some code to the tests which suddenly
> makes them pass under Wine - that's not a fix, Wine implementation should
> be fixed instead.

What I understood from Wolfgang's previous explanations is that there is currently this situation:

todo_wine
 test1();
/* test1 fails because wine has a bug. As a side-effect, bytes will be left in the transmission buffer under wine, but not under windows. */
test2();
/* test2 also fails under wine because of the failure of test1 that influences the behaviour of test2, not because the functionality tested by test2 is broken. This make it impossible to individually evaluate test1() and test2(). */

So either
* you add code in between that resets the state so that test2 can be run independently of whether test1 failed (as Wolfgang proposes), or
* you put the tests in completely different executables so they don't influence each other (hopefully), or
* you reorder the test so that first all the tests that currently work under wine are run (under the assumption that no regression will ever creep in), or
* you require that bugs in wine routines are fixed in the order that they are called in that test (which would be strange)

Jonas

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

[Dmitry Timoshkov]

Jonas Maebe  wrote:
> > The tests must pass under Wine without any additional "fixes" as they do
> > currently under Windows. If you add some code to the tests which suddenly
> > makes them pass under Wine - that's not a fix, Wine implementation should
> > be fixed instead.
>
> What I understood from Wolfgang's previous explanations is that there is currently this situation:
>
> todo_wine
>  test1();
> /* test1 fails because wine has a bug. As a side-effect, bytes will be left in the transmission buffer under wine, but not under windows. */
> test2();
> /* test2 also fails under wine because of the failure of test1 that influences the behaviour of test2, not because the functionality tested by test2 is broken. This make it impossible to individually evaluate test1() and test2(). */
>
> So either
> - you add code in between that resets the state so that test2 can be run independently of whether test1 failed (as Wolfgang proposes), or
> - you put the tests in completely different executables so they don't influence each other (hopefully), or
> - you reorder the test so that first all the tests that currently work under wine are run (under the assumption that no regression will ever creep in), or
> - you require that bugs in wine routines are fixed in the order that they are called in that test (which would be strange)

How about a patch set which does:
1/2. add a workaround to the tests to pass under Wine
2/2. fix Wine code and simultaneously remove the workaround added by 1/2.

But that looks even more strange if the fix contained in 2/2 could be
sent alone.

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

[Jonas Maebe]

On 05 Sep 2013, at 16:58, Dmitry Timoshkov wrote:
> Jonas Maebe  wrote:
> > So either
> > - you add code in between that resets the state so that test2 can be run independently of whether test1 failed (as Wolfgang proposes), or
> > - you put the tests in completely different executables so they don't influence each other (hopefully), or
> > - you reorder the test so that first all the tests that currently work under wine are run (under the assumption that no regression will ever creep in), or
> > - you require that bugs in wine routines are fixed in the order that they are called in that test (which would be strange)
>
> How about a patch set which does:
> 1/2. add a workaround to the tests to pass under Wine
> 2/2. fix Wine code and simultaneously remove the workaround added by 1/2.
>
> But that looks even more strange if the fix contained in 2/2 could be
> sent alone.

I disagree it's a "workaround to make the tests pass under Wine". It's simply good practice to make supposedly unrelated tests actually independent from each other. The fact that this makes some extra tests succeed under Wine simply means that the test code was broken before (unless the only goal was to test whether everything works or not everything works, as opposed to testing whether individual functionality works). And when a regression occurs afterwards, it will also be easier to see what exactly broke by the regression rather than having to add all that code again to make the tests independent from each other once more.

Jonas

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

[Wolfgang Walter]

Am Donnerstag, 5. September 2013, 23:58:57 schrieb Dmitry Timoshkov:
> Jonas Maebe  wrote:
> > > The tests must pass under Wine without any additional "fixes" as they do
> > > currently under Windows. If you add some code to the tests which
> > > suddenly
> > > makes them pass under Wine - that's not a fix, Wine implementation
> > > should
> > > be fixed instead.
> >
> > What I understood from Wolfgang's previous explanations is that there is
> > currently this situation:
> >
> > todo_wine
> >
> > test1();
> >
> > /* test1 fails because wine has a bug. As a side-effect, bytes will be
> > left in the transmission buffer under wine, but not under windows. */
> > test2();
> > /* test2 also fails under wine because of the failure of test1 that
> > influences the behaviour of test2, not because the functionality tested
> > by test2 is broken. This make it impossible to individually evaluate
> > test1() and test2(). */
> >
> > So either
> > - you add code in between that resets the state so that test2 can be run
> >
> > independently of whether test1 failed (as Wolfgang proposes), or * you
> > put the tests in completely different executables so they don't influence
> > each other (hopefully), or * you reorder the test so that first all the
> > tests that currently work under wine are run (under the assumption that
> > no regression will ever creep in), or * you require that bugs in wine
> > routines are fixed in the order that they are called in that test (which
> > would be strange)
>
> How about a patch set which does:
> 1/2. add a workaround to the tests to pass under Wine
> 2/2. fix Wine code and simultaneously remove the workaround added by 1/2.

I don't have a fix for

WaitCommEvent() does not return ERROR_INVALID_PARAMETER without ovl 
structure

And I had no plans to do so nor did I claim.

I removed the side effect that this test makes the next test fail which tests 
the EV_TXEMPTY handling even if that it would work perfectly.

So:

test A
test B

test A fails on wine and this influences test B in such a way that it will 
always fail, too.

I remove this dependency.

Now test B succeeds. This is not because I worked arround a wine-bug in what B 
should test but because wine behaves correctly in this test.

And test A still fails.
> But that looks even more strange if the fix contained in 2/2 could be
> sent alone.

Regards,

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

[Wolfgang Walter]

Am Donnerstag, 5. September 2013, 23:32:00 schrieben Sie:
> 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.
> >
> > What do you mean with that? The tests indeed do succeed now and there is a
> > reason they do: when you call WaitCommEvent() while the tx buffer is not
> > empty
> > yet the wine code will detect that EV_TXEMPTY correctly:
>
> The tests must pass under Wine without any additional "fixes" as they do
> currently under Windows. If you add some code to the tests which suddenly
> makes them pass under Wine - that's not a fix, Wine implementation should
> be fixed instead.

You basically say that one may not fix a bug before fixing another one which 
is not related only because they are tested for in a special order and these 
tests influence each other though they are not really related.

Again:

In this case not wine behaves incorrectly but the test is simply wrong. It 
first tests if WaitCommEvent() returns  ERROR_INVALID_PARAMETER if called with 
NULL without without ovl structure. Then it tests the EV_TXEMPTY handling. The 
latter test will always fail even if it works perfectly as long as wine does 
not return ERROR_INVALID_PARAMETER but instead does the write in the first 
test. This is because the second tests does not wait long enough for both 
writes to complete.

Regards,

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

[Dmitry Timoshkov]

Wolfgang Walter  wrote:
> > > > No, the source of the failures is still there.
> > >
> > > What do you mean with that? The tests indeed do succeed now and there is a
> > > reason they do: when you call WaitCommEvent() while the tx buffer is not
> > > empty
> > > yet the wine code will detect that EV_TXEMPTY correctly:
> >
> > The tests must pass under Wine without any additional "fixes" as they do
> > currently under Windows. If you add some code to the tests which suddenly
> > makes them pass under Wine - that's not a fix, Wine implementation should
> > be fixed instead.
>
> You basically say that one may not fix a bug before fixing another one which 
> is not related only because they are tested for in a special order and these 
> tests influence each other though they are not really related.

Adding a workaround to the tests to compensate a Wine bug and as a side effect
remove some todo_wines is not a fix. Yes, some tests depend on each other, but
that's on purpose, there is no need to break this dependency just because you
can make some later tests suddenly "pass".