[Piglit] [PATCH] DSA: fix error value for *TextureSubImage* when target doesn't match

[Arthur Huillet]

Hi,

On Thu, 23 Apr 2015 14:26:32 +0300
Martin Peres <martin.peres at linux.intel.com> wrote:

> >>> Does the patch look OK to you?
> >> Yeah, patch looks perfectly fine. But I'd like to hear from Laura (or
> >> Martin perhaps, who reviewed a lot of these) about whether this was
> >> done on purpose or not, pending a spec fix.
> 
> I am not aware of any bug report filed by Laura on this. Given that she 
> wrote this code in October last year, I am sure she would have received 
> an answer from Khronos if she had filed a bug. This would suggest it is 
> a bug in the test.
> 
> Let's wait for her comment to make sure of it.
> 
> Anyway, thanks for the patch Arthur! I know it can be frustrating to 
> have to check every failing test while wondering whether this is a test 
> or a driver bug. We should encourage developers writing piglit tests to 
> try them on multiple drivers that already support the extension they are 
> testing. This is however impossible to mandate.

Asking Piglit contributors to test their tests (...) on more than one implementation
is certainly a good idea, but it's not always practical. I think it is easier to ask,
for the tests for which it is appropriate (like the ones I fixed), to quote the part
of the specification that they are testing. This also has the advantage of making 
the reviewers' life easier - I don't think many people know by heart what error 
is supposed to be returned by a particular error condition, so having a piece of 
spec to check is helpful.
I'll go a little further and suggest that in order to review a "GL_INVALID_*" 
test properly, you *need* to look at the spec. So better quote it directly.
 
Here are more examples from DSA:

"bind-texture-unit.c:63:

    glGenTextures(1, &name);
    glBindTextureUnit(0, name);
    pass &= piglit_check_gl_error(GL_INVALID_ENUM);

But here's the spec:
    "An INVALID_OPERATION error is generated by BindTextureUnit if tex-
ture is not zero or the name of an existing texture object."

So I believe INVALID_OPERATION is the correct thing to return (will send a patch
when I can).

Then, bind-texture-unit.c:70:

    /* Texture unit doesn't exist */

    glDeleteTextures(1, &name);
    glCreateTextures(GL_TEXTURE_2D, 1, &name);
    glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &nunits);
    glBindTextureUnit(nunits, name); /* Too High */
    pass &= piglit_check_gl_error(GL_INVALID_OPERATION);

I haven't been able to find the relevant language to justify
GL_INVALID_OPERATION here. (NVIDIA returns INVALID_VALUE)
I won't send a patch since I can't back up my claim with spec."

> Do you try your tests against Intel's or AMD's driver for Windows? If 
> so, why not start working on them in the open and push them to piglit 
> since it now supports Windows?

I'm a Linux engineer and if I can avoid working on Windows, I will. :)

-- 
Greetings, 
A. Huillet