[Piglit] [PATCH 1/7] arb_vertex_attrib_binding: Test error conditions

[Fredrik Höglund]

On Tuesday 29 October 2013, Eric Anholt wrote:
> Sorry we all dropped this on the floor.  I'm taking a look at your
> series now.
> 
> Fredrik Höglund <fredrik at kde.org> writes:
> > diff --git a/tests/spec/arb_vertex_attrib_binding/errors.c b/tests/spec/arb_vertex_attrib_binding/errors.c
> > new file mode 100644
> > index 0000000..adac098
> > --- /dev/null
> > +++ b/tests/spec/arb_vertex_attrib_binding/errors.c
> 
> > +#define MIN(a, b) (a < b ? a : b)
> 
> There's already a MIN2 macro you can use.

This test doesn't actually use the macro. I guess it's a left over from the
test I based it on.

> > +	/* uncreached */
> 
> "unreached"
> 
> > +void
> > +piglit_init(int argc, char **argv)
> > +{
> > +	GLint maxBindings, maxAttribs, maxRelativeOffset;
> > +	GLuint vbo, vao;
> > +	int i;
> > +
> > +	piglit_require_extension("GL_ARB_vertex_attrib_binding");
> > +	piglit_require_extension("GL_ARB_vertex_array_bgra");
> > +	piglit_require_extension("GL_ARB_vertex_type_2_10_10_10_rev");
> > +	piglit_require_extension("GL_ARB_ES2_compatibility");
> 
> Would be nice to see the extra extensions moved into
> piglit_is_extension_supported() checks for executing those tests.
> Though I suppose since a lot of the code for them is expecting
> INVALID_ENUM, you can execute that with or without the extension
> providing them :)

Yeah, I've been meaning to do that, but I wanted to hear what others had
to say about the ARB_vertex_attrib_binding dependencies first.

> > +
> > +	/* Create and bind a vertex array object */
> > +	glGenVertexArrays(1, &vao);
> > +	glBindVertexArray(vao);
> > +
> > +	glGenBuffers(1, &vbo);
> > +	glBindBuffer(GL_ARRAY_BUFFER, vbo);
> > +
> > +
> > +	/* Query limits */
> > +	glGetIntegerv(GL_MAX_VERTEX_ATTRIBS, &maxAttribs);
> > +	glGetIntegerv(GL_MAX_VERTEX_ATTRIB_BINDINGS, &maxBindings);
> > +	glGetIntegerv(GL_MAX_VERTEX_ATTRIB_RELATIVE_OFFSET, &maxRelativeOffset);
> > +
> > +	/* Clear the error state */
> > +	while (glGetError() != GL_NO_ERROR) {}
> 
> What error state are you clearing here?  If you really do expect to have
> something you're ignoring, piglit_reset_gl_error() is the helper for
> this.

There shouldn't be any errors at this point, it's just make sure we
start with a clean slate.

Other piglit tests seem to do this as well.

> > +	/* index >= GL_MAX_VERTEX_ATTRIB_BINDINGS */
> > +	glBindVertexBuffer(maxBindings, vbo, 0, 0);
> > +	if (!piglit_check_gl_error(GL_INVALID_VALUE))
> > +		piglit_report_result(PIGLIT_FAIL);
> 
> Instead of piglit_report_result(PIGLIT_FAIL) immediately, please set
> pass = false, and only at the end of the test do
> piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
> 
> That way somebody writing a new implementation gets to see what's still
> failing as they incrementally make things work.

Good point.
 
> Once these comments are fixed, this test is:
> 
> Reviewed-by: Eric Anholt <eric at anholt.net>
> 
> Errors I don't see covered:
> 
> "An INVALID_OPERATION error is generated if no vertex array object is
> bound." (When in core profile for BindVertexbuffer, VertexAttribBinding,
> VertexBindingDivisor)
>
> "      [Core profile only:]
>     An INVALID_OPERATION error is generated if buffer is not zero or a
>     name returned from a previous call to GenBuffers, or if such a name
>     has since been deleted with DeleteBuffers."

Good catch. The specification actually didn't say anything about non-gen
buffer names when I started on the implementation.

> So it seems like we might want a core profile-only extra errors test.

Yeah.
 
> A non-error I noticed not explicitly touched in this series: "If
> <buffer> is zero, any buffer object attached to this bindpoint is
> detached."

I'll add that in get.c, since it already has tests for binding buffers.

Fredrik