Re: [PATCH] shell32/tests: Drop shell folder test workarounds for Windows <= 2000

[Nikolay Sivov]

On 1/11/2018 10:15 AM, Alex Henrie wrote:
>  hmod = GetModuleHandleA("kernel32.dll");
>  pIsWow64Process = (void*)GetProcAddress(hmod, "IsWow64Process");
> - pGetSystemWow64DirectoryW = (void*)GetProcAddress(hmod, "GetSystemWow64DirectoryW");

Will this work on XP?
> @@ -278,10 +237,10 @@ static void test_ParseDisplayName(void)
>      }
>  MultiByteToWideChar(CP_ACP, 0, cNonExistDir1A, -1, cTestDirW, MAX_PATH);
> - hr = IShellFolder_ParseDisplayName(IDesktopFolder,
> - hr = IShellFolder_ParseDisplayName(IDesktopFolder,
>     NULL, NULL, cTestDirW, NULL, &newPIDL, 0);
> - ok((hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) || (hr == E_FAIL),
> -
>    "ParseDisplayName returned %08x, expected 80070002 or E_FAIL\n", hr);
> -
>
> ok(hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND),
> -
>    "ParseDisplayName returned %08x, expected 0x80070002\n", hr);
>
> res = GetFileAttributesA(cNonExistDir2A);
>    if(res != INVALID_FILE_ATTRIBUTES)
>
> @@ -291,21 +250,15 @@ static void test_ParseDisplayName(void)
>      }
>  MultiByteToWideChar(CP_ACP, 0, cNonExistDir2A, -1, cTestDirW, MAX_PATH);
> - hr = IShellFolder_ParseDisplayName(IDesktopFolder,
> - hr = IShellFolder_ParseDisplayName(IDesktopFolder,
>     NULL, NULL, cTestDirW, NULL, &newPIDL, 0);
> - ok((hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) || (hr == E_FAIL) || (hr == E_INVALIDARG),
> -
>    "ParseDisplayName returned %08x, expected 80070002, E_FAIL or E_INVALIDARG\n", hr);
> - todo_wine ok(hr == E_INVALIDARG, "ParseDisplayName returned %08x, expected E_INVALIDARG\n", hr);

Maybe it's better to use FAILED() in such cases.
>  IShellFolder_Release(psfSystemDir);
>
> @@ -586,23 +527,19 @@ if (0)
>              {
>                  IPersist *pp;
>                  hr = IShellFolder_QueryInterface(psfChild, &IID_IPersist, (void**)&pp);
> -
>            ok(hr == S_OK ||
> -
>               broken(hr == E_NOINTERFACE), /* Win9x, NT4, W2K */
> -
>               "Got 0x%08x\n", hr);
> -
>            ok(hr == S_OK, "Got 0x%08x\n", hr);
>            if(SUCCEEDED(hr))
>            {
>                CLSID id;
>                hr = IPersist_GetClassID(pp, &id);
>                ok(hr == S_OK, "Got 0x%08x\n", hr);
> -
>                /* CLSID_ShellFSFolder on some w2k systems */
> -
>                ok(IsEqualIID(&id, &CLSID_ShellDocObjView) || broken(IsEqualIID(&id, &CLSID_ShellFSFolder)),
> -
>                    "Unexpected classid %s\n", wine_dbgstr_guid(&id));
> -
>                ok(IsEqualIID(&id, &CLSID_ShellDocObjView), "Unexpected classid %s\n", wine_dbgstr_guid(&id));
>                IPersist_Release(pp);
>            }

Please make this and similar cases more linear, we don't need
conditional block anymore.
>              IShellFolder_Release(psfChild);
>          }
> -
>        pILFree(pidl);
> -
>        ILFree(pidl);
>    }
>    DeleteFileA(pathA);
> }
>
> @@ -623,12 +560,10 @@ if (0)
>          {
>              hr = IShellFolder_BindToObject(psfDesktop, pidl, NULL, &IID_IShellFolder, (void**)&psfChild);
>              ok(hr == E_FAIL || /* Vista+ */
> -
>           hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) || /* XP, W2K3 */
> -
>           hr == E_INVALIDARG || /* W2K item in top dir */
> -
>           broken(hr == S_OK), /* Win9x, NT4, W2K */
> -
>           hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), /* XP, W2K3 */
>           "Got 0x%08x\n", hr);

Same here, FAILED() might be better.
> - /* Some tests for IShellFolder::SetNameOf */
> - if (pSHGetFolderPathAndSubDirA)
> - hr = SHBindToParent(pidlTestFile, &IID_IShellFolder, (VOID**)&psfPersonal, &pidlLast);

Here and later on, since you're changing the line anyway, please use
actual type plus a space like (void **), same for other pointer types.
>  /* SHBindToParent fails, if called with a NULL PIDL. */
> - hr = pSHBindToParent(NULL, &IID_IShellFolder, (VOID**)&psfPersonal, &pidlLast);
> - hr = SHBindToParent(NULL, &IID_IShellFolder, (void**)&psfPersonal, &pidlLast);
> ok (hr != S_OK, "SHBindToParent(NULL) should fail!\n");

That one should actually be FAILED().

Overall I think it's good, but too large for a single change. I would
start with getting rid of dynamic imports as a first patch, leaving
broken() cleanup and special version dependent cases for later.