[PATCH] Some tests for lsa
Julius Schwartzenberg
julius.schwartzenberg at gmail.com
Sun Feb 19 04:27:21 CST 2017
Sebastian Lackner wrote:
> On 17.02.2017 00:29, Julius Schwartzenberg wrote:
>> ---
>> dlls/secur32/tests/Makefile.in | 1 +
>> dlls/secur32/tests/lsa.c | 206 +++++++++++++++++++++++++++++++++++++++++
>> include/ntsecapi.h | 50 ++++++++++
>> 3 files changed, 257 insertions(+)
>> create mode 100644 dlls/secur32/tests/lsa.c
>
> Thanks for providing the updated version. Unfortunately there are still various
> problems left, see the comments below.
Thanks a lot for the review!
> Patches should have a subject like "component: title". In this case something
> like "secur32/tests: Add some tests for lsa functions." might be appropriate.
Should I put that in the commit message? I think git will put it as the
subject as well then.
> It is preferred to add tests to existing files. You can still add your own
> copyright of course. Splitting in separate files is usually done when there
> are sufficient tests.
I don't care about copyright, but Bruno asked for that. I added the
files to this file because these functions are also defined in lsa.c and
this would be consistent with the other tests. Would an existing file be
more suitable?
>> +{
>> + NTSTATUS status;
>> + HANDLE lsa_handle;
>> +
>
> There are still some lines (see for example above) with trailing whitespace in your patch.
That's bad yeah, I should really take care of that!
>> + status = LsaConnectUntrusted(&lsa_handle);
>> +
>> + ok(status == STATUS_SUCCESS, "LsaConnectUntrusted(valid_handle) returned 0x%x\n", status);
>
> You should try to avoid leaks and always close handles at the end of each test - in
> this case with LsaDeregisterLogonProcess().
I missed this one yep.
>> +}
>> +
>> +static void test_LsaLookupAuthenticationPackage_null_handle(void)
>> +{
>> + NTSTATUS status;
>> + LSA_STRING package_name;
>> + ULONG auth_package;
>> + char name[] = MICROSOFT_KERBEROS_NAME_A;
>> + package_name.Buffer = name;
>> + package_name.Length = sizeof(MICROSOFT_KERBEROS_NAME_A) - 1;
>> +
>> + status = LsaLookupAuthenticationPackage(NULL, &package_name, &auth_package);
>> +
>> + todo_wine ok(status == STATUS_INVALID_HANDLE || status == STATUS_INVALID_CONNECTION,
>
> Please always try to document where you observed which status value. Depending on what you
> consider more correct, you could mark the other value as broken() and a comment which Windows
> versions are affected.
The difference is between older and newer versions (<=6.0 and >=6.1). I
learned that from the testbot. It seems they made the messages more
precise in later versions. Should I just add a comment?
>> + LsaDeregisterLogonProcess(&lsa_handle);
>
> You should remove the & here. Also, you might want to check the result of each function,
> even if they are also tested separately.
This is something I still mix up from time to time. If there are any
recommendations on a good IDE which could point such things out, I'd be
glad to try it.
>> +
>> + ok(status == STATUS_SUCCESS,
>> + "LsaLookupAuthenticationPackage(valid_handle, \"Kerberos\", ...) returned 0x%x\n", status);
>
> You might also want to test the auth_package output parameter here.
Bruno suggested this also, but I hoped to add more tests for that in a
second patch. Maybe I should try to get at least a basic test going here
already though.
I intend to send a better patch soon. I am sending to wine-patches each
time, so that Marvin runs the tests, but correct me if I should send to
wine-devel first as long as it's still going through the review cycle.
Of course if others have remarks too, I'll be glad to incorporate any
feedback I get. Thanks!
Best regards,
Julius
More information about the wine-devel
mailing list