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

[Martin Peres]

On 23/04/15 15:12, Arthur Huillet wrote:
> 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 think we already ask piglit developers to quote the spec at the 
beginning of their tests. I certainly tried to do that on my tests and 
only test the behaviour described in this test. If it has been done, 
then I guess the review was a problem here. It would seem like multiple 
months passed between the moment the patches were written and the moment 
they got commited. I fear that no-one wanted to do a good review so it 
just got pushed.

Sometimes, it is good for readability to re-quote the spec in-line or 
move part of the spec documentation where it is tested (like in this 
test [1]), but this is not always working or possible, like in this test 
[2].

In any case, we indeed need to be careful about these errors.

> 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).
Please do :)

Seems like the test has been written to check for no change in behaviour 
with between the DSA and non-DSA paths instead of a close look at the 
documentation.

>
> 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."

We should not test for unspecified behaviour. That also deserves a patch 
to get rid of the check altogether.
>
>> 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. :)

Ah ah, indeed!

I assumed that the tests were written by the windows engineers and 
suggested that they could piglit too. If you also write tests from 
Linux, then you have even less excuses not to make them public in piglit :p



[1] 
http://cgit.freedesktop.org/piglit/commit/?id=8c1d2e8bac5d3526091ca911dd5d4ed0f859d392
[2] 
http://cgit.freedesktop.org/piglit/commit/?id=fa7c2ac879962e3922e7e694955c6879156b7d45