[PATCH] dpnet: Add Server tests

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

My apologies for the late review. I've been sick the past days and also had to put a lot of attention towards other stuff.

Am 2015-10-13 um 11:05 schrieb Alistair Leslie-Hughes:
> +static const WCHAR localhost[] = {'1','2','7','.','0','.','0','.','1',0};
Minor nitpick: Is there a reason why you use "127.0.0.1" instead of "localhost"?

> +static int destroy_dnt = 0;
What does "dnt" stand for?

> +static HRESULT WINAPI DirectPlayMessageHandler(void *pvUserContext, DWORD dwMessageId, void *pMsgBuffer)
You got rid of CamelCase in the global variables. Please do it here as well and call the function direct_play_message_handler or similar. Also avoid things like dwThisIsADword and pDidIMentionThisIsaPointer.

> +        case DPN_MSGID_INDICATE_CONNECT:
> +            trace("DPN_MSGID_CREATE_PLAYER\n");
Copy-paste mistake.

> +static HRESULT WINAPI DirectPlayClientMsHandler(void *context, DWORD dwMessageId, void *buffer)
"MsHandler"?

> +{
> +    HRESULT hr;
> +
> +    switch(dwMessageId)
> +    {
> +        case DPN_MSGID_ENUM_HOSTS_RESPONSE:
> +        {
> +            DPNMSG_ENUM_HOSTS_RESPONSE *host_msg;
> +            host_msg = (PDPNMSG_ENUM_HOSTS_RESPONSE)buffer;
PDPNMSG_ENUM_HOSTS_RESPONSE -> DPNMSG_ENUM_HOSTS_RESPONSE * please.

> +            memset(&hostdesc, 0, sizeof(DPN_APPLICATION_DESC));
You can use sizeof(hostdesc) instead of sizeof(DPN_APPLICATION_DESC) here.

> +            memcpy(&hostdesc, host_msg->pApplicationDescription, sizeof(DPN_APPLICATION_DESC) );
Same.

> +    hr = CoCreateInstance(&CLSID_DirectPlay8Client, NULL, CLSCTX_INPROC_SERVER, &IID_IDirectPlay8Client, (void **)&client);
> +    ok(hr == S_OK, "CoCreateInstance failed with 0x%x\n", hr);
> +    if(hr != S_OK)
> +        return;
You don't need the if() check here. If CoCreateInstance fails the test already failed and you can just let it crash.

> +    memset( &appdesc, 0, sizeof(DPN_APPLICATION_DESC) );
> +    appdesc.dwSize  = sizeof( DPN_APPLICATION_DESC );
> +    appdesc.guidApplication  = appguid;
> +
> +    connect  = CreateEventA( NULL, TRUE, FALSE, NULL);
> +    hostenum = CreateEventA( NULL, TRUE, FALSE, NULL);
Inconsistent parenthesis placement. It seems the code was copypasted together from other places.

> +    hr = IDirectPlay8Client_Initialize(client, NULL, DirectPlayClientMsHandler, 0);
> +    ok(hr == S_OK, "IDirectPlay8Client_Initialize failed with %x\n", hr);
> +
> +    hr = CoCreateInstance( &CLSID_DirectPlay8Address, NULL, CLSCTX_ALL, &IID_IDirectPlay8Address, (LPVOID*)&host);
> +    ok(hr == S_OK, "IDirectPlay8Address failed with 0x%08x\n", hr);
> +
> +    hr = IDirectPlay8Address_SetSP(host, &CLSID_DP8SP_TCPIP);
> +    ok(hr == S_OK, "IDirectPlay8Address_SetSP failed with 0x%08x\n", hr);
> +
> +    hr = CoCreateInstance( &CLSID_DirectPlay8Address, NULL, CLSCTX_ALL, &IID_IDirectPlay8Address, (LPVOID*)&device);
> +    ok(hr == S_OK, "IDirectPlay8Address failed with 0x%08x\n", hr);
You're using LPCRAP here again.

I think you're not making any productive use of the address object you create here and you're leaking it. Later on you're creating a separate address and overwrite this one.

> +    hr = IDirectPlay8Address_AddComponent(host, DPNA_KEY_HOSTNAME, localhost, sizeof(localhost),
> +                                                         DPNA_DATATYPE_STRING);
> +    ok(hr == S_OK, "IDirectPlay8Address failed with 0x%08x\n", hr);
This is obviously something for a different patch / test, but do you know how the various components play together with the service provider? I'd assume that a Serial or Modem service provider requires different components than the TCPIP service provider.

> @@ -109,5 +269,9 @@ START_TEST(server)
>  
>      create_server();
>  
> +    create_client();
> +
> +    cleanup();
It is difficult to see what is going on by just reading the main function of the test. create_foo() and cleanup() suggest that they create / destroy helper objects, but all of them do a serious amount of testing. You cannot disable the tests separately without breaking the entire test.

I'd suggest to have helper functions that create server and client and one enumeration test, a test for connecting to a server, ..., and call the creation functions in each individual test, similar to how e.g. create_device() is working in various d3d tests. This makes this patch more complicated, but it will be very helpful in the long run when more tests are added that need fresh objects. Sooner or later you'll also want to be able to disable individual tests for debugging and / or faster execution times if you work on a new test.

Cheers,
Stefan

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJWJSi7AAoJEN0/YqbEcdMwz3sP/ArVZHXjUMVD45/bGbqJ7ZIX
oKYP/GTHEBVJtA3EICapjIhN/92intcsQxtjYy0sK5ol4U2Yeg3HDon/PTSihEgC
oK0cr1Jf8Z1lN7+kuIgDc4agMrW7UxAhBuJ4CqMFsOkLFbuem30VXnFj+/1VhCRC
zYqhirihyFyH+8ac4g6kQLJBnPP5gtJ3YKbDYoE+LM0wF1eIUTfvcEuMfxp8tF4s
gO4k7fpjvOzrf7J/z1MK/J+1OUQxvyGPp+HikZ8ZUXD16A6Dqwc7nATL3f+wMV85
JICLuXiyqV7slnkb0A2rIbMIM2Y5cF7pHAjz08thGYSWno+wJFXAcGtH12JPT1uD
61XQfh/lPZKXgeJRKwpwq3meztVVxzb1V4mfQsNWvi81nzEnu2VRvs8PZqAyR09E
vU7yFNXq/cDikRN9NrtX/tTv7ZaHwM1HwZVqGzA+mB4vlQ8pApzycYLQh1sOVQRm
g39eSq2qNx/7pk53G7Chxm/8sQ3Xa7s5D0F7PqvDTxlXA0xo8Q1r3+zH/NQDGvS8
UsKz0br6rRFdArpyzv9VUxF2Ynn+xig25JPk0ofbJx90H677kAJJ2QMyik4DOWGb
sojlCQ0n9W5zR1kY3mbNWxa6mFiBsnexvoJ+eSiqnybkc+b+SDz1vJrSsOcKp1ys
XUIIuCpcMtvVp85zcZG3
=/XJU
-----END PGP SIGNATURE-----