[Piglit] [PATCH v2 01/10] GL3.2 GL_ARB_sync: Test to check the correct error messages are returned for invalid inputs for ClientWaitSync

[Chad Versace]

On 09/27/2013 04:22 PM, Chad Versace wrote:
> On 08/20/2013 11:26 AM, Nicholas Mack wrote:
>> v2: Fix comments, initialize variables, rewrite loop through all bit masks
>> ---
>>   tests/spec/arb_sync/CMakeLists.gl.txt       |   1 +
>>   tests/spec/arb_sync/ClientWaitSync-errors.c | 103 ++++++++++++++++++++++++++++
>>   2 files changed, 104 insertions(+)
>>   create mode 100644 tests/spec/arb_sync/ClientWaitSync-errors.c
>>
>> diff --git a/tests/spec/arb_sync/CMakeLists.gl.txt b/tests/spec/arb_sync/CMakeLists.gl.txt
>> index dd4cf35..05f0972 100644
>> --- a/tests/spec/arb_sync/CMakeLists.gl.txt
>> +++ b/tests/spec/arb_sync/CMakeLists.gl.txt
>> @@ -12,3 +12,4 @@ link_libraries (
>>
>>   piglit_add_executable (arb_sync-repeat-wait repeat-wait.c)
>>   piglit_add_executable (arb_sync-timeout-zero timeout-zero.c)
>> +piglit_add_executable (arb_sync-ClientWaitSync-errors ClientWaitSync-errors.c)
>> diff --git a/tests/spec/arb_sync/ClientWaitSync-errors.c b/tests/spec/arb_sync/ClientWaitSync-errors.c
>> new file mode 100644
>> index 0000000..815585d
>> --- /dev/null
>> +++ b/tests/spec/arb_sync/ClientWaitSync-errors.c
>> @@ -0,0 +1,103 @@
>> +/**
>> + * Copyright © 2013 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +/**
>> + * Test ClientWaitSync() returns correct error messages for invalid input
>> + *
>> + *
>> + * Section 5.2.1(Waiting for Sync Objects) of OpenGL 3.2 Core says:
>> + * "If <sync> is not the name of a sync object, an INVALID_VALUE error
>> + *  is generated. If <flags> contains any bits other than
>> + *  SYNC_FLUSH_COMMANDS_BIT, an INVALID_VALUE error is generated."
>> + *
>> + */
>
> File-level comments need a \file command. Otherwise, Doxygen applies them to the
> first symbol in the file rather than the file itself. That is, the comment
> shoudl look like this:
>
> /**
>   * \file
>   * Test ClientWaitSync() returns ...
>   *
>   * Section 5.2.1 ...
>   */
>
>> +
>> +#include "piglit-util-gl-common.h"
>> +
>> +PIGLIT_GL_TEST_CONFIG_BEGIN
>> +
>> +config.supports_gl_compat_version = 10;
>> +config.supports_gl_core_version = 31;
>> +
>> +PIGLIT_GL_TEST_CONFIG_END
>> +
>> +enum piglit_result
>> +piglit_display(void)
>> +{
>> +    /* UNREACHED */
>> +    return PIGLIT_FAIL;
>> +}
>> +
>> +void
>> +piglit_init(int argc, char **argv)
>> +{
>> +    bool pass = true;
>> +    GLsync a = (GLsync)0xDEADBEEF;
>> +    GLsync b = (GLsync)20;
>> +    GLenum mess1;
>
> The name 'mess1' isn't very descriptive :). It should be renamed to 'status'
> or 'sync_status'.
>
>> +    int i;
>> +
>> +    /* sync not set up yet so this should fail with both GL error and
>> +     * respond GL_WAIT_FAILED
>> +     */
>> +    mess1 = glClientWaitSync(a, GL_SYNC_FLUSH_COMMANDS_BIT, 0);
>> +    pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;
>> +    if(mess1 != GL_WAIT_FAILED) {
>> +        printf("Expected GL_WAIT_FAILED but returned: %s\n",
>> +            piglit_get_gl_enum_name(mess1));
>> +        pass = false;
>> +    }
>> +
>> +    if (piglit_get_gl_version() < 32) {
>> +        piglit_require_extension("GL_ARB_sync");
>> +    }
>
> All requirements must be moved to the top of piglit_init(). Otherwise, if
> neither requirement were satisfied, the above call to glClientWaitSync()
> may segfault because it may be a garbage function pointer.
>
>> +
>> +    a = glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
>> +
>> +    /* test that valid sync results in NO_ERROR */
>> +    mess1 = glClientWaitSync(a, GL_SYNC_FLUSH_COMMANDS_BIT, 0);
>> +    pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
>> +
>> +    /* test that invalid sync results in INVALID_VALUE */
>> +    mess1 = glClientWaitSync(b, GL_SYNC_FLUSH_COMMANDS_BIT, 0);
>> +    pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;
>
> This paragraph is equivalent to paragraph 1, and so should be removed.
> Also, this removes the only use of 'b', so remove 'b' too.
>
>> +
>> +    /* test valid flag values result in NO_ERROR (only one option) */
>> +    mess1 = glClientWaitSync(a, GL_SYNC_FLUSH_COMMANDS_BIT, 0);
>> +    pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
>
> This is verbatim equivalent to paragraph 2, and so should be removed.
>
>> +
>> +    /* test that invalid flag value results in INVALID_VALUE */
>> +    for (i = 0; i < sizeof(GLbitfield) * 8; i++) {
>> +        GLbitfield mask = 1 << i;
>> +        /* Skip over the valid bit */
>> +        if (mask == GL_SYNC_FLUSH_COMMANDS_BIT) {
>> +            continue;
>> +        }
>> +        mess1 = glClientWaitSync(a, mask, 0);

I forgot. The test should also check here that `mess1 == GL_WAIT_FAILED`.

>> +        pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;
>> +    }
>> +
>> +    glDeleteSync(a);
>> +
>> +    piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
>> +}
>
> Also, you need to add the test to all.tests. Otherwise, the test looks
> correct.