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

[Alex Henrie]

2017-09-08 2:12 GMT-06:00 Nikolay Sivov <bunglehead at gmail.com>:
> 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?

I was originally going to add tests for shortcut files, and I wanted
to see whether FolderItem_IsLink returns TRUE for a .lnk file that is
not actually a shortcut. You're right that empty files would be good
enough for these tests though.

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

Sure, that sounds reasonable.

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

Just trying to be thorough; VT_ERROR returns the parent folder
regardless of the numeric value. Is it OK to only test 0?

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

OK, I'll remove the null checks before string comparisons. Thanks
again for the feedback.

-Alex