Re: quartz/tests: Add tests for IVMRMonitorConfig and IVMRMonitorConfig9 interface (try 2)

[Nikolay Sivov]

On 11/25/2013 10:23, Sebastian Lackner wrote:
> - hr = CoCreateInstance(&CLSID_VideoRenderer, NULL, CLSCTX_INPROC_SERVER,
> -
>                      &IID_IUnknown, (LPVOID*)&pVideoRenderer);
> - ok(hr != S_OK || pVideoRenderer != NULL, "CoCreateInstance returned S_OK, but pVideoRenderer is NULL.\n");
> - if (hr != S_OK || !pVideoRenderer)
> - {
> -
>    skip("VideoRenderer is not available, skipping QI test.\n");
> -
>    return;
> - }
> -
> QI_SUCCEED(pVideoRenderer, IID_IBaseFilter, pBaseFilter);
> RELEASE_EXPECT(pBaseFilter, 1);

It's enough to test for return value, also if wine supports this class 
this should be win_skip(). What's a point of separate QI here? You could 
CoCreateInstance with IID_IBaseFilter.
> - hr = IUnknown_QueryInterface(pVMR7, &IID_IVMRMonitorConfig, (LPVOID*)&pMonitorConfig);
> - ok(hr == S_OK, "IUnknown_QueryInterface returned %x.\n", hr);
> - ok(pMonitorConfig != NULL, "pMonitorConfig is NULL.\n");
> - if (!pMonitorConfig) goto out;

Is it really possible to fail here?
> -
>    ok(info[numdev].guid.pGUID != &info[numdev].guid.GUID || memcmp(&info[numdev].guid.GUID, &max_guid, sizeof(max_guid)) != 0,
> -
>            "GetAvailableMonitors returned info[%d].GUID = {FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF}, expected any other value.\n", numdev);

It's shorter to just print resulting guid.
> - /* check that result is filled out, we do not check if the values actually make any sense */
> - while (numdev--)
> - {
> -
>    ok(info[numdev].uDevID != (UINT)-1,
> -
>            "GetAvailableMonitors returned info[%d].uDevID = -1, expected != -1.\n", numdev);
> -
>    ok(memcmp(&info[numdev].rcMonitor, &max_rect, sizeof(max_rect)) != 0,
> -
>            "GetAvailableMonitors returned info[%d].rcMonitor = {-1, -1, -1, -1}, expected any other value.\n", numdev);
> -
>    ok(info[numdev].hMon != (HMONITOR)0 && info[numdev].hMon != (HMONITOR)-1,
> -
>            "GetAvailableMonitors returned info[%d].hMon = %p, expected != 0 and != -1.\n", numdev, info[numdev].hMon);
> -
>    ok(info[numdev].dwFlags != (DWORD)-1,
> -
>            "GetAvailableMonitors returned info[%d].dwFlags = -1, expected != -1.\n", numdev);
> -
>    ok(info[numdev].szDevice[0] != 0 && info[numdev].szDevice[0] != (WCHAR)-1,
> -
>            "GetAvailableMonitors returned info[%d].szDevice[0] = %d, expected != 0 and != -1.\n", numdev, info[numdev].szDevice[0]);
> -
>    todo_wine ok(info[numdev].szDescription[0] != 0 && info[numdev].szDescription[0] != (WCHAR)-1,
> -
>            "GetAvailableMonitors returned info[%d].szDescription[0] = %d, expected != 0 and != -1.\n", numdev, info[numdev].szDescription[0]);
> -
>    ok(info[numdev].dwVendorId != (DWORD)-1,
> -
>            "GetAvailableMonitors returned info[%d].dwVendorId = -1, expected != -1.\n", numdev);
> -
>    ok(info[numdev].dwDeviceId != (DWORD)-1,
> -
>            "GetAvailableMonitors returned info[%d].dwDeviceId = -1, expected != -1.\n", numdev);
> -
>    ok(info[numdev].dwSubSysId != (DWORD)-1,
> -
>            "GetAvailableMonitors returned info[%d].dwSubSysId = -1, expected != -1.\n", numdev);
> -
>    ok(info[numdev].dwRevision != (DWORD)-1,
> -
>            "GetAvailableMonitors returned info[%d].dwRevision = -1, expected != -1.\n", numdev);
> - }

Personally I don't like this, I think this should be more explicit - set 
fields separately to some invalid value that shouldn't happen and check 
it back. For example for string field it makes no sense setting it to -1 
if it's always filled.