Re: [PATCH 1/2] ntdll/tests: Create a test for RtlGetNtProductType().

[Zebediah Figura]

Hello Serge,

On 2/28/20 5:10 AM, Serge Gautherie wrote:
> @@ -3673,6 +3681,46 @@ static void test_LdrRegisterDllNotification(void)
>      pLdrUnregisterDllNotification(cookie);
>  }
>
> +static void test_RtlGetNtProductType(void)
> +{
> +    DWORD type;
> +
> +    if (!pRtlGetNtProductType)
> +    {
> +        win_skip("RtlGetNtProductType is not available\n");
> +        return;
> +    }
> +
> +    /* NULL is not a special value for this function */

This comment seems unnecessary, given the code.
> +#ifdef TEST_WITH_SEH
> +    /* With SEH, address is different, but it still crashes on Windows:
> +     * 'rtl: unhandled exception c0000005 at 00000001'
> +     */

This seems like an internal detail, and so not interesting enough to
comment.
> +    __TRY
> +    {
> +        BOOLEAN ret;
> +
> +        ret = pRtlGetNtProductType(NULL);
> +
> +        if (ret)
> +            ok(FALSE, "RtlGetNtProductType(NULL) succeeded\n");
> +        else
> +            ok(FALSE, "RtlGetNtProductType(NULL) did not crash\n");

This branch seems pointless.
> +    }
> +    __EXCEPT(EXCEPTION_EXECUTE_HANDLER)
> +    {
> +        /* As expected */

This comment seems unnecessary.
> +        ok(TRUE, "\n");

And ok(TRUE) does nothing, so there's no point in having it.
> +    }
> +    __ENDTRY
> +#endif

Does a program actually depend on this function crashing? If not, I
suspect it would be better to leave off this test entirely, or add
something like the following, as is done elsewhere in Wine tests:

if (0)
{
    /* crashes on Windows */
    ret = pRtlGetNtProductType(NULL);
}
> +
> +    type = 0xdeadbeef;
> +    ok(pRtlGetNtProductType(&type), "RtlGetNtProductType failed\n");
> +    ok(type >= VER_NT_WORKSTATION && type <= VER_NT_SERVER, "unknown type %u\n", type);
> +    trace("NtProductType: %u\n", type);
> +}
> +

Is this trace() really worth having? We already have too many...