Re: [PATCH 3/3] d3drm/tests: Some tests for mesh builder and faces

[Henri Verbeet]

On 9 June 2017 at 01:36, Nikolay Sivov  wrote:
> +#define test_vector_eq(a, b, c, d) test_vector_eq_(__LINE__, a, b, c, d)
> +static void test_vector_eq_(unsigned int line, D3DVECTOR *v, float x, float y, float z)

"v" should probably be const. More generally, with the scheme we've
been using for the other D3D tests, this would look like the
following:

static void check_vector_(unsigned int line, const D3DVECTOR *v,
float x, float y, float x, unsigned int ulps)

and print the ok_() as something like "Got unexpected vector {%.8e,
%.8e, %.8e}, expected {%.8e, %.8e, %.8e}.\n". Note that the %.8e is
because %f is lossy. See e.g. compare_vec4() in the ddraw and d3d11
tests.
> - memset(v, 0, sizeof(v));
> - hr = IDirect3DRMFace_GetVertex(face1, 0, v, n);
> - ok(SUCCEEDED(hr), "Failed to get vertex data, hr %#x.\n", hr);
> - ok(U1(v[0]).x == 1.0f, "Wrong component v[0].x = %f (expected %f)\n", U1(v[0]).x, 1.0f);
> - ok(U2(v[0]).y == 2.0f, "Wrong component v[0].y = %f (expected %f)\n", U2(v[0]).y, 2.0f);
> - ok(U3(v[0]).z == 3.0f, "Wrong component v[0].z = %f (expected %f)\n", U3(v[0]).z, 3.0f);
> -

That should probably use check_vector(), and I think there are more
places in the d3drm tests that would benefit from using
check_vector().
> - color = IDirect3DRMFace_GetColor(face1);
>
> +todo_wine
> - ok(color == 0xff00ff00, "Got wrong color, %#x.\n", color);

%#x is a little awkward for colours, 0x%08x would be more appropriate.
I'd argue for "unexpected" over "wrong".
> - hr = IDirect3DRMMeshBuilder3_GetFace(mesh_builder3, 0, &face2);
> - ok(SUCCEEDED(hr), "Failed to delete a face, %#x.\n", hr);

s/delete/retrieve/
I think that should be "the" instead of "a", but I think simply
omitting the article would also be fine.
> -#define IDirect3DRMFace_GetColor(p,a)                     (p)->lpVtbl->GetColor(p,a)
> +#define IDirect3DRMFace_GetColor(p)                       (p)->lpVtbl->GetColor(p)

IDirect3DRMFace2 seems to have the same issue.