Re: [2/2] oleacc: implemented GetRoleText[A/W] with tests (try3)
"Nikolay Sivov" <bunglehead(a)gmail.com> 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. -- Dmitry.
Dmitry Timoshkov wrote: > "Nikolay Sivov" <bunglehead(a)gmail.com> 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.
"Nikolay Sivov" <bunglehead(a)gmail.com> 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. 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. -- Dmitry.
Dmitry Timoshkov wrote: > "Nikolay Sivov" <bunglehead(a)gmail.com> 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. > > 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.
"Nikolay Sivov" <bunglehead(a)gmail.com> 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? -- Dmitry.
Dmitry Timoshkov wrote:
"Nikolay Sivov" <bunglehead(a)gmail.com> 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.
participants (2)
-
Dmitry Timoshkov -
Nikolay Sivov