Re: winspool/tests: fix broken test for GetPrinterDriver

[Dmitry Timoshkov]

"Detlef Riekenberg" <wine.dev(a)web.de> wrote:
> @@ -939,36 +939,36 @@ static void test_GetPrinterDriver(void)
>
> for (level = -1; level <= 7; level++)
>      {
> -        SetLastError(0xdeadbeef);
> +        SetLastError(MAGIC_DEAD);

Please don't change this, stick to the style used everywhere else, i.e. use
0xdeadbeef directly.
> needed = (DWORD)-1;
>          ret = GetPrinterDriver(hprn, NULL, level, NULL, 0, &needed);
> -        ok(!ret, "level %d: GetPrinterDriver should fail\n", level);
> -        if (level >= 1 && level <= 6)
> -        {
> -            ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, "wrong error %ld\n", GetLastError());
> -            ok(needed > 0,"not expected needed buffer size %ld\n", needed);
> -        }
> -        else
> -        {
> -            ok(GetLastError() == ERROR_INVALID_LEVEL, "wrong error %ld\n", GetLastError());
> -            ok(needed == (DWORD)-1,"not expected needed buffer size %ld\n", needed);
> -            continue;
> -        }
> +        ok(!ret && ((GetLastError() == ERROR_INVALID_LEVEL) ||
> +            (GetLastError() == ERROR_INSUFFICIENT_BUFFER) ||
> +            (GetLastError() == ERROR_OUTOFMEMORY)), "%d: returned %d with %ld " \
> +            "(expected '0' with: ERROR_INVALID_LEVEL, ERROR_INSUFFICIENT_BUFFER" \
> +            "or ERROR_OUTOFMEMORY)\n", level, ret, GetLastError());
> +
> +        if (GetLastError() != ERROR_INSUFFICIENT_BUFFER) continue;
> +
> +        ok(needed > 0,"%d: returned size %ld\n", level, needed);
> +        if (needed == 0) continue;

I object to this change. The test needs a clear indication whether it's an invalid
or a not supported parameter. Do not remove if {} else {}, that's an essential part
of the test. A comment explaining the change would be also nice.
> ret = GetPrinterDriver(hprn, NULL, level, buf, needed, &filled);
> -        ok(ret, "level %d: GetPrinterDriver error %ld\n", level, GetLastError());
> -        ok(needed == filled, "needed %ld != filled %ld\n", needed, filled);
> +        ok( ret, "%d: returned %d with %ld (expected '!= 0')\n",
> +            level, ret, GetLastError());

"expected '!= 0'" is wrong, ret is BOOL, so "expected TRUE" or better leave it as
it is now, error message already implies that the API has failed.
> -            ok(di_2->cVersion >= 0 && di_2->cVersion <= 3, "di_2->cVersion = %ld\n", di_2->cVersion);
> +            ok((di_2->cVersion >= 0 && di_2->cVersion <= 3) ||
> +               (di_2->cVersion == 0x0400), "di_2->cVersion = %ld\n", di_2->cVersion);

A comment explaining which windows version returns 0x400 (which contradicts MSDN)
would be helpful.
> ok(di_2->pName != NULL, "not expected NULL ptr\n");
>              ok(di_2->pEnvironment != NULL, "not expected NULL ptr\n");
>              ok(di_2->pDriverPath != NULL, "not expected NULL ptr\n");
> @@ -977,15 +977,15 @@ static void test_GetPrinterDriver(void)
>
> trace("cVersion %ld\n", di_2->cVersion);
>              trace("pName %s\n", di_2->pName);
> -            calculated += strlen(di_2->pName) + 1;
> +            calculated += lstrlenA(di_2->pName) + 1;

If you want to replace strlen by lstrlenA (why?) that should be a separate
patch. Same for below changes.

-- 
Dmitry.