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 :
> Signed-off-by: Paul Gofman
>
> 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.