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

[Alex Henrie]

2018-01-11 0:45 GMT-07:00 Nikolay Sivov <nsivov at codeweavers.com>:
> 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?

Yes, GetSystemWow64Directory is available on Windows XP and 2003.

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

The return value is consistently E_INVALIDARG in Windows XP through
10. Even though there wasn't a "right" value to return before, there
definitely is now.

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

OK.

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

Here FAILED might make a little more sense, but there are lots of
tests that give the XP and Vista cases separately, and we'd want to
change them all.

>>
>>
>> -
>>      /* 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().

Good catch. In this case, Vista and later return E_INVALIDARG, which
makes sense, but XP and 2003 return E_OUTOFMEMORY, which makes no
sense. We definitely want to test against the Vista+ behavior here and
consider the XP behavior broken.

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

OK. I will resubmit later tonight as two separate patches. About 2/3
of the diff was just getting rid of unnecessary dynamic imports.

Thanks for the review!

-Alex