Re: [PATCH 1/3] d3dx9/tests: Add tests for cross effect handles usage.

[Matteo Bruni]

2017-04-11 15:22 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp(a)gmail.com>
> ---
>  dlls/d3dx9_36/tests/effect.c | 75 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c
> index de5ae69..62da636 100644
> --- a/dlls/d3dx9_36/tests/effect.c
> +++ b/dlls/d3dx9_36/tests/effect.c
> @@ -4515,18 +4515,26 @@ static void test_isparameterused_children(ID3DXEffect *effect, D3DXHANDLE tech,
>      }
>  }
>
> -static void test_isparameterused_param_with_children(ID3DXEffect *effect, D3DXHANDLE tech, const char *name,
> -        BOOL expected_result)
> +static void test_isparameterused_param_with_children(ID3DXEffect *effect, ID3DXEffect *effect2, D3DXHANDLE tech,
> +        const char *name, BOOL expected_result)

I think you want to pass the source line to the function (like e.g.
test_effect_preshader_compare_vconsts()) now that you call it from
multiple places.
> {
>      D3DXHANDLE param;
>
> -    param = effect->lpVtbl->GetParameterByName(effect, NULL, name);
> +    ok(!effect->lpVtbl->IsParameterUsed(effect, (D3DXHANDLE)name, tech) == !expected_result,
> +            "Unexpected IsParameterUsed() result for %s.\n", name);
> +
> +    if (effect2)
> +        param = effect2->lpVtbl->GetParameterByName(effect2, NULL, name);
> +    else
> +        param = effect->lpVtbl->GetParameterByName(effect, NULL, name);
>      ok(!!param, "GetParameterByName failed for %s.\n", name);
>
> +    todo_wine_if(!!effect2 && expected_result)

You don't need the !! here, the && gives an integer result anyway.
> ok(!effect->lpVtbl->IsParameterUsed(effect, param, tech) == !expected_result,
>              "Unexpected IsParameterUsed() result for %s.\n", name);

This is not new, but again the negations shouldn't be necessary.
Actually if it turns out that they are, that means IsParameterUsed()
doesn't always return a proper BOOL and that's an interesting result
by itself.
> +static void test_cross_effect_handle(IDirect3DDevice9 *device)
> +{
> +    ID3DXEffect *effect1, *effect2;
> +    D3DXHANDLE param1, param2;
> +    static int expected_ivect[4] = {28, 29, 30, 31};
> +    int ivect[4];
> +    HRESULT hr;
> +
> +    hr = D3DXCreateEffect(device, test_effect_preshader_effect_blob, sizeof(test_effect_preshader_effect_blob),
> +            NULL, NULL, 0, NULL, &effect1, NULL);
> +    ok(hr == D3D_OK, "Got result %#x.\n", hr);
> +    hr = D3DXCreateEffect(device, test_effect_preshader_effect_blob, sizeof(test_effect_preshader_effect_blob),
> +            NULL, NULL, 0, NULL, &effect2, NULL);
> +    ok(hr == D3D_OK, "Got result %#x.\n", hr);

It probably makes sense to add a ok(effect1 != effect2) here, to show
that native doesn't do effect object deduplication (which would
trivially imply that the rest of this test passes).
> +    param1 = effect1->lpVtbl->GetParameterByName(effect1, NULL, "g_iVect");
> +    ok(!!param1, "GetParameterByName failed.\n");
> +
> +    param2 = effect2->lpVtbl->GetParameterByName(effect2, NULL, "g_iVect");
> +    ok(!!param2, "GetParameterByName failed.\n");

Same here with param1 and param2.