Re: user32: Fix a test that now passes in Windows

[Reece Dunn]

On 25/10/2007, James Hawkins wrote:
> Hi,
>
> Changelog:
> - Fix a test that now passes in Windows.

Looking at the code, it looks as if they are now passing in Wine :).
> dlls/user32/tests/dde.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> - str = (LPSTR)DdeAccessData(hdata, &size);
> - ok(!lstrcmpA(str, "requested data\r\n"), "Expected 'requested data\r\n', got %s\n", str);

Shouldn't there be quote marks around the %s (i.e. '%s', to match the
expected value)?
> - ok(size == 19, "Expected 19, got %d\n", size);
> -
> - ret = DdeUnaccessData(hdata);
> - ok(ret == TRUE, "Expected TRUE, got %d\n", ret);

These are new test cases. Since these are not fixes to existing test
cases, they should really be in a separate patch.

- Reece

user32: Fix a test that now passes in Windows

[James Hawkins]

On 10/25/07, Reece Dunn  wrote:
> On 25/10/2007, James Hawkins wrote:
> > Hi,
> >
> > Changelog:
> > - Fix a test that now passes in Windows.
>
> Looking at the code, it looks as if they are now passing in Wine :).

I don't think you are looking at the code, nor does it seem you've run
the tests.  The tests that now pass in Wine are moved out of the
todo_wine block, and the one remaining failing test is still in the
todo_wine block.
> > dlls/user32/tests/dde.c |   13 ++++++++++---
> >  1 files changed, 10 insertions(+), 3 deletions(-)
> > - str = (LPSTR)DdeAccessData(hdata, &size);
> > - ok(!lstrcmpA(str, "requested data\r\n"), "Expected 'requested data\r\n', got %s\n", str);
>
> Shouldn't there be quote marks around the %s (i.e. '%s', to match the
> expected value)?

No.  The quotes around 'requested data\r\n' are for ease of reading the code.
> > - ok(size == 19, "Expected 19, got %d\n", size);
> > -
> > - ret = DdeUnaccessData(hdata);
> > - ok(ret == TRUE, "Expected TRUE, got %d\n", ret);
>
> These are new test cases. Since these are not fixes to existing test
> cases, they should really be in a separate patch.

Huh?  You're really coming out of right field.  I don't think it needs
explanation, but you don't seem to get it.  It's not a 'new' test
case, it's a part of the test block 'XTYP_REQUEST, fAckReq = TRUE'.
The call used to fail before in Windows, but was fixed by a timing
patch committed by Julliard.  There's no point in checking the
requested data if the call fails.  Now the call doesn't fail, so we
check the data.

Please read the code thoroughly before making comments like these.

user32: Fix a test that now passes in Windows

[Reece Dunn]

On 26/10/2007, James Hawkins  wrote:
> On 10/25/07, Reece Dunn  wrote:
> > On 25/10/2007, James Hawkins wrote:
> > > Hi,
> > >
> > > Changelog:
> > > - Fix a test that now passes in Windows.
> >
> > Looking at the code, it looks as if they are now passing in Wine :).
>
> I don't think you are looking at the code, nor does it seem you've run
> the tests.  The tests that now pass in Wine are moved out of the
> todo_wine block, and the one remaining failing test is still in the
> todo_wine block.

Looking at the code (i.e. the supplied patch), the change that is "Fix
a test that now passes in Windows" is:

-    ok(res == 0xdeadbeef, "Expected 0xdeadbeef, got %08x\n", res);
+    ok(hdata != NULL, "Expected non-NULL hdata, got %p\n", hdata);
+    ok(ret == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", ret);
     todo_wine
     {
-        ok(hdata == NULL, "Expected NULL hdata, got %p\n", hdata);
-        ok(ret == DMLERR_DATAACKTIMEOUT, "Expected
DMLERR_DATAACKTIMEOUT, got %d\n", ret);
+        ok(res == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %08x\n", res);
     }

Because the changes to hdata, res and ret have modified what passes on
Wine, they have been rearranged. Therefore, my comment there was
wrong.
> > > dlls/user32/tests/dde.c |   13 ++++++++++---
> > >  1 files changed, 10 insertions(+), 3 deletions(-)
> > > - str = (LPSTR)DdeAccessData(hdata, &size);
> > > - ok(!lstrcmpA(str, "requested data\r\n"), "Expected 'requested data\r\n', got %s\n", str);
> >
> > Shouldn't there be quote marks around the %s (i.e. '%s', to match the
> > expected value)?
>
> No.  The quotes around 'requested data\r\n' are for ease of reading the code.

What if str contains newline characters that don't match the expected
case? It would be easier to see that if the string was quoted.
> > > - ok(size == 19, "Expected 19, got %d\n", size);
> > > -
> > > - ret = DdeUnaccessData(hdata);
> > > - ok(ret == TRUE, "Expected TRUE, got %d\n", ret);
> >
> > These are new test cases. Since these are not fixes to existing test
> > cases, they should really be in a separate patch.
>
> Huh?  You're really coming out of right field.  I don't think it needs
> explanation, but you don't seem to get it.  It's not a 'new' test
> case, it's a part of the test block 'XTYP_REQUEST, fAckReq = TRUE'.

I was referring to the last hunk in that patch. These are /new/ lines
of code that add /new/ ok checks (referring to them as test cases was
bad phrasing on my part).
> The call used to fail before in Windows, but was fixed by a timing
> patch committed by Julliard.  There's no point in checking the
> requested data if the call fails.  Now the call doesn't fail, so we
> check the data.

Ok.

- Reece

user32: Fix a test that now passes in Windows

[Dan Hipschman]

Some of what you're saying has validity, but all your criticisms are on
very minor points, and I was surprised to see them.  I think they're
offset by the fact that James is a long-time developer who has a lot of
work to do.  I'm sure the Wine community appreciates every person who
wants to help to make Wine better, but please get to know the culture of
the Wine developers before criticizing them.  Stop CCing wine-patches,
it's for patches only.

Thanks.