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?
>