[PATCH] d3dx9/tests: Add test for out of bound array selector in effect.

[Matteo Bruni]

2017-03-07 18:47 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>

Hi Paul! Patch looks mostly good to me BUT I have a couple comments anyway...

> ---
>  dlls/d3dx9_36/tests/effect.c | 159 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 141 insertions(+), 18 deletions(-)
>
> diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c
> index b3845c5..fe84173 100644
> --- a/dlls/d3dx9_36/tests/effect.c
> +++ b/dlls/d3dx9_36/tests/effect.c
> @@ -3813,6 +3813,33 @@ static const DWORD test_effect_preshader_effect_blob[] =
>  #define TEST_EFFECT_PRESHADER_VSHADER_POS 2458
>  #define TEST_EFFECT_PRESHADER_VSHADER_LEN 13
>
> +static BOOL test_effect_preshader_compare_shader(IDirect3DDevice9 *device, int expected_shader_index)
> +{
> +    IDirect3DVertexShader9 *vshader;
> +    void *byte_code;
> +    unsigned int byte_code_size;
> +    HRESULT hr;
> +    BOOL ret;
> +
> +    if (FAILED(hr = IDirect3DDevice9_GetVertexShader(device, &vshader)) || !vshader)
> +        return FALSE;

It seems preferable to keep the detailed ok() calls from the old test
instead of a generic ok() message on the return value of
test_effect_preshader_compare_shader(). I guess you want to avoid to
have ambiguous ok() calls in this helper. Until we get something like
the with_context() thing from
https://source.winehq.org/patches/data/131137 just passing a string
with the test name to the helper function and using that in the ok()
messages should do the trick.

> +    hr = effect->lpVtbl->BeginPass(effect, 1);
> +    todo_wine ok(hr == E_FAIL, "Got result %#x.\n", hr);
> +    if (SUCCEEDED(hr))
> +        effect->lpVtbl->EndPass(effect);
> +
> +    hr = effect->lpVtbl->BeginPass(effect, 1);
> +    ok(hr == D3D_OK, "Got result %#x.\n", hr);

That reminds me, do we have tests for two BeginPass() calls without an
EndPass() in between? Might be interesting to see what happens both
with the same or different pass index. Other somewhat interesting
cases might be BeginPass() outside of Begin() / End() and multiple
Begin() / End() / EndPass() calls in a row. It should probably be in a
separate patch though.

BTW, I think it's somewhat idiomatic to call it "out of bounds" even
when technically there is a single bound involved.