Re: [2/2] oleacc: implemented GetRoleText[A/W] with tests (try3)

[Dmitry Timoshkov]

"Nikolay Sivov"  wrote:

First of all there is no need to send the configure.ac patch separately.
> - /* zero role number - not documented */
> - ret = GetRoleTextA(0, NULL, 0);
> - ok(ret > 0, "GetRoleTextA doesn't return length for zero role number, got %d\n", ret);
> - ret = GetRoleTextW(0, NULL, 0);
> - ok(ret > 0, "GetRoleTextW doesn't return length for zero role number, got %d\n", ret);
> -
> - /* NULL buffer, return length */
> - ret = GetRoleTextA(ROLE_SYSTEM_TITLEBAR, NULL, 0);
> - ok(ret > 0, "GetRoleTextA doesn't return length on NULL buffer, got %d\n", ret);
> - ret = GetRoleTextA(ROLE_SYSTEM_TITLEBAR, NULL, 1);
> - ok(ret > 0, "GetRoleTextA doesn't return length on NULL buffer, got %d\n", ret);
> - ret = GetRoleTextW(ROLE_SYSTEM_TITLEBAR, NULL, 0);
> - ok(ret > 0, "GetRoleTextW doesn't return length on NULL buffer, got %d\n", ret);
> - ret = GetRoleTextW(ROLE_SYSTEM_TITLEBAR, NULL, 1);
> - ok(ret > 0, "GetRoleTextW doesn't return length on NULL buffer, got %d\n", ret);

If you mean that the above tests do test that the returned value is the length
of the string that's not true, ret > 0 doesn't mean anything.
> - /* use bigger buffer */
> - ret = GetRoleTextA(ROLE_SYSTEM_TITLEBAR, NULL, 0);
> - buff = HeapAlloc(GetProcessHeap(), 0, 2*ret);
> - buff[2*ret-1] = '*';
> - ret = GetRoleTextA(ROLE_SYSTEM_TITLEBAR, buff, 2*ret);
> - ok(buff[2*ret-1] == '*', "GetRoleTextA shouldn't fill buffer with NULLs\n");
> - HeapFree(GetProcessHeap(), 0, buff);
> -
> - ret = GetRoleTextW(ROLE_SYSTEM_TITLEBAR, NULL, 0);
> - buffW = HeapAlloc(GetProcessHeap(), 0, 2*ret*sizeof(WCHAR));
> - buffW[2*ret-1] = '*';
> - ret = GetRoleTextW(ROLE_SYSTEM_TITLEBAR, buffW, 2*ret);
> - ok(buffW[2*ret-1] == '*', "GetRoleTextW shouldn't fill buffer with NULLs\n");
> - HeapFree(GetProcessHeap(), 0, buffW);
>
> +}

You are confusing yourself in the above tests. What that 2*ret and
2*ret*sizeof(WCHAR) above values are supposed to mean? Besides you never
check the 'ret' for a presumably expected result.

[2/2] oleacc: implemented GetRoleText[A/W] with tests (try3)

[Nikolay Sivov]

Dmitry Timoshkov wrote:
> "Nikolay Sivov"  wrote:
>
> First of all there is no need to send the configure.ac patch separately.

Ok, but how could I remove auto generated files from patch?
Now I commit 3 times:
1) configure.ac
2) regenerated 'configure'
3) main patch body.

Then I simply don't send the second one. If there's a easier way please 
let me know.
> > - /* zero role number - not documented */
> > - ret = GetRoleTextA(0, NULL, 0);
> > - ok(ret > 0, "GetRoleTextA doesn't return length for zero role
> >
> > number, got %d\n", ret);
> > - ret = GetRoleTextW(0, NULL, 0);
> > - ok(ret > 0, "GetRoleTextW doesn't return length for zero role
> >
> > number, got %d\n", ret);
> > -
> > - /* NULL buffer, return length */
> > - ret = GetRoleTextA(ROLE_SYSTEM_TITLEBAR, NULL, 0);
> > - ok(ret > 0, "GetRoleTextA doesn't return length on NULL buffer,
> >
> > got %d\n", ret);
> > - ret = GetRoleTextA(ROLE_SYSTEM_TITLEBAR, NULL, 1);
> > - ok(ret > 0, "GetRoleTextA doesn't return length on NULL buffer,
> >
> > got %d\n", ret);
> > - ret = GetRoleTextW(ROLE_SYSTEM_TITLEBAR, NULL, 0);
> > - ok(ret > 0, "GetRoleTextW doesn't return length on NULL buffer,
> >
> > got %d\n", ret);
> > - ret = GetRoleTextW(ROLE_SYSTEM_TITLEBAR, NULL, 1);
> > - ok(ret > 0, "GetRoleTextW doesn't return length on NULL buffer,
> >
> > got %d\n", ret);
>
> If you mean that the above tests do test that the returned value is 
> the length
> of the string that's not true, ret > 0 doesn't mean anything.

Actually it does, to prove this I need to add a new module named 
oleaccrc.dll which contains this resources on Win.
I'll send this with my next try.
> > - /* use bigger buffer */
> > - ret = GetRoleTextA(ROLE_SYSTEM_TITLEBAR, NULL, 0);
> > - buff = HeapAlloc(GetProcessHeap(), 0, 2*ret);
> > - buff[2*ret-1] = '*';
> > - ret = GetRoleTextA(ROLE_SYSTEM_TITLEBAR, buff, 2*ret);
> > - ok(buff[2*ret-1] == '*', "GetRoleTextA shouldn't fill buffer
> >
> > with NULLs\n");
> > - HeapFree(GetProcessHeap(), 0, buff);
> > -
> > - ret = GetRoleTextW(ROLE_SYSTEM_TITLEBAR, NULL, 0);
> > - buffW = HeapAlloc(GetProcessHeap(), 0, 2*ret*sizeof(WCHAR));
> > - buffW[2*ret-1] = '*';
> > - ret = GetRoleTextW(ROLE_SYSTEM_TITLEBAR, buffW, 2*ret);
> > - ok(buffW[2*ret-1] == '*', "GetRoleTextW shouldn't fill buffer
> >
> > with NULLs\n");
> > - HeapFree(GetProcessHeap(), 0, buffW);
> >
> > +}
>
> You are confusing yourself in the above tests. What that 2*ret and
> 2*ret*sizeof(WCHAR) above values are supposed to mean? Besides you never
> check the 'ret' for a presumably expected result.

The purpose of this is to check does GetRoleText change buffer after 
returned string (fill with NULL for example) or not.

[2/2] oleacc: implemented GetRoleText[A/W] with tests (try3)

[Dmitry Timoshkov]

"Nikolay Sivov"  wrote:
> > First of all there is no need to send the configure.ac patch separately.
>
> Ok, but how could I remove auto generated files from patch?
> Now I commit 3 times:
> - configure.ac
> - regenerated 'configure'
> - main patch body.
>
> Then I simply don't send the second one. If there's a easier way please 
> let me know.

Use git update-index to explicitly mark the files to commit, then call
git commit.
> > If you mean that the above tests do test that the returned value is 
> > the length
> > of the string that's not true, ret > 0 doesn't mean anything.
>
> Actually it does,

It doesn't. ret > 0 just means that you get a non zero value. Is it 5,
20, 1000? Does it match an actually returned string length?
> to prove this I need to add a new module named 
> oleaccrc.dll which contains this resources on Win.

I don't understand the argument about oleaccrc.dll.
> > You are confusing yourself in the above tests. What that 2*ret and
> > 2*ret*sizeof(WCHAR) above values are supposed to mean? Besides you never
> > check the 'ret' for a presumably expected result.
>
> The purpose of this is to check does GetRoleText change buffer after 
> returned string (fill with NULL for example) or not.

Then use explicit values instead, '*sizeof(WCHAR)' part is really confusing.
The coment about not checking the 'ret' still applies.

[2/2] oleacc: implemented GetRoleText[A/W] with tests (try3)

[Nikolay Sivov]

Dmitry Timoshkov wrote:
> "Nikolay Sivov"  wrote:
> > > First of all there is no need to send the configure.ac patch 
> > > separately.
> >
> > Ok, but how could I remove auto generated files from patch?
> > Now I commit 3 times:
> > - configure.ac
> > - regenerated 'configure'
> > - main patch body.
> >
> > Then I simply don't send the second one. If there's a easier way 
> > please let me know.
>
> Use git update-index to explicitly mark the files to commit, then call
> git commit.

Thanks, I'll try.
> > to prove this I need to add a new module named oleaccrc.dll which 
> > contains this resources on Win.
>
> I don't understand the argument about oleaccrc.dll.

This module contains localized resource strings used by GetRoleText in 
Win. I tried to avoid adding it but it's the only way to make test check 
returned length for each string.

[2/2] oleacc: implemented GetRoleText[A/W] with tests (try3)

[Dmitry Timoshkov]

"Nikolay Sivov"  wrote:
> > > to prove this I need to add a new module named oleaccrc.dll which 
> > > contains this resources on Win.
> >
> > I don't understand the argument about oleaccrc.dll.
>
> This module contains localized resource strings used by GetRoleText in 
> Win. I tried to avoid adding it but it's the only way to make test check 
> returned length for each string.

Is there any reason you can't use lstrlenA/W on the buffer after a successful
GetRoleText invocation and compare it with the value returned by the call with
NULL buffer?

[2/2] oleacc: implemented GetRoleText[A/W] with tests (try3)

[Nikolay Sivov]

Dmitry Timoshkov wrote:
> "Nikolay Sivov"  wrote:
> > > > to prove this I need to add a new module named oleaccrc.dll which 
> > > > contains this resources on Win.
> > >
> > > I don't understand the argument about oleaccrc.dll.
> >
> > This module contains localized resource strings used by GetRoleText 
> > in Win. I tried to avoid adding it but it's the only way to make test 
> > check returned length for each string.
>
> Is there any reason you can't use lstrlenA/W on the buffer after a 
> successful
> GetRoleText invocation and compare it with the value returned by the 
> call with
> NULL buffer?

Hm. No, I think there's no such reason.