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...