Re: [PATCH 1/3] shell32/tests: Add more tests for FolderItems.

[Nikolay Sivov]

On 21.07.2016 7:45, 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, "file_defs[%d]: CreateDirectory failed: %08x\n", i, GetLastError());
> -
>            break;
> -
> -
>        case SHORTCUT:
> -
>            sl = NULL;
> -
>            r = CoCreateInstance(&CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER, &IID_IShellLinkA, (void**)&sl);
> -
>            ok(SUCCEEDED(r), "file_defs[%d]: CoCreateInstance failed: %08x\n", i, r);
> -
>            ok(!!sl, "sl is null\n");
> -
> -
>            shortcut = NULL;
> -
>            r = IShellLinkA_QueryInterface(sl, &IID_IPersistFile, (void**)&shortcut);
> -
>            ok(r == S_OK, "file_defs[%d]: IShellLink::QueryInterface failed: %08x\n", i, r);
> -
>            ok(!!shortcut, "file_defs[%d]: shortcut is null\n", i);
> -
> -
>            MultiByteToWideChar(CP_ACP, 0, file_defs[i].name, -1, wstr, sizeof(wstr)/sizeof(WCHAR));
> -
>            r = IPersistFile_Save(shortcut, wstr, FALSE);
> -
>            ok(r == S_OK, "file_defs[%d]: IPersistFile::Save failed: %08x\n", i, r);
> -
> -
>            IPersistFile_Release(shortcut);
> -
>            IShellLinkA_Release(sl);

If you're only interested in IPersistFile you can request it directly
when calling CoCreateInstance().
> - r = FolderItems_QueryInterface(items, &IID_FolderItems2, (void**)&items2);
> - ok(r == S_OK || broken(E_NOINTERFACE) /* xp and later */, "FolderItems::QueryInterface failed: %08x\n", r);
> - ok(!!items2 || broken(!items2) /* xp and later */, "items2 is null\n");

Second ok() is a bit meaningless, because it's expected to be consistent
with previous return value test. I think it could be placed into if (r
== S_OK), with broken() removed.
> -
> -
>    bstr = NULL;
> -
>    r = FolderItem_get_Path(item, &bstr);
> -
>    ok(r == S_OK, "file_defs[%d]: FolderItem::get_Path failed: %08x\n", i, r);
> -
>    ok(!!bstr, "file_defs[%d]: bstr is null\n", i);
> -
> -
>    bstr2 = NULL;
> -
>    r = FolderItem_get_Path(item, &bstr2);
> -
>    ok(r == S_OK, "file_defs[%d]: FolderItem::get_Path failed: %08x\n", i, r);
> -
>    ok(bstr2 != bstr, "file_defs[%d]: bstr and bstr2 are the same\n", i);
> -
>    SysFreeString(bstr2);

This is a regular convention for [out] BSTR arguments. It only makes
sense to explicitly test it if it differs from expected behavior.
> -
> - V_I4(&var) = sizeof(file_defs)/sizeof(file_defs[0]);
> - item = NULL;
> - r = FolderItems_Item(items, var, &item);
>
> +todo_wine
> - ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
> - ok(!item, "item is not null\n");
> - if (0) /* crashes on xp */
>    {
>        r = FolderItems_get_Application(items, NULL);
>
> @@ -422,12 +623,20 @@ todo_wine
>              ok(r == E_INVALIDARG, "expected E_INVALIDARG, got %08x\n", r);
>          }
> -
>    verbs = (FolderItemVerbs*)0xdeadbeef;
>    r = FolderItems3_get_Verbs(items3, &verbs);
>
> todo_wine
>          ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
>          ok(!verbs, "verbs is not null\n");
>      }
> - for (i = 0; i < sizeof(file_defs)/sizeof(file_defs[0]); i++)
> - {
> -
>    if (file_defs[i].type == DIRECTORY)
> -
>        RemoveDirectoryA(file_defs[i].name);
> -
>    else
> -
>        DeleteFileA(file_defs[i].name);
> - }

Could this cleanup be merged with one of the loops you have earlier?
>