Re: [1/4] ole32/test: Add InitCache test

[Huw Davies]

On Tue, May 02, 2017 at 06:10:44PM +0800, Wei Xie wrote:
> From 4f8b94b5a8c7739901eae8f19317ca6255911d20 Mon Sep 17 00:00:00 2001
> From: Wei xie 
> Date: Tue, 2 May 2017 18:07:22 +0800
> Subject: [PATCH 1/4] ole32/test: Add InitCache test

ole32/test: -> ole32/tests:
> +static HRESULT WINAPI DataObject2_QueryInterface(
> -
>        IDataObject*     iface,
> -
>        REFIID           riid,
> -
>        void**           ppvObject)
>
> +{
> - CHECK_EXPECTED_METHOD("DataObject2_QueryInterface");
> -
> - if (IsEqualIID(riid, &IID_IDataObject) || IsEqualIID(riid, &IID_IUnknown))
> - {
> -
>    *ppvObject = iface;
> -
>    return S_OK;
> - }
> - *ppvObject = NULL;
> - return S_OK;
>
> +}

There's no need to create a new IDataObject implementation.
Use the existing one and add a file-scope variable to control
what _GetData does (which is all you really care about for
this test).
> +static HRESULT WINAPI DataObject2_GetData(
> -
>    IDataObject*     iface,
> -
>    LPFORMATETC      pformatetcIn,
> -
>    STGMEDIUM*       pmedium)
>
> +{
> -
>    *pformatetcIn = FormatEtc;
> -
>    *pmedium = StgMedium;
> -
>    return S_OK;
>
> +}

You're not checking this method is called, which should be
one of the goals of this test!

pfomratetcIn is an [in] variable, so assigning things to it is
useless.
> @@ -1838,10 +1994,23 @@ todo_wine {
>  DeleteDC(hdcMem);
>
> - todo_wine {

You remove a todo_wine here and yet this patch doesn't change
Wine's implementation, so this will produce a failure.  The
test must run cleanly on Wine (and Windows) after just this
patch is applied.  If you fix the implementation later on in
the series, then remove the todo_wine in that patch, not this
one.
> - /* IDataObject_GetData Invalid */

I don't understand how this comment relates to the code.
>  hr = IOleCache2_InitCache(pOleCache, &DataObject);
>  ok(hr == CACHE_E_NOCACHE_UPDATED, "IOleCache_InitCache should have returned CACHE_E_NOCACHE_UPDATED instead of 0x%08x\n", hr);
> - }
> -
> - fmtetc.ptd = NULL;
> - fmtetc.dwAspect = DVASPECT_CONTENT;
> - fmtetc.lindex = -1;
> - fmtetc.cfFormat = CF_BITMAP;
> - fmtetc.tymed = TYMED_GDI;
> - stgmedium.tymed = TYMED_GDI;
> - stgmedium.hBitmap = CreateCompatibleBitmap(hdcMem, 100, 100);
> - stgmedium.pUnkForRelease = NULL;
> - IDataObject_SetData(&DataObject2, &fmtetc, &stgmedium, FALSE);
> -
> - /* IOleCache2_InitCache not need IOleCache2_Cache first */
> - hr = IOleCache2_InitCache(pOleCache, &DataObject2);
> - ok(hr == CACHE_E_NOCACHE_UPDATED, "IOleCache2_InitCache should have returned CACHE_E_NOCACHE_UPDATED instead of 0x%08x\n", hr);

Why is the hunk even necessary, you're just calling InitCache again
below a test that shows it fails.
>  IPersistStorage_Release(pPS);
>  IViewObject_Release(pViewObject);
>
> @@ -1851,6 +2020,44 @@ todo_wine {
>      CHECK_NO_EXTRA_METHODS();
>      }
> - hdcMem = CreateCompatibleDC(NULL);
> - fmtetc.ptd = NULL;
> - fmtetc.dwAspect = DVASPECT_CONTENT;
> - fmtetc.lindex = -1;
> - fmtetc.cfFormat = CF_BITMAP;
> - fmtetc.tymed = TYMED_GDI;
> - stgmedium.tymed = TYMED_GDI;
> - stgmedium.hBitmap = CreateCompatibleBitmap(hdcMem, 100, 100);
> - stgmedium.pUnkForRelease = NULL;
> - IDataObject_SetData(&DataObject2, &fmtetc, &stgmedium, FALSE);
> -
> - expected_method_list = methods_cacheinit;
> -
> - hr = CreateDataCache(NULL, &CLSID_NULL, &IID_IOleCache2, (LPVOID *)&pOleCache);
> - ok_ole_success(hr, "CreateDataCache");
> -
> - hr = IOleCache2_QueryInterface(pOleCache, &IID_IViewObject2, (LPVOID *)&pViewObject2);
> - ok_ole_success(hr, "IOleCache_QueryInterface(IID_IViewObject2)");
> -
> - hr = IOleCache2_Cache(pOleCache, &fmtetc, 0, &dwConnection);
> - ok_ole_success(hr, "IOleCache2_Cache");
> - hr = IOleCache2_InitCache(pOleCache, &DataObject2);
> - ok_ole_success(hr, "IOleCache2_InitCache");
> -
> - hr = IViewObject2_SetAdvise(pViewObject2, DVASPECT_CONTENT, ADVF_PRIMEFIRST, &AdviseSink);
> - ok_ole_success(hr, "IViewObject2_SetAdvise");
> -
> - hr = IViewObject2_GetExtent(pViewObject2, DVASPECT_CONTENT, -1, NULL, &sz );
> - ok_ole_success(hr, "IViewObject2_GetExtent");
> -
> - hr = IViewObject2_Draw(pViewObject2, DVASPECT_CONTENT, -1, NULL, NULL, NULL, hdcMem, &rcBounds, NULL, draw_continue, 0xdeadbeef);
> - ok_ole_success(hr, "IViewObject2_GetExtent");
> -
> - IViewObject2_Release(pViewObject2);
> - IOleCache2_Release(pOleCache);
> -
> - DeleteDC(hdcMem);

For this bit of the test, drop the IViewObject calls.  What we care
about is testing InitCache().

Then I suggest calling _Cache twice with two different formats and
checking that _InitCache calls DataObject_GetData for those two
formats.  You'll need to make DataObject_GetData respond to these two
different formats.

It's a bit more complicated than that however because in the CF_BITMAP
case _InitCache apparently calls _GetData with CF_DIB (this is part of the
cache's ability to use dibs instead of bitmaps and vice-versa).  You
may want to avoid using CF_BITMAP to test this at this stage (we can
add CF_BITMAP <-> CF_DIB tests later).

Huw.