[PATCH] Some tests for lsa

[Julius Schwartzenberg]

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