Re: [PATCH 1/3] shell32/tests: Add tests for FolderItems_Item and FolderItems_get_Count.

[Nikolay Sivov]

On 08.09.2017 8:29, Alex Henrie wrote:
> - for (i = 0; i < sizeof(file_defs)/sizeof(file_defs[0]); i++)
> - {
> -
>    switch (file_defs[i].type)
> -
>    {
> -
>        case DIRECTORY:
> -
>            r = CreateDirectoryA(file_defs[i].name, NULL);
> -
>            ok(r, "CreateDirectory failed: %08x\n", GetLastError());
> -
>            PathCombineA(buf, file_defs[i].name, "foo.txt");
> -
>            file = CreateFileA(buf, 0, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
> -
>            ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());
> -
>            CloseHandle(file);
> -
>            break;
> -
> -
>        case EMPTY:
> -
>            file = CreateFileA(file_defs[i].name, 0, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
> -
>            ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());
> -
>            CloseHandle(file);
> -
>            break;
> -
> -
>        case RANDOM:
> -
>            file = CreateFileA(file_defs[i].name, GENERIC_WRITE, 0, NULL,
> -
>                               CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
> -
>            ok(file != INVALID_HANDLE_VALUE, "CreateFile failed: %08x\n", GetLastError());
> -
>            for (j = 0; j < sizeof(buf); j++)
> -
>                buf[j] = rand() % 256;
> -
>            r = WriteFile(file, buf, sizeof(buf), &dwcount, NULL);
> -
>            ok(r, "WriteFile failed: %08x\n", GetLastError());
> -
>            CloseHandle(file);
> -
>            break;
> -
>    }
> - }

Why would it matter if file is empty or not? And if it does for some
reason, why filling it with random content?
> - MultiByteToWideChar(CP_ACP, 0, file_defs[0].name, -1, wstr, MAX_PATH);
> - V_VT(&var) = VT_BSTR;
> - V_BSTR(&var) = SysAllocString(wstr);

I understand why you have test names as single byte strings, but this
construct is used more than once, so maybe add a helper for that. Could
be similar to _bstr_() in msxml test, but without global pointer array.
> - for (i = -10; i < 10; i++)
> - {
> -
>    V_VT(&var) = VT_ERROR;
> -
>    V_ERROR(&var) = i;

VT_ERROR is used for optional arguments, what does [-10,10) range mean?
I think it's enough to test some HRESULT-style failure code.
> -
>    ok(!!bstr, "bstr is null\n");
> -
>    ok(!lstrcmpW(bstr, cur_dir),
> -
>       "expected %s, got %s\n", wine_dbgstr_w(cur_dir), wine_dbgstr_w(bstr));

First check seems redundant.