Re: [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