Re: [PATCH 1/4] kernel32/tests: Add simple UTF-7 encoding tests.

[Sebastian Lackner]

On 20.10.2014 06:41, Alex Henrie wrote:
> ---
>  dlls/kernel32/tests/codepage.c | 134 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 134 insertions(+)
>
> diff --git a/dlls/kernel32/tests/codepage.c b/dlls/kernel32/tests/codepage.c
> index 8423c75..684ede1 100644
> --- a/dlls/kernel32/tests/codepage.c
> +++ b/dlls/kernel32/tests/codepage.c
> @@ -412,6 +412,138 @@ static void test_string_conversion(LPBOOL bUsedDefaultChar)
>      ok(GetLastError() == 0xdeadbeef, "GetLastError() is %u\n", GetLastError());
>  }
>
> +static void test_utf7_encoding(void)
> +{
> +    struct simple_test
> +    {
> +        WCHAR utf16[1024];
> +        int utf16_len;
> +        char utf7[1024];
> +        int utf7_len;

1024 is unnecessary big. A couple of bytes would be sufficient for all these tests.
> +    };
> +
> +    static const struct simple_test simple_tests[] = {

No need to give that struct a name. You can simply do:

static const struct
{
    ...
}
simple_tests[] =
{
    ...
}
> +        /* tests some valid UTF-16 */
> +        {
> +            {0x4F60,0x597D,0x5417,0},
> +            4,
> +            "+T2BZfVQX-",
> +            11
> +        },
> +        /* tests some invalid UTF-16 */
> +        /* (stray lead surrogate) */

That looks ugly, either put it in a single line or use:

/* line1
 * line2 */
> +        {
> +            {0xD801,0},
> +            2,
> +            "+2AE-",
> +            6
> +        },
> +        /* tests some more invalid UTF-16 */
> +        /* (codepoint does not exist) */
> +        {
> +            {0xFF00,0},
> +            2,
> +            "+/wA-",
> +            6
> +        }
> +    };
> +
> +    int i;
> +
> +    for (i = 0; i < sizeof(simple_tests) / sizeof(simple_tests[0]); i++)
> +    {
> +        char c_buffer[1024];
> +        WCHAR w_buffer[1024];

Same as above, way too big.
> +        int len;
> +
> +        c_buffer[sizeof(c_buffer) - 1] = 0;
> +        w_buffer[sizeof(w_buffer) / sizeof(WCHAR) - 1] = 0;

It probably makes more sense to initialize the buffers before each test, instead of one time at the beginning.
> +
> +        /* test string conversion with srclen=-1 */
> +        memset(c_buffer, '#', sizeof(c_buffer) - 1);
> +        SetLastError(0xdeadbeef);
> +        len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, sizeof(c_buffer), NULL, NULL);
> +        if (GetLastError() == ERROR_CALL_NOT_IMPLEMENTED)
> +        {
> +            skip("UTF-7 encoding not implemented\n");
> +            return;
> +        }

At my opinion it would be better to do one basic test at the beginning, and then abort immediately, instead of doing that inside of the for loop, if its not too complicated.
> +        ok(len == simple_tests[i].utf7_len &&
> +           strcmp(c_buffer, simple_tests[i].utf7) == 0 &&
> +           c_buffer[len] == '#',
> +           "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len);
> +        ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError());
> +
> +        /* test string conversion with srclen=-2 */
> +        memset(c_buffer, '#', sizeof(c_buffer) - 1);
> +        SetLastError(0xdeadbeef);
> +        len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -2, c_buffer, sizeof(c_buffer), NULL, NULL);
> +        ok(len == simple_tests[i].utf7_len &&
> +           strcmp(c_buffer, simple_tests[i].utf7) == 0 &&
> +           c_buffer[len] == '#',
> +           "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len);
> +        ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError());

You are testing a lot of things in a single ok(...) block. It will be very difficult to find out whats wrong if we get test failures in the future. I think it would be better to split that into multiple checks, probably with some more meaningful message about what is wrong (wrong len, wrong content, ...)
> +
> +        /* test string conversion with dstlen=len-1 */
> +        memset(c_buffer, '#', sizeof(c_buffer) - 1);
> +        SetLastError(0xdeadbeef);
> +        len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, simple_tests[i].utf7_len - 1, NULL, NULL);
> +        ok(len == 0 &&
> +           memcmp(c_buffer, simple_tests[i].utf7, simple_tests[i].utf7_len - 1) == 0 &&
> +           c_buffer[simple_tests[i].utf7_len - 1] == '#',
> +           "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len);
> +        ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, "error=%x\n", GetLastError());
> +
> +        /* test string conversion with dstlen=len */
> +        memset(c_buffer, '#', sizeof(c_buffer) - 1);
> +        SetLastError(0xdeadbeef);
> +        len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, simple_tests[i].utf7_len, NULL, NULL);
> +        ok(len == simple_tests[i].utf7_len &&
> +           strcmp(c_buffer, simple_tests[i].utf7) == 0 &&
> +           c_buffer[len] == '#',
> +           "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len);
> +        ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError());
> +
> +        /* test string conversion with dstlen=len+1 */
> +        memset(c_buffer, '#', sizeof(c_buffer) - 1);
> +        SetLastError(0xdeadbeef);
> +        len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, simple_tests[i].utf7_len + 1, NULL, NULL);
> +        ok(len == simple_tests[i].utf7_len &&
> +           strcmp(c_buffer, simple_tests[i].utf7) == 0 &&
> +           c_buffer[len] == '#',
> +           "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len);
> +        ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError());
> +
> +        /* test dry run with dst=NULL and dstlen=0 */
> +        memset(c_buffer, '#', sizeof(c_buffer));
> +        SetLastError(0xdeadbeef);
> +        len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, NULL, 0, NULL, NULL);
> +        ok(len == simple_tests[i].utf7_len &&
> +           c_buffer[0] == '#',
> +           "simple_test failure i=%i len=%i\n", i, len);
> +        ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError());
> +
> +        /* test dry run with dst!=NULL and dstlen=0 */
> +        memset(c_buffer, '#', sizeof(c_buffer) - 1);
> +        SetLastError(0xdeadbeef);
> +        len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, 0, NULL, NULL);
> +        ok(len == simple_tests[i].utf7_len &&
> +           c_buffer[0] == '#',
> +           "simple_test failure i=%i len=%i\n", i, len);
> +        ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError());
> +
> +        /* all simple utf16-to-utf7 tests can be reversed to make utf7-to-utf16 tests */
> +        memset(w_buffer, '#', sizeof(w_buffer) - sizeof(WCHAR));
> +        SetLastError(0xdeadbeef);
> +        len = MultiByteToWideChar(CP_UTF7, 0, simple_tests[i].utf7, -1, w_buffer, sizeof(w_buffer) / sizeof(WCHAR));
> +        todo_wine ok(len == simple_tests[i].utf16_len &&
> +           memcmp(w_buffer, simple_tests[i].utf16, len * sizeof(WCHAR)) == 0 &&
> +           w_buffer[len] == 0x2323,
> +           "simple_test failure i=%i dst=%s len=%i\n", i, wine_dbgstr_w(w_buffer), len);
> +        todo_wine ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError());

Why do you use todo_wine when the whole test block is skipped? Moreover, it should probably go into the later patches which add the MultibyteToWideChar tests.
> +    }
> +}
> +
>  static void test_undefined_byte_char(void)
>  {
>      static const struct tag_testset {
> @@ -618,6 +750,8 @@ START_TEST(codepage)
>      test_string_conversion(NULL);
>      test_string_conversion(&bUsedDefaultChar);
>
> +    test_utf7_encoding();
> +
>      test_undefined_byte_char();
>      test_threadcp();
>  }