Re: [PATCH 3/3] msscript.ocx/tests: Add tests for IScriptControl::Run.

[Jacek Caban]

On 9/26/19 4:43 PM, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu
>
> dlls/msscript.ocx/tests/msscript.c | 560 ++++++++++++++++++++++++++++-
>   1 file changed, 558 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/msscript.ocx/tests/msscript.c b/dlls/msscript.ocx/tests/msscript.c
> index 95b2a69..091d882 100644
> --- a/dlls/msscript.ocx/tests/msscript.c
> +++ b/dlls/msscript.ocx/tests/msscript.c
> @@ -29,6 +29,7 @@
>
> #include "msscript.h"
>   #include "wine/test.h"
> +DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0);
>
> #define TESTSCRIPT_CLSID "{178fc164-f585-4e24-9c13-4bb7faf80746}"
>   static const GUID CLSID_TestScript =
> @@ -92,12 +93,17 @@ DEFINE_EXPECT(CreateInstance);
>   DEFINE_EXPECT(SetInterfaceSafetyOptions);
>   DEFINE_EXPECT(InitNew);
>   DEFINE_EXPECT(Close);
> +DEFINE_EXPECT(QI_IDispatchEx);
> +DEFINE_EXPECT(GetIDsOfNames);
> +DEFINE_EXPECT(Invoke);
> +DEFINE_EXPECT(InvokeEx);
>   DEFINE_EXPECT(SetScriptSite);
>   DEFINE_EXPECT(QI_IActiveScriptParse);
>   DEFINE_EXPECT(SetScriptState_INITIALIZED);
>   DEFINE_EXPECT(SetScriptState_STARTED);
>   DEFINE_EXPECT(ParseScriptText);
>   DEFINE_EXPECT(AddNamedItem);
> +DEFINE_EXPECT(GetScriptDispatch);
>
> #define EXPECT_REF(obj,ref) _expect_ref((IUnknown*)obj, ref, __LINE__)
>   static void _expect_ref(IUnknown* obj, ULONG ref, int line)
> @@ -226,6 +232,317 @@ static const IObjectSafetyVtbl ObjectSafetyVtbl = {
>
> static IObjectSafety ObjectSafety = { &ObjectSafetyVtbl };
>
> +static void dispparams_release(DISPPARAMS *dp)
> +{
> - while (dp->cArgs--)
> -
>    VariantClear(&dp->rgvarg[dp->cArgs]);
> - HeapFree(GetProcessHeap(), 0, dp->rgvarg);
>
> +}
> -
>
> +static HRESULT safearray_to_dispparams(SAFEARRAY *sa, DISPPARAMS *out)
> +{
> - DISPPARAMS dp;
> - HRESULT hr;
> - UINT i;
> -
> - if (sa->cDims == 0 || !(sa->fFeatures & FADF_VARIANT))
> -
>    return E_FAIL;
> -
> - dp.cArgs = sa->rgsabound[0].cElements;
> - dp.rgdispidNamedArgs = NULL;
> - dp.cNamedArgs = 0;
> -
> - dp.rgvarg = HeapAlloc(GetProcessHeap(), 0, dp.cArgs * sizeof(*dp.rgvarg));
> - if (!dp.rgvarg) return E_OUTOFMEMORY;
> -
> - hr = SafeArrayLock(sa);
> - if (FAILED(hr))
> - {
> -
>    HeapFree(GetProcessHeap(), 0, dp.rgvarg);
> -
>    return hr;
> - }
> -
> - for (i = 0; i < dp.cArgs; i++)
> - {
> -
>    /* The DISPPARAMS are stored in reverse order */
> -
>    VARIANT *src = (VARIANT*)((char*)(sa->pvData) + (dp.cArgs - i - 1) * sa->cbElements);
> -
> -
>    V_VT(&dp.rgvarg[i]) = VT_EMPTY;
> -
>    hr = VariantCopy(&dp.rgvarg[i], src);
> -
>    if (FAILED(hr))
> -
>    {
> -
>        dp.cArgs = i;
> -
>        dispparams_release(&dp);
> -
>        goto err;
> -
>    }
> - }
> - *out = dp;
> -
>
> +err:
> - SafeArrayUnlock(sa);
> - return hr;
>
> +}
> -
>
> +static struct
> +{
> - UINT line;
> - const WCHAR *name;
> - DISPPARAMS dp;
>
> +} Dispatch_expect;
> -
>
> +#define set_Dispatch_expect(n,sa) do { \
> - Dispatch_expect.line = __LINE__; \
> - Dispatch_expect.name = (n); \
> - ok(safearray_to_dispparams((sa), &Dispatch_expect.dp) == S_OK, "safearray_to_dispparams failed.\n"); \
>
> +} while(0)

That part seems to be more complicated than needed. Since you always 
pass the same arguments anyway, you could just as well hardcode expected 
arguments in Invoke[Ex] implementation. If you really need something 
more flexible, how about storing DISPPARAMS passed to Invoke[Ex] 
somewhere in global variable and testing it in the caller, after Run() 
call returns?
> +static const IDispatchExVtbl DispatchExVtbl = {
> - DispatchEx_QueryInterface,
> - DispatchEx_AddRef,
> - DispatchEx_Release,
> - DispatchEx_GetTypeInfoCount,
> - DispatchEx_GetTypeInfo,
> - DispatchEx_GetIDsOfNames,
> - DispatchEx_Invoke,
> - DispatchEx_GetDispID,
> - DispatchEx_InvokeEx,
> - DispatchEx_DeleteMemberByName,
> - DispatchEx_DeleteMemberByDispID,
> - DispatchEx_GetMemberProperties,
> - DispatchEx_GetMemberName,
> - DispatchEx_GetNextDispID,
> - DispatchEx_GetNameSpaceParent
>
> +};
> -
>
> +static IDispatchEx DispatchEx = { &DispatchExVtbl };
> -
>
> +static BOOL DispatchEx_available = FALSE;
> +static HRESULT WINAPI Dispatch_QueryInterface(IDispatch *iface, REFIID riid, void **ppv)
> +{
> - *ppv = NULL;
> -
> - if (IsEqualGUID(&IID_IDispatchEx, riid))
> - {
> -
>    CHECK_EXPECT(QI_IDispatchEx);
> -
>    if (DispatchEx_available)
> -
>    {
> -
>        *ppv = &DispatchEx;
> -
>        return S_OK;
> -
>    }
> -
>    return E_NOINTERFACE;
> - }
> -
> - ok(0, "unexpected riid %s\n", wine_dbgstr_guid(riid));
> - return E_NOINTERFACE;
>
> +}

You don't really need separated vtbls for IDispatch and IDispatchEx. You 
could have a single IDispatchEx implementation and simply not expose 
IDispatchEx from QI in cases when you want to test plain IDispatch.
> @@ -1757,6 +2078,240 @@ static void test_IScriptControl_ExecuteStatement(void)
>       }
>   }
>
> +static void test_IScriptControl_Run(void)
> +{
> - SAFEARRAYBOUND bnd[] = { { 2, 0 }, { 2, 0 } };
> - LONG idx0_0[] = { 0, 0 };
> - LONG idx0_1[] = { 1, 0 };
> - LONG idx1_0[] = { 0, 1 };
> - LONG idx1_1[] = { 1, 1 };
> - IScriptControl *sc;
> - SAFEARRAY *params;
> - VARIANT var;
> - HRESULT hr;
> - BSTR str;
> -
> - hr = CoCreateInstance(&CLSID_ScriptControl, NULL, CLSCTX_INPROC_SERVER | CLSCTX_INPROC_HANDLER,
> -
>                      &IID_IScriptControl, (void**)&sc);
> - ok(hr == S_OK, "Failed to create IScriptControl interface: 0x%08x.\n", hr);
> -
> - params = NULL;
> - str = a2bstr("identifier");
> - hr = IScriptControl_Run(sc, str, &params, &var);
> - ok(hr == E_POINTER, "IScriptControl_Run returned: 0x%08x.\n", hr);
> -
> - params = SafeArrayCreate(VT_VARIANT, 1, bnd);
> - ok(params != NULL, "Failed to create SafeArray.\n");
> -
> - V_VT(&var) = VT_I4;
> - V_I4(&var) = 10;
> - SafeArrayPutElement(params, idx0_0, &var);
> - V_I4(&var) = 3;
> - SafeArrayPutElement(params, idx0_1, &var);
> -
> - hr = IScriptControl_Run(sc, str, &params, &var);
> - ok(hr == E_FAIL, "IScriptControl_Run returned: 0x%08x.\n", hr);
> -
> - hr = IScriptControl_Run(sc, str, NULL, &var);
> - ok(hr == E_POINTER, "IScriptControl_Run returned: 0x%08x.\n", hr);
> -
> - hr = IScriptControl_Run(sc, str, &params, NULL);
> - ok(hr == E_POINTER, "IScriptControl_Run returned: 0x%08x.\n", hr);
> - SysFreeString(str);
> -
> - hr = IScriptControl_Run(sc, NULL, &params, &var);
> - ok(hr == E_FAIL, "IScriptControl_Run returned: 0x%08x.\n", hr);
> -
> - str = a2bstr("jscript");
> - hr = IScriptControl_put_Language(sc, str);
> - ok(hr == S_OK, "IScriptControl_put_Language failed: 0x%08x.\n", hr);
> - SysFreeString(str);
> -
> - str = a2bstr("foobar");
> - hr = IScriptControl_Run(sc, str, &params, &var);
> - ok(hr == DISP_E_UNKNOWNNAME, "IScriptControl_Run failed: 0x%08x.\n", hr);
> - todo_wine CHECK_ERROR(sc, 0);
> - SysFreeString(str);
> -
> - str = a2bstr("function subtract(a, b) { return a - b; }\n");
> - hr = IScriptControl_AddCode(sc, str);
> - ok(hr == S_OK, "IScriptControl_AddCode failed: 0x%08x.\n", hr);
> - todo_wine CHECK_ERROR(sc, 0);
> - SysFreeString(str);
> -
> - str = a2bstr("Subtract");
> - hr = IScriptControl_Run(sc, str, &params, &var);
> - ok(hr == DISP_E_UNKNOWNNAME, "IScriptControl_Run failed: 0x%08x.\n", hr);
> - SysFreeString(str);
> -
> - str = a2bstr("subtract");
> - hr = IScriptControl_Run(sc, str, &params, NULL);
> - ok(hr == E_POINTER, "IScriptControl_Run failed: 0x%08x.\n", hr);
> - todo_wine CHECK_ERROR(sc, 0);
> -
> - hr = IScriptControl_Run(sc, str, &params, &var);
> - ok(hr == S_OK, "IScriptControl_Run failed: 0x%08x.\n", hr);
> - ok((V_VT(&var) == VT_I4) && (V_I4(&var) == 7), "V_VT(var) = %d, V_I4(var) = %d.\n", V_VT(&var), V_I4(&var));
> - todo_wine CHECK_ERROR(sc, 0);
> - VariantClear(&var);

You don't need to clear VT_I4 variant.
> - SafeArrayDestroy(params);
> -
> - /* The array must be of VT_VARIANT type, else it either
> -
>   crashes on Windows, or returns DISP_E_BADVARTYPE. */
> - if (!broken(1))
> - {
> -
>    params = SafeArrayCreate(VT_I4, 1, bnd);
> -
>    ok(params != NULL, "Failed to create SafeArray.\n");
> -
> -
>    V_I4(&var) = 10;
> -
>    SafeArrayPutElement(params, idx0_0, &V_I4(&var));
> -
>    V_I4(&var) = 3;
> -
>    SafeArrayPutElement(params, idx0_1, &V_I4(&var));
> -
> -
>    V_VT(&var) = VT_EMPTY;
> -
>    hr = IScriptControl_Run(sc, str, &params, &var);
> -
>    ok(hr == DISP_E_BADVARTYPE, "IScriptControl_Run returned: 0x%08x.\n", hr);
> -
>    ok(V_VT(&var) == VT_EMPTY, "V_VT(var) = %d.\n", V_VT(&var));
> -
>    SafeArrayDestroy(params);
> - }

This is not really an interesting test and having it run only on Wine 
may be misleading. Please remove it.

Jacek