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.