[1/2] shlwapi/tests: Add wvnsprintfW comformance test

[Sebastian Lackner]

Hi,

thanks for your contribution. I have a couple of remarks below:

On 06.05.2016 03:55, 谢威 wrote:
> 
> 0001-shlwapi-Add-wvnsprintfW-comformance-test.patch
> 
> 
> From cdaeba38efb4d2edcdff5e6ec7b75fa5b56d52d7 Mon Sep 17 00:00:00 2001
> From: XieWei <xiewei at linuxdeepin.com>
> Date: Fri, 6 May 2016 09:42:05 +0800
> Subject: shlwapi: Add wvnsprintfW comformance test
> 
> Signed-off-by: XieWei <xiewei at linuxdeepin.com>

The Wine project requires all contributors to submit patches with their
full first and last name. If this isn't your full name, please fix it
in your git settings.

> ---
>  dlls/shlwapi/tests/string.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/dlls/shlwapi/tests/string.c b/dlls/shlwapi/tests/string.c
> index ac14db0..7896827 100644
> --- a/dlls/shlwapi/tests/string.c
> +++ b/dlls/shlwapi/tests/string.c
> @@ -68,6 +68,7 @@ static INT     (WINAPIV *pwnsprintfW)(LPWSTR,INT,LPCWSTR, ...);
>  static LPWSTR  (WINAPI *pStrChrNW)(LPCWSTR,WCHAR,UINT);
>  static BOOL    (WINAPI *pStrToInt64ExA)(LPCSTR,DWORD,LONGLONG*);
>  static BOOL    (WINAPI *pStrToInt64ExW)(LPCWSTR,DWORD,LONGLONG*);
> +static INT     (WINAPI *pwvnsprintfW)(LPWSTR,INT,LPCWSTR,__ms_va_list);
>  
>  static int strcmpW(const WCHAR *str1, const WCHAR *str2)
>  {
> @@ -1601,6 +1602,81 @@ static void test_StrCatChainW(void)
>      ok(buf[5] == 'e', "Expected buf[5] = 'e', got %x\n", buf[5]);
>  }
>  
> +static INT _wvnsprintfW_p_wrapper(LPWSTR pdest, INT maxlen, LPCWSTR format, ...)

Please declare the function with WINAPIV calling convention.
Also please note that a "p" or "lp" prefix is usually omitted new Wine code.

> +{
> +    INT ret;
> +    __ms_va_list valist;
> +    __ms_va_start(valist, format);
> +    ret = pwvnsprintfW(pdest, maxlen, format, valist);
> +    __ms_va_end(valist);
> +    return ret;
> +}
> +
> +static void test_wvnsprintfW(void)
> +{
> +    INT ret = 0;
> +    WCHAR buffer[10] = {0};
> +    const WCHAR fmt[] = {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0', 0};
> +    const WCHAR emptyfmt[] = {0};
> +    const WCHAR result1[] = {'1', '2', '3', '4', '5', '6', '7', '8', 0};
> +    const WCHAR result2[] = {'1', '2', '3', '4', '5', '6', '7', '8', '9', 0};
> +    const WCHAR result3[] = {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0', 0};
> +
> +    if (!pwvnsprintfW)
> +    {
> +        win_skip("wvnsprintfW not available\n");
> +        return;
> +    }

Its a matter of taste, but you usually don't need such a check unless you
are aware of a specific Windows version where it does not exist.

> +
> +    /* crashes on wine */
> +    ret = _wvnsprintfW_p_wrapper(NULL, 0, fmt);
> +    ok(broken(ret == 0) || ret == -1 /* win7 */,
> +       "Expected wvnsprintfW to return %d, expected 0 or -1\n", ret);

As you also noticed, those tests will crash on Wine, which means you can't
really add them until the implementation is fixed. You can either move those
tests to your second patch, or change the order of your patches.

Also, I think the current ok() message is a bit misleading. Something like
"Expected wvnsprintfW to return 0 or -1, got %d\n" would be more clear.
This also affects a couple of places below.

> +
> +    if (0)
> +    {
> +         /* crashes on wine and XP and win7 */

Wrong indentation in the line above.

> +        ret = _wvnsprintfW_p_wrapper(NULL, 3/* >0 */, fmt);
> +        ok(ret == -1, "Expected wvnsprintfW to return -1, got %d\n", ret);
> +    }
> +
> +    /* crashes on wine */
> +    ret = _wvnsprintfW_p_wrapper(buffer, 0, NULL);
> +    ok(broken(ret == 0) || ret == -1 /* win7 */,
> +       "Expected wvnsprintfW to return %d, expected 0 or -1\n", ret);
> +
> +    ret = _wvnsprintfW_p_wrapper(buffer, 0, fmt);
> +    ok(broken(ret == 0) || ret == -1 /* win7 */,
> +       "Expected wvnsprintfW to return %d, expected 0 or -1\n", ret);
> +
> +    if (0)
> +    {
> +        /* crashes on wine and XP */
> +        ret = _wvnsprintfW_p_wrapper(buffer, 10, NULL);
> +        ok(ret == -1, "Expected wvnsprintfW to return -1, got %d\n", ret);
> +    }
> +
> +    ret = _wvnsprintfW_p_wrapper(buffer, 1, emptyfmt);
> +    ok(ret == 0, "Expected wvnsprintfW to return 0, got %d\n", ret);

This test fails in Wine, which means it should be marked with todo_wine.

> +
> +    ret = _wvnsprintfW_p_wrapper(buffer, 10, emptyfmt);
> +    ok(ret == 0, "Expected wvnsprintfW to return 0, got %d\n", ret);
> +
> +    ret = _wvnsprintfW_p_wrapper(buffer, sizeof(buffer)/sizeof(buffer[0])-1, fmt);
> +    ok(broken(ret == 8) || ret == -1 /* win7 */,
> +       "Expected wvnsprintfW to return %d, expected 8 or -1\n", ret);
> +    ok(lstrcmpW(buffer, result1) == 0, "Wrong result [%s]\n", wine_dbgstr_w(buffer));

You can omit the "[" and "]" here. wine_dbgstr_w will add quotation marks around the string.

> +
> +    ret = _wvnsprintfW_p_wrapper(buffer, sizeof(buffer)/sizeof(buffer[0]), fmt);
> +    ok(broken(ret == 9) || ret == -1 /* win7 */,
> +       "Expected wvnsprintfW to return %d, expected 9 or -1\n", ret);
> +    ok(lstrcmpW(buffer, result2) == 0, "Wrong result [%s]\n", wine_dbgstr_w(buffer));
> +
> +    ret = _wvnsprintfW_p_wrapper(buffer, sizeof(buffer)/sizeof(buffer[0])+1, fmt);
> +    ok(ret == 10, "Expected wvnsprintfW to return 10, got %d\n", ret);
> +    ok(lstrcmpW(buffer, result3) == 0, "Wrong result [%s]\n", wine_dbgstr_w(buffer));

Those two tests above also fail in Wine, so they should be marked with todo_wine.

> +}
> +
>  START_TEST(string)
>  {
>    HMODULE hShlwapi;
> @@ -1640,6 +1716,7 @@ START_TEST(string)
>    pwnsprintfW = (void *)GetProcAddress(hShlwapi, "wnsprintfW");
>    pStrToInt64ExA = (void *)GetProcAddress(hShlwapi, "StrToInt64ExA");
>    pStrToInt64ExW = (void *)GetProcAddress(hShlwapi, "StrToInt64ExW");
> +  pwvnsprintfW = (void *)GetProcAddress(hShlwapi, "wvnsprintfW");
>  
>    test_StrChrA();
>    test_StrChrW();
> @@ -1687,6 +1764,7 @@ START_TEST(string)
>    test_StrStrNW();
>    test_StrStrNIW();
>    test_StrCatChainW();
> +  test_wvnsprintfW();
>  
>    CoUninitialize();
>  }
> -- 2.7.0
> 
> 
>