Re: [Piglit] [PATCH 1/4] ATI_fs: add api error tests
Miklós Máté writes:
> On 21/11/17 21:09, Eric Anholt wrote:
>> Miklós Máté writes:
>>> +
>>> + piglit_report_result(PIGLIT_PASS);
>>> +}
>>> diff --git a/tests/spec/ati_fragment_shader/error02-inside.c
>>> b/tests/spec/ati_fragment_shader/error02-inside.c
>>> new file mode 100644
>>> index 0..5dee70cf6
>>> --- /dev/null
>>> +++ b/tests/spec/ati_fragment_shader/error02-inside.c
>>> @@ -0,0 +1,59 @@
>>> +/* TODO license header */
>>> +
>>> +/**
>>> + * Tests API errors for GL_ATI_fragment_shader.
>>> + * One for each paragraph in the Errors section.
>>> + */
>>> +
>>> +#include "piglit-util-gl.h"
>>> +
>>> +PIGLIT_GL_TEST_CONFIG_BEGIN
>>> +
>>> + config.supports_gl_compat_version = 10;
>>> + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
>>> +
>>> +PIGLIT_GL_TEST_CONFIG_END
>>> +
>>> +enum piglit_result
>>> +piglit_display(void)
>>> +{
>>> + /* UNREACHED */
>>> + return PIGLIT_FAIL;
>>> +}
>>> +
>>> +#define check_gl_error(err) if (!piglit_check_gl_error(err))
>>> piglit_report_result(PIGLIT_FAIL)
>>> +
>>> +void
>>> +piglit_init(int argc, char **argv)
>>> +{
>>> + piglit_require_extension("GL_ATI_fragment_shader");
>>> +
>>> + /*
>>> +* Paragraph 2 of the Errors section:
>>> +*
>>> +* The error INVALID_OPERATION is generated if GenFragmentShadersATI,
>>> +* BindFragmentShaderATI, DeleteFragmentShaderATI, or
>>> +* BeginFragmentShaderATI are specified inside a
>>> +* Begin/EndFragmentShaderATI pair.
>>> +*/
>>> +
>>> + glBeginFragmentShaderATI();
>>> +
>>> + check_gl_error(GL_NO_ERROR);
>>> + glGenFragmentShadersATI(1);
>>> + check_gl_error(GL_INVALID_OPERATION);
>> Instead of the macro, we'll often have
>>
>> bool pass = true;
>>
>> ...
>>
>> pass = piglit_check_gl_error(GL_NO_ERROR) && pass
>> glGenFragmentShadersATI(1);
>> pass = piglit_check_gl_error(GL_INVALID_OPERATION) && pass
>>
>> ...
>>
>> piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
>>
>> That way you get to see multiple errors if there are different cases
>> failing.
> First I did that, but I had a difficult time finding out where Mesa
> failed to throw the expected error, so I made the check fatal. If people
> prefer non-fatal checks I can change it back.
>
piglit_check_gl_error() prints the file and line number of the check
that failed, to help you find it.
I'm not requiring a change here, just suggesting a way this has been
handled elsewhere.
>>> + check_gl_error(GL_NO_ERROR);
>> I'd drop the GL_NO_ERROR checks -- you know there's no error because the
>> last thing you did is check the errors.
> As far as I understand the errors are pushed into a stack. I do these no
> error checks to make sure the stack is empty before the critical call.
> If it's not possible for one gl call to push more than one error into
> the stack, then the no error checks can indeed be trimmed down in this test.
You can see the path in _mesa_error() -> _mesa_record_error() -- it's
just a single value, not a stack.
signature.asc
Description: PGP signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 1/4] ATI_fs: add api error tests
On 21/11/17 21:09, Eric Anholt wrote:
Miklós Máté writes:
One for each paragraph in the Errors section of the spec.
Signed-off-by: Miklós Máté
---
tests/spec/ati_fragment_shader/error01-genzero.c | 51
tests/spec/ati_fragment_shader/error02-inside.c| 59 +
tests/spec/ati_fragment_shader/error03-outside.c | 89 +++
tests/spec/ati_fragment_shader/error04-endshader.c | 93
tests/spec/ati_fragment_shader/error05-passes.c| 123 ++
.../spec/ati_fragment_shader/error06-regswizzle.c | 263 +
tests/spec/ati_fragment_shader/error07-instcount.c | 89 +++
tests/spec/ati_fragment_shader/error08-secondary.c | 82 +++
tests/spec/ati_fragment_shader/error09-allconst.c | 78 ++
tests/spec/ati_fragment_shader/error10-dotx.c | 115 +
.../spec/ati_fragment_shader/error11-invaliddst.c | 171 ++
.../spec/ati_fragment_shader/error12-invalidsrc.c | 151
.../spec/ati_fragment_shader/error13-invalidarg.c | 121 ++
.../spec/ati_fragment_shader/error14-invalidmod.c | 129 ++
We tend to implement these API error tests as a single .c file. As
you've experienced, this is a whole lot of copy and pasting, and it's
usually not too big of a deal to get the implementation to pass all of
them.
If it *is* hard to get the implementation to pass all of them, you may
look into piglit_report_subtest_result().
That said, there are some sizeable tests here and some nice usage of
subtests in a few of them, so I think you've generally made the right
choice.
Thanks for the review. I'll refine the code accordingly.
Also, don't forget to add your tests to all.py! If you don't, your
tests won't be run in everyone's regression testing. Also, the build
system for the tests should be in the same commit as the tests
themselves.
This is just a preview of my work, in the final version I'll add the
tests to all.py. I'll also remove your existing ati-fs-bad-delete test,
because it's a subset of my api-gen test :)
diff --git a/tests/spec/ati_fragment_shader/error01-genzero.c
b/tests/spec/ati_fragment_shader/error01-genzero.c
new file mode 100644
index 0..a19fb2ffc
--- /dev/null
+++ b/tests/spec/ati_fragment_shader/error01-genzero.c
@@ -0,0 +1,51 @@
+/* TODO license header */
You can see a sample of the license to use in the COPYING file -- just
copy and paste and change the date and "Intel Corporation" to your name.
OK, I'll use that license.
+
+/**
+ * Tests API errors for GL_ATI_fragment_shader.
+ * One for each paragraph in the Errors section.
+ */
I'd move the comment from piglit_init() to here.
Good idea, I'll do that.
+
+#include "piglit-util-gl.h"
+
+PIGLIT_GL_TEST_CONFIG_BEGIN
+
+ config.supports_gl_compat_version = 10;
+ config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
+
+PIGLIT_GL_TEST_CONFIG_END
+
+enum piglit_result
+piglit_display(void)
+{
+ /* UNREACHED */
+ return PIGLIT_FAIL;
+}
+
+#define check_gl_error(err) if (!piglit_check_gl_error(err))
piglit_report_result(PIGLIT_FAIL)
+
+void
+piglit_init(int argc, char **argv)
+{
+ unsigned id = 42;
Unnecessary initializer.
+ piglit_require_extension("GL_ATI_fragment_shader");
+
+ /*
+* Paragraph 1 of the Errors section:
+*
+* The error INVALID_VALUE is generated if GenFragmentShadersATI is
+* called where is zero.
+*/
+
+ id = glGenFragmentShadersATI(0);
+ check_gl_error(GL_INVALID_VALUE);
+
+ /* The spec also says that 0 is returned */
+ if (id != 0)
+ piglit_report_result(PIGLIT_FAIL);
+
+ /* Note that the spec also says that no shaders are generated,
+* but there is no way of testing that */
Our general style is that the closing */ of a multi-line comment goes on
its own line.
+
+ piglit_report_result(PIGLIT_PASS);
+}
diff --git a/tests/spec/ati_fragment_shader/error02-inside.c
b/tests/spec/ati_fragment_shader/error02-inside.c
new file mode 100644
index 0..5dee70cf6
--- /dev/null
+++ b/tests/spec/ati_fragment_shader/error02-inside.c
@@ -0,0 +1,59 @@
+/* TODO license header */
+
+/**
+ * Tests API errors for GL_ATI_fragment_shader.
+ * One for each paragraph in the Errors section.
+ */
+
+#include "piglit-util-gl.h"
+
+PIGLIT_GL_TEST_CONFIG_BEGIN
+
+ config.supports_gl_compat_version = 10;
+ config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
+
+PIGLIT_GL_TEST_CONFIG_END
+
+enum piglit_result
+piglit_display(void)
+{
+ /* UNREACHED */
+ return PIGLIT_FAIL;
+}
+
+#define check_gl_error(err) if (!piglit_check_gl_error(err))
piglit_report_result(PIGLIT_FAIL)
+
+void
+piglit_init(int argc, char **argv)
+{
+ piglit_require_extension("GL_ATI_fragment_shader");
+
+ /*
+* Paragraph 2 of the Errors section:
+*
+* The error INVALID_OPERATION is generated if GenFrag
Re: [Piglit] [PATCH 1/4] ATI_fs: add api error tests
Miklós Máté writes:
> One for each paragraph in the Errors section of the spec.
>
> Signed-off-by: Miklós Máté
> ---
> tests/spec/ati_fragment_shader/error01-genzero.c | 51
> tests/spec/ati_fragment_shader/error02-inside.c| 59 +
> tests/spec/ati_fragment_shader/error03-outside.c | 89 +++
> tests/spec/ati_fragment_shader/error04-endshader.c | 93
> tests/spec/ati_fragment_shader/error05-passes.c| 123 ++
> .../spec/ati_fragment_shader/error06-regswizzle.c | 263
> +
> tests/spec/ati_fragment_shader/error07-instcount.c | 89 +++
> tests/spec/ati_fragment_shader/error08-secondary.c | 82 +++
> tests/spec/ati_fragment_shader/error09-allconst.c | 78 ++
> tests/spec/ati_fragment_shader/error10-dotx.c | 115 +
> .../spec/ati_fragment_shader/error11-invaliddst.c | 171 ++
> .../spec/ati_fragment_shader/error12-invalidsrc.c | 151
> .../spec/ati_fragment_shader/error13-invalidarg.c | 121 ++
> .../spec/ati_fragment_shader/error14-invalidmod.c | 129 ++
We tend to implement these API error tests as a single .c file. As
you've experienced, this is a whole lot of copy and pasting, and it's
usually not too big of a deal to get the implementation to pass all of
them.
If it *is* hard to get the implementation to pass all of them, you may
look into piglit_report_subtest_result().
That said, there are some sizeable tests here and some nice usage of
subtests in a few of them, so I think you've generally made the right
choice.
Also, don't forget to add your tests to all.py! If you don't, your
tests won't be run in everyone's regression testing. Also, the build
system for the tests should be in the same commit as the tests
themselves.
> diff --git a/tests/spec/ati_fragment_shader/error01-genzero.c
> b/tests/spec/ati_fragment_shader/error01-genzero.c
> new file mode 100644
> index 0..a19fb2ffc
> --- /dev/null
> +++ b/tests/spec/ati_fragment_shader/error01-genzero.c
> @@ -0,0 +1,51 @@
> +/* TODO license header */
You can see a sample of the license to use in the COPYING file -- just
copy and paste and change the date and "Intel Corporation" to your name.
> +
> +/**
> + * Tests API errors for GL_ATI_fragment_shader.
> + * One for each paragraph in the Errors section.
> + */
I'd move the comment from piglit_init() to here.
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + /* UNREACHED */
> + return PIGLIT_FAIL;
> +}
> +
> +#define check_gl_error(err) if (!piglit_check_gl_error(err))
> piglit_report_result(PIGLIT_FAIL)
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + unsigned id = 42;
Unnecessary initializer.
> + piglit_require_extension("GL_ATI_fragment_shader");
> +
> + /*
> + * Paragraph 1 of the Errors section:
> + *
> + * The error INVALID_VALUE is generated if GenFragmentShadersATI is
> + * called where is zero.
> + */
> +
> + id = glGenFragmentShadersATI(0);
> + check_gl_error(GL_INVALID_VALUE);
> +
> + /* The spec also says that 0 is returned */
> + if (id != 0)
> + piglit_report_result(PIGLIT_FAIL);
> +
> + /* Note that the spec also says that no shaders are generated,
> + * but there is no way of testing that */
Our general style is that the closing */ of a multi-line comment goes on
its own line.
> +
> + piglit_report_result(PIGLIT_PASS);
> +}
> diff --git a/tests/spec/ati_fragment_shader/error02-inside.c
> b/tests/spec/ati_fragment_shader/error02-inside.c
> new file mode 100644
> index 0..5dee70cf6
> --- /dev/null
> +++ b/tests/spec/ati_fragment_shader/error02-inside.c
> @@ -0,0 +1,59 @@
> +/* TODO license header */
> +
> +/**
> + * Tests API errors for GL_ATI_fragment_shader.
> + * One for each paragraph in the Errors section.
> + */
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_compat_version = 10;
> + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> + /* UNREACHED */
> + return PIGLIT_FAIL;
> +}
> +
> +#define check_gl_error(err) if (!piglit_check_gl_error(err))
> piglit_report_result(PIGLIT_FAIL)
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + piglit_require_extension("GL_ATI_fragment_shader");
> +
> + /*
> + * Paragraph 2 of the Errors section:
> + *
> + * The error INVALID_OPERATION is generated if GenFragmentShadersATI,
> + * BindFragmentShaderATI, DeleteFragmentShaderATI, or
> + * BeginFragmentShaderATI are specified inside a
> + * Begin/EndFragmentShaderATI pair.
> +
