Re: [Piglit] Allow junit to be used for summary generation

[Jose Fonseca]

On 09/10/15 01:21, baker.dyla...@gmail.com wrote:

This series updates the junit backend to allow it to properly load junit
and convert it back into piglit's internal representation, thus allowing
it to be summarized using the piglit summary tools

There is still some work that needs to be done beyond this, most of the
platform metadata isn't stored yet and restored, but I have a plan for
that. I have some other refactoring work that I think will make that
easier, and I'd like to get there before landing that.

This is enough to be able to compare junit and json results using the
console and html summaries.

There is a caveat here, and that's patch 3. To compare json and junit we
need to be able to restore the names of the junit tests to *exactly*
what they were before, and currently we don't have a way to reverse the
'.' -> '_' conversion. My proposal is to change '.' into '___', which is
very unlikely in a real test name (though we could change it to almost
anything that would be unique). This may break some existing setup
(Mark, I think this will probably break some of our expected fail/crash
data).



I don't object this, but instead of this brittle testname (de)munge, 
have you considered/tried using an additional XML attribute with 
unmodified piglit test name?  I expect that Jenkins junit parser will 
just outright ignore it.


Jose
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] gles-3.0: NV_read_depth extension test

[Tapani Pälli]

On 10/09/2015 10:56 AM, Tapani Pälli wrote:



On 10/09/2015 10:26 AM, Iago Toral wrote:

On Thu, 2015-10-08 at 15:13 +0300, Tapani Pälli wrote:

Signed-off-by: Tapani Pälli 
---
  tests/all.py |   5 +
  tests/spec/gles-3.0/CMakeLists.gles3.txt |   1 +
  tests/spec/gles-3.0/read-depth.c | 176
+++
  3 files changed, 182 insertions(+)
  create mode 100644 tests/spec/gles-3.0/read-depth.c

diff --git a/tests/all.py b/tests/all.py
index 5bc36d0..79d6314 100644
--- a/tests/all.py
+++ b/tests/all.py
@@ -4179,6 +4179,11 @@ with profile.group_manager(
  g(['khr_compressed_astc-miptree_gles2'], 'miptree-gles')

  with profile.group_manager(
+ PiglitGLTest,
+ grouptools.join('spec', 'nv_read_depth')) as g:
+g(['read_depth_gles3'])
+
+with profile.group_manager(
  PiglitGLTest,
  grouptools.join('spec', 'oes_compressed_paletted_texture'))
as g:
  g(['oes_compressed_paletted_texture-api'], 'basic API')
diff --git a/tests/spec/gles-3.0/CMakeLists.gles3.txt
b/tests/spec/gles-3.0/CMakeLists.gles3.txt
index d56a43e..864c862 100644
--- a/tests/spec/gles-3.0/CMakeLists.gles3.txt
+++ b/tests/spec/gles-3.0/CMakeLists.gles3.txt
@@ -6,5 +6,6 @@ piglit_add_executable (gles-3.0-drawarrays-vertexid
drawarrays-vertexid.c)
  piglit_add_executable(minmax_${piglit_target_api} minmax.c)
  piglit_add_executable(oes_compressed_etc2_texture-miptree_gles3
oes_compressed_etc2_texture-miptree.c)
  piglit_add_executable(texture-immutable-levels_gles3
texture-immutable-levels.c)
+piglit_add_executable(read_depth_gles3 read-depth.c)

  # vim: ft=cmake:
diff --git a/tests/spec/gles-3.0/read-depth.c
b/tests/spec/gles-3.0/read-depth.c
new file mode 100644
index 000..456c7e0
--- /dev/null
+++ b/tests/spec/gles-3.0/read-depth.c
@@ -0,0 +1,176 @@
+/*
+ * Copyright © 2015 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.
+ */
+
+/** @file read-depth.c
+ *
+ * Tests NV_read_depth implementation
+ *
+ * Test iterates over table of depth buffer formats and expected
types to
+ * read values back from each format. For each format it renders a
rectangle at
+ * different depth levels, reads back a pixel and verifies expected
depth value.
+ */
+
+#include "piglit-util-gl.h"
+
+PIGLIT_GL_TEST_CONFIG_BEGIN
+
+config.supports_gl_es_version = 30;
+config.window_visual = PIGLIT_GL_VISUAL_DEPTH;
+
+PIGLIT_GL_TEST_CONFIG_END
+
+static GLint prog;
+
+const char *vs_source =
+"attribute vec4 vertex;\n"
+"uniform float depth;\n"


Indentation


will fix




+"void main()\n"
+"{\n"
+"   gl_Position = vec4(vertex.xy, depth, 1.0);\n"
+"}\n";
+
+const char *fs_source =
+"void main()\n"
+"{\n"
+"   gl_FragColor = vec4(1.0, 1.0, 1.0, 1.0);\n"
+"}\n";
+
+const GLenum tests[] = {
+GL_DEPTH_COMPONENT16, GL_UNSIGNED_INT_24_8_OES,
+GL_DEPTH_COMPONENT24, GL_UNSIGNED_INT_24_8_OES,
+GL_DEPTH_COMPONENT32F, GL_FLOAT,
+};
+#define TESTS_SIZE (sizeof(tests)/sizeof(tests[0]))
+
+
+static bool
+equals(float a, float b)
+{
+   if (fabs(a - b) < 0.1)
+  return true;


Indentation (piglit is 8 spaces like you do in other places)


will fix


+   return false;
+}
+
+static GLuint
+create_depth_fbo(GLenum depth_type)
+{
+GLuint fbo, buffer;
+GLenum status;
+
+glGenRenderbuffers(1, );
+glBindRenderbuffer(GL_RENDERBUFFER, buffer);
+glRenderbufferStorage(GL_RENDERBUFFER,
+depth_type, piglit_width, piglit_height);


The secons line should be aligned with GL_RENDERBUFFER, you have this
same issue in every other place where you break a function call in two
lines.


Hmm, I did not know this style is used in Piglit. I'll go through some
files and fix if this is used by others too.


+glGenFramebuffers(1, );
+glBindFramebuffer(GL_FRAMEBUFFER, 

Re: [Piglit] [PATCH] gles-3.0: NV_read_depth extension test

[Tapani Pälli]

On 10/09/2015 10:26 AM, Iago Toral wrote:

On Thu, 2015-10-08 at 15:13 +0300, Tapani Pälli wrote:

Signed-off-by: Tapani Pälli 
---
  tests/all.py |   5 +
  tests/spec/gles-3.0/CMakeLists.gles3.txt |   1 +
  tests/spec/gles-3.0/read-depth.c | 176 +++
  3 files changed, 182 insertions(+)
  create mode 100644 tests/spec/gles-3.0/read-depth.c

diff --git a/tests/all.py b/tests/all.py
index 5bc36d0..79d6314 100644
--- a/tests/all.py
+++ b/tests/all.py
@@ -4179,6 +4179,11 @@ with profile.group_manager(
  g(['khr_compressed_astc-miptree_gles2'], 'miptree-gles')

  with profile.group_manager(
+ PiglitGLTest,
+ grouptools.join('spec', 'nv_read_depth')) as g:
+g(['read_depth_gles3'])
+
+with profile.group_manager(
  PiglitGLTest,
  grouptools.join('spec', 'oes_compressed_paletted_texture')) as g:
  g(['oes_compressed_paletted_texture-api'], 'basic API')
diff --git a/tests/spec/gles-3.0/CMakeLists.gles3.txt 
b/tests/spec/gles-3.0/CMakeLists.gles3.txt
index d56a43e..864c862 100644
--- a/tests/spec/gles-3.0/CMakeLists.gles3.txt
+++ b/tests/spec/gles-3.0/CMakeLists.gles3.txt
@@ -6,5 +6,6 @@ piglit_add_executable (gles-3.0-drawarrays-vertexid 
drawarrays-vertexid.c)
  piglit_add_executable(minmax_${piglit_target_api} minmax.c)
  piglit_add_executable(oes_compressed_etc2_texture-miptree_gles3 
oes_compressed_etc2_texture-miptree.c)
  piglit_add_executable(texture-immutable-levels_gles3 
texture-immutable-levels.c)
+piglit_add_executable(read_depth_gles3 read-depth.c)

  # vim: ft=cmake:
diff --git a/tests/spec/gles-3.0/read-depth.c b/tests/spec/gles-3.0/read-depth.c
new file mode 100644
index 000..456c7e0
--- /dev/null
+++ b/tests/spec/gles-3.0/read-depth.c
@@ -0,0 +1,176 @@
+/*
+ * Copyright © 2015 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.
+ */
+
+/** @file read-depth.c
+ *
+ * Tests NV_read_depth implementation
+ *
+ * Test iterates over table of depth buffer formats and expected types to
+ * read values back from each format. For each format it renders a rectangle at
+ * different depth levels, reads back a pixel and verifies expected depth 
value.
+ */
+
+#include "piglit-util-gl.h"
+
+PIGLIT_GL_TEST_CONFIG_BEGIN
+
+   config.supports_gl_es_version = 30;
+   config.window_visual = PIGLIT_GL_VISUAL_DEPTH;
+
+PIGLIT_GL_TEST_CONFIG_END
+
+static GLint prog;
+
+const char *vs_source =
+"attribute vec4 vertex;\n"
+   "uniform float depth;\n"


Indentation


will fix




+"void main()\n"
+"{\n"
+"   gl_Position = vec4(vertex.xy, depth, 1.0);\n"
+"}\n";
+
+const char *fs_source =
+"void main()\n"
+"{\n"
+"   gl_FragColor = vec4(1.0, 1.0, 1.0, 1.0);\n"
+"}\n";
+
+const GLenum tests[] = {
+   GL_DEPTH_COMPONENT16, GL_UNSIGNED_INT_24_8_OES,
+   GL_DEPTH_COMPONENT24, GL_UNSIGNED_INT_24_8_OES,
+   GL_DEPTH_COMPONENT32F, GL_FLOAT,
+};
+#define TESTS_SIZE (sizeof(tests)/sizeof(tests[0]))
+
+
+static bool
+equals(float a, float b)
+{
+   if (fabs(a - b) < 0.1)
+  return true;


Indentation (piglit is 8 spaces like you do in other places)


will fix


+   return false;
+}
+
+static GLuint
+create_depth_fbo(GLenum depth_type)
+{
+   GLuint fbo, buffer;
+   GLenum status;
+
+   glGenRenderbuffers(1, );
+   glBindRenderbuffer(GL_RENDERBUFFER, buffer);
+   glRenderbufferStorage(GL_RENDERBUFFER,
+   depth_type, piglit_width, piglit_height);


The secons line should be aligned with GL_RENDERBUFFER, you have this
same issue in every other place where you break a function call in two
lines.


Hmm, I did not know this style is used in Piglit. I'll go through some 
files and fix if this is used by others too.



+   glGenFramebuffers(1, );
+   

Re: [Piglit] [PATCH] gles-3.0: NV_read_depth extension test

[Iago Toral]

On Fri, 2015-10-09 at 11:00 +0300, Tapani Pälli wrote:
> 
> On 10/09/2015 10:56 AM, Tapani Pälli wrote:
> >
> >
> > On 10/09/2015 10:26 AM, Iago Toral wrote:
> >> On Thu, 2015-10-08 at 15:13 +0300, Tapani Pälli wrote:
> >>> Signed-off-by: Tapani Pälli 
> >>> ---
> >>>   tests/all.py |   5 +
> >>>   tests/spec/gles-3.0/CMakeLists.gles3.txt |   1 +
> >>>   tests/spec/gles-3.0/read-depth.c | 176
> >>> +++
> >>>   3 files changed, 182 insertions(+)
> >>>   create mode 100644 tests/spec/gles-3.0/read-depth.c
> >>>
> >>> diff --git a/tests/all.py b/tests/all.py
> >>> index 5bc36d0..79d6314 100644
> >>> --- a/tests/all.py
> >>> +++ b/tests/all.py
> >>> @@ -4179,6 +4179,11 @@ with profile.group_manager(
> >>>   g(['khr_compressed_astc-miptree_gles2'], 'miptree-gles')
> >>>
> >>>   with profile.group_manager(
> >>> + PiglitGLTest,
> >>> + grouptools.join('spec', 'nv_read_depth')) as g:
> >>> +g(['read_depth_gles3'])
> >>> +
> >>> +with profile.group_manager(
> >>>   PiglitGLTest,
> >>>   grouptools.join('spec', 'oes_compressed_paletted_texture'))
> >>> as g:
> >>>   g(['oes_compressed_paletted_texture-api'], 'basic API')
> >>> diff --git a/tests/spec/gles-3.0/CMakeLists.gles3.txt
> >>> b/tests/spec/gles-3.0/CMakeLists.gles3.txt
> >>> index d56a43e..864c862 100644
> >>> --- a/tests/spec/gles-3.0/CMakeLists.gles3.txt
> >>> +++ b/tests/spec/gles-3.0/CMakeLists.gles3.txt
> >>> @@ -6,5 +6,6 @@ piglit_add_executable (gles-3.0-drawarrays-vertexid
> >>> drawarrays-vertexid.c)
> >>>   piglit_add_executable(minmax_${piglit_target_api} minmax.c)
> >>>   piglit_add_executable(oes_compressed_etc2_texture-miptree_gles3
> >>> oes_compressed_etc2_texture-miptree.c)
> >>>   piglit_add_executable(texture-immutable-levels_gles3
> >>> texture-immutable-levels.c)
> >>> +piglit_add_executable(read_depth_gles3 read-depth.c)
> >>>
> >>>   # vim: ft=cmake:
> >>> diff --git a/tests/spec/gles-3.0/read-depth.c
> >>> b/tests/spec/gles-3.0/read-depth.c
> >>> new file mode 100644
> >>> index 000..456c7e0
> >>> --- /dev/null
> >>> +++ b/tests/spec/gles-3.0/read-depth.c
> >>> @@ -0,0 +1,176 @@
> >>> +/*
> >>> + * Copyright © 2015 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.
> >>> + */
> >>> +
> >>> +/** @file read-depth.c
> >>> + *
> >>> + * Tests NV_read_depth implementation
> >>> + *
> >>> + * Test iterates over table of depth buffer formats and expected
> >>> types to
> >>> + * read values back from each format. For each format it renders a
> >>> rectangle at
> >>> + * different depth levels, reads back a pixel and verifies expected
> >>> depth value.
> >>> + */
> >>> +
> >>> +#include "piglit-util-gl.h"
> >>> +
> >>> +PIGLIT_GL_TEST_CONFIG_BEGIN
> >>> +
> >>> +config.supports_gl_es_version = 30;
> >>> +config.window_visual = PIGLIT_GL_VISUAL_DEPTH;
> >>> +
> >>> +PIGLIT_GL_TEST_CONFIG_END
> >>> +
> >>> +static GLint prog;
> >>> +
> >>> +const char *vs_source =
> >>> +"attribute vec4 vertex;\n"
> >>> +"uniform float depth;\n"
> >>
> >> Indentation
> >
> > will fix
> >
> >>
> >>> +"void main()\n"
> >>> +"{\n"
> >>> +"   gl_Position = vec4(vertex.xy, depth, 1.0);\n"
> >>> +"}\n";
> >>> +
> >>> +const char *fs_source =
> >>> +"void main()\n"
> >>> +"{\n"
> >>> +"   gl_FragColor = vec4(1.0, 1.0, 1.0, 1.0);\n"
> >>> +"}\n";
> >>> +
> >>> +const GLenum tests[] = {
> >>> +GL_DEPTH_COMPONENT16, GL_UNSIGNED_INT_24_8_OES,
> >>> +GL_DEPTH_COMPONENT24, GL_UNSIGNED_INT_24_8_OES,
> >>> +GL_DEPTH_COMPONENT32F, GL_FLOAT,
> >>> +};
> >>> +#define 

Re: [Piglit] Allow junit to be used for summary generation

[Dylan Baker]

On Thu, Oct 8, 2015 at 6:09 PM, Mark Janes  wrote:

> baker.dyla...@gmail.com writes:
>
> > This series updates the junit backend to allow it to properly load junit
> > and convert it back into piglit's internal representation, thus allowing
> > it to be summarized using the piglit summary tools
> >
> > There is still some work that needs to be done beyond this, most of the
> > platform metadata isn't stored yet and restored, but I have a plan for
> > that. I have some other refactoring work that I think will make that
> > easier, and I'd like to get there before landing that.
> >
> > This is enough to be able to compare junit and json results using the
> > console and html summaries.
>
> I don't have a use case for comparing junit and json.  If anyone tried
> to compare json to our junit, they would see lots of differences because
> json does not respect the "expected-failures" filter.  That filter will
> also be unusable for json, because of the same test name character
> conversion.
>

I think that we should make the filter work for both. I'm not sure yet what
the right solution is for that, but I think it's worth making happen, I
think there are a lot of reasons for it, but the main one I see is that it
would eliminate distinctions between the backends.

I also have a series I'm working on that uses json for the intermediate
atomic writes, and then having a function that just converts the
TestrunResult object generated at the end of the run into the output we
want (like json or junit, or nunit, or whatever else). This reduces code
duplication a lot and reduces the number of code paths that aren't being
tested by me a lot. This is advantageous in unify the expected
failure/crash code to use the native piglit names rather than the junit
names, since we could apply it before converting the tests and writing them
out. That feels like the best solution to me, though it may mean some work
converting our existing crash/fail lists.


>
> > There is a caveat here, and that's patch 3. To compare json and junit we
> > need to be able to restore the names of the junit tests to *exactly*
> > what they were before, and currently we don't have a way to reverse the
> > '.' -> '_' conversion. My proposal is to change '.' into '___', which is
> > very unlikely in a real test name (though we could change it to almost
> > anything that would be unique). This may break some existing setup
> > (Mark, I think this will probably break some of our expected fail/crash
> > data).
>
> It seems to me that it will be simpler for everyone to disallow
> junit/json comparisons.  I just need a way for users to visualize a
> dozen junit test files for disparate platforms and test suites.
>

It might be, but I suspect that someone will try it at some point. I
imagine a workflow like:
1) developer gets a report about breaking some system
2) developer makes changes, and runs subset of piglit on their system
3) developer wants to compare those changes to the original junit


Dylan
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] gles-3.0: NV_read_depth extension test

[Iago Toral]

On Thu, 2015-10-08 at 15:13 +0300, Tapani Pälli wrote:
> Signed-off-by: Tapani Pälli 
> ---
>  tests/all.py |   5 +
>  tests/spec/gles-3.0/CMakeLists.gles3.txt |   1 +
>  tests/spec/gles-3.0/read-depth.c | 176 
> +++
>  3 files changed, 182 insertions(+)
>  create mode 100644 tests/spec/gles-3.0/read-depth.c
> 
> diff --git a/tests/all.py b/tests/all.py
> index 5bc36d0..79d6314 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -4179,6 +4179,11 @@ with profile.group_manager(
>  g(['khr_compressed_astc-miptree_gles2'], 'miptree-gles')
>  
>  with profile.group_manager(
> + PiglitGLTest,
> + grouptools.join('spec', 'nv_read_depth')) as g:
> +g(['read_depth_gles3'])
> +
> +with profile.group_manager(
>  PiglitGLTest,
>  grouptools.join('spec', 'oes_compressed_paletted_texture')) as g:
>  g(['oes_compressed_paletted_texture-api'], 'basic API')
> diff --git a/tests/spec/gles-3.0/CMakeLists.gles3.txt 
> b/tests/spec/gles-3.0/CMakeLists.gles3.txt
> index d56a43e..864c862 100644
> --- a/tests/spec/gles-3.0/CMakeLists.gles3.txt
> +++ b/tests/spec/gles-3.0/CMakeLists.gles3.txt
> @@ -6,5 +6,6 @@ piglit_add_executable (gles-3.0-drawarrays-vertexid 
> drawarrays-vertexid.c)
>  piglit_add_executable(minmax_${piglit_target_api} minmax.c)
>  piglit_add_executable(oes_compressed_etc2_texture-miptree_gles3 
> oes_compressed_etc2_texture-miptree.c)
>  piglit_add_executable(texture-immutable-levels_gles3 
> texture-immutable-levels.c)
> +piglit_add_executable(read_depth_gles3 read-depth.c)
>  
>  # vim: ft=cmake:
> diff --git a/tests/spec/gles-3.0/read-depth.c 
> b/tests/spec/gles-3.0/read-depth.c
> new file mode 100644
> index 000..456c7e0
> --- /dev/null
> +++ b/tests/spec/gles-3.0/read-depth.c
> @@ -0,0 +1,176 @@
> +/*
> + * Copyright © 2015 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.
> + */
> +
> +/** @file read-depth.c
> + *
> + * Tests NV_read_depth implementation
> + *
> + * Test iterates over table of depth buffer formats and expected types to
> + * read values back from each format. For each format it renders a rectangle 
> at
> + * different depth levels, reads back a pixel and verifies expected depth 
> value.
> + */
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_es_version = 30;
> + config.window_visual = PIGLIT_GL_VISUAL_DEPTH;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +static GLint prog;
> +
> +const char *vs_source =
> +"attribute vec4 vertex;\n"
> + "uniform float depth;\n"

Indentation

> +"void main()\n"
> +"{\n"
> +"   gl_Position = vec4(vertex.xy, depth, 1.0);\n"
> +"}\n";
> +
> +const char *fs_source =
> +"void main()\n"
> +"{\n"
> +"   gl_FragColor = vec4(1.0, 1.0, 1.0, 1.0);\n"
> +"}\n";
> +
> +const GLenum tests[] = {
> + GL_DEPTH_COMPONENT16, GL_UNSIGNED_INT_24_8_OES,
> + GL_DEPTH_COMPONENT24, GL_UNSIGNED_INT_24_8_OES,
> + GL_DEPTH_COMPONENT32F, GL_FLOAT,
> +};
> +#define TESTS_SIZE (sizeof(tests)/sizeof(tests[0]))
> +
> +
> +static bool
> +equals(float a, float b)
> +{
> +   if (fabs(a - b) < 0.1)
> +  return true;

Indentation (piglit is 8 spaces like you do in other places)

> +   return false;
> +}
> +
> +static GLuint
> +create_depth_fbo(GLenum depth_type)
> +{
> + GLuint fbo, buffer;
> + GLenum status;
> +
> + glGenRenderbuffers(1, );
> + glBindRenderbuffer(GL_RENDERBUFFER, buffer);
> + glRenderbufferStorage(GL_RENDERBUFFER,
> + depth_type, piglit_width, piglit_height);

The secons line should be aligned with GL_RENDERBUFFER, you have this
same issue in every other place where you break a function call in two
lines.

> + glGenFramebuffers(1, );

Re: [Piglit] [PATCH] gles-3.0: NV_read_depth extension test

[Ian Romanick]

On 10/09/2015 12:26 AM, Iago Toral wrote:
> On Thu, 2015-10-08 at 15:13 +0300, Tapani Pälli wrote:
>> Signed-off-by: Tapani Pälli 
>> +/* Step from -1.0 to 1.0, linear depth. Render a
>> + * rectangle at depth i, read pixel and verify
>> + * expected depth value.
>> + */
>> +for (i = -1.0; !equals(i, 1.0 + step); i += step) {
> 
> Since this is the only place where you call equals I think it is just
> easier to do:
> 
> for (i = -1.0; i < 1.1 + step; i += step) {

Why not just i <= 1.0?

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] tests/general: add draw-sync to all.py

[Marek Olšák]

Thanks, I'm going to push this in a moment.

Marek

On Sun, Sep 6, 2015 at 10:58 AM, Albert Freeman
 wrote:
> The draw-sync test (from 2011) was never placed in all.py.
>
> Signed-off-by: Albert Freeman 
> ---
> Changed "!opengl 1.5" to "!Opengl 1.1" as suggested by Eric Anholt.
> Made the test run concurrently.
>
>  tests/all.py | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/all.py b/tests/all.py
> index fcfc5cd..d8e526c 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -873,6 +873,7 @@ with profile.group_manager(
>  g(['draw-copypixels-sync'], run_concurrent=False)
>  g(['draw-pixel-with-texture'])
>  g(['drawpix-z'])
> +g(['draw-sync'])
>  g(['fog-modes'], run_concurrent=False)
>  g(['fragment-center'], run_concurrent=False)
>  g(['geterror-invalid-enum'], run_concurrent=False)
> --
> 2.5.1
>
> ___
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] Allow junit to be used for summary generation

[Mark Janes]

Jose Fonseca  writes:

> On 09/10/15 01:21, baker.dyla...@gmail.com wrote:
>> This series updates the junit backend to allow it to properly load junit
>> and convert it back into piglit's internal representation, thus allowing
>> it to be summarized using the piglit summary tools
>>
>> There is still some work that needs to be done beyond this, most of the
>> platform metadata isn't stored yet and restored, but I have a plan for
>> that. I have some other refactoring work that I think will make that
>> easier, and I'd like to get there before landing that.
>>
>> This is enough to be able to compare junit and json results using the
>> console and html summaries.
>>
>> There is a caveat here, and that's patch 3. To compare json and junit we
>> need to be able to restore the names of the junit tests to *exactly*
>> what they were before, and currently we don't have a way to reverse the
>> '.' -> '_' conversion. My proposal is to change '.' into '___', which is
>> very unlikely in a real test name (though we could change it to almost
>> anything that would be unique). This may break some existing setup
>> (Mark, I think this will probably break some of our expected fail/crash
>> data).
>>
>
> I don't object this, but instead of this brittle testname (de)munge, 
> have you considered/tried using an additional XML attribute with 
> unmodified piglit test name?  I expect that Jenkins junit parser will 
> just outright ignore it.

My recollection is that we found the junit parser to be strict:

https://svn.jenkins-ci.org/trunk/hudson/dtkit/dtkit-format/dtkit-junit-model/src/main/resources/com/thalesgroup/dtkit/junit/model/xsd/junit-4.xsd

>
> Jose
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] Allow junit to be used for summary generation

[Mark Janes]

Dylan Baker  writes:

> On Thu, Oct 8, 2015 at 6:09 PM, Mark Janes  wrote:
>
>> baker.dyla...@gmail.com writes:
>>
>> > This series updates the junit backend to allow it to properly load junit
>> > and convert it back into piglit's internal representation, thus allowing
>> > it to be summarized using the piglit summary tools
>> >
>> > There is still some work that needs to be done beyond this, most of the
>> > platform metadata isn't stored yet and restored, but I have a plan for
>> > that. I have some other refactoring work that I think will make that
>> > easier, and I'd like to get there before landing that.
>> >
>> > This is enough to be able to compare junit and json results using the
>> > console and html summaries.
>>
>> I don't have a use case for comparing junit and json.  If anyone tried
>> to compare json to our junit, they would see lots of differences because
>> json does not respect the "expected-failures" filter.  That filter will
>> also be unusable for json, because of the same test name character
>> conversion.
>>
>
> I think that we should make the filter work for both. I'm not sure yet what
> the right solution is for that, but I think it's worth making happen, I
> think there are a lot of reasons for it, but the main one I see is that it
> would eliminate distinctions between the backends.
>
> I also have a series I'm working on that uses json for the intermediate
> atomic writes, and then having a function that just converts the
> TestrunResult object generated at the end of the run into the output we
> want (like json or junit, or nunit, or whatever else). This reduces code
> duplication a lot and reduces the number of code paths that aren't being
> tested by me a lot. This is advantageous in unify the expected
> failure/crash code to use the native piglit names rather than the junit
> names, since we could apply it before converting the tests and writing them
> out. That feels like the best solution to me, though it may mean some work
> converting our existing crash/fail lists.
>
>
>>
>> > There is a caveat here, and that's patch 3. To compare json and junit we
>> > need to be able to restore the names of the junit tests to *exactly*
>> > what they were before, and currently we don't have a way to reverse the
>> > '.' -> '_' conversion. My proposal is to change '.' into '___', which is
>> > very unlikely in a real test name (though we could change it to almost
>> > anything that would be unique). This may break some existing setup
>> > (Mark, I think this will probably break some of our expected fail/crash
>> > data).
>>
>> It seems to me that it will be simpler for everyone to disallow
>> junit/json comparisons.  I just need a way for users to visualize a
>> dozen junit test files for disparate platforms and test suites.
>>
>
> It might be, but I suspect that someone will try it at some point. I
> imagine a workflow like:
> 1) developer gets a report about breaking some system
> 2) developer makes changes, and runs subset of piglit on their system
> 3) developer wants to compare those changes to the original junit
>

For the example workflow, the developer needs the pass/fail
configuration files.  Without them, the local results will not
correspond to what the CI produces.  We can provide those files, but
will developers really want to use them for their test runs?  The
configurations change several times a day as bugs are fixed or
regressions are written up as bugs.

If you want to solve the general problem, I think the most sensible
thing is to place limits on test names:

 * xml compatible (eg '<\/>&')
 * json compatible (eg '{}:')
 * config file compatible (eg '[]=:;#')
 * jenkins compatible ("api","search")

 and possibly even
 * command-line friendly (eg '()> &*')

Then, alter all existing problematic test names and assert that all
future test names conform.  Munging test names just adds confusion:  to
bisect a test, I'll need to reverse the '.' -> '__' transformation
before using names in --include-tests.

Personally, I find it irritating that so many test names incorporate
exotic characters.  But is it really worth the effort to rename
everything?  Will developers agree to the restrictions?  What about
BuildBot or other CI restrictions?

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] tests/general: add draw-sync to all.py

[Marek Olšák]

Pushed, thanks.

Marek

On Sun, Sep 6, 2015 at 10:58 AM, Albert Freeman
 wrote:
> The draw-sync test (from 2011) was never placed in all.py.
>
> Signed-off-by: Albert Freeman 
> ---
> Changed "!opengl 1.5" to "!Opengl 1.1" as suggested by Eric Anholt.
> Made the test run concurrently.
>
>  tests/all.py | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/all.py b/tests/all.py
> index fcfc5cd..d8e526c 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -873,6 +873,7 @@ with profile.group_manager(
>  g(['draw-copypixels-sync'], run_concurrent=False)
>  g(['draw-pixel-with-texture'])
>  g(['drawpix-z'])
> +g(['draw-sync'])
>  g(['fog-modes'], run_concurrent=False)
>  g(['fragment-center'], run_concurrent=False)
>  g(['geterror-invalid-enum'], run_concurrent=False)
> --
> 2.5.1
>
> ___
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] Allow junit to be used for summary generation

[Jose Fonseca]

On 09/10/15 17:44, Mark Janes wrote:

Jose Fonseca  writes:


On 09/10/15 01:21, baker.dyla...@gmail.com wrote:

This series updates the junit backend to allow it to properly load junit
and convert it back into piglit's internal representation, thus allowing
it to be summarized using the piglit summary tools

There is still some work that needs to be done beyond this, most of the
platform metadata isn't stored yet and restored, but I have a plan for
that. I have some other refactoring work that I think will make that
easier, and I'd like to get there before landing that.

This is enough to be able to compare junit and json results using the
console and html summaries.

There is a caveat here, and that's patch 3. To compare json and junit we
need to be able to restore the names of the junit tests to *exactly*
what they were before, and currently we don't have a way to reverse the
'.' -> '_' conversion. My proposal is to change '.' into '___', which is
very unlikely in a real test name (though we could change it to almost
anything that would be unique). This may break some existing setup
(Mark, I think this will probably break some of our expected fail/crash
data).



I don't object this, but instead of this brittle testname (de)munge,
have you considered/tried using an additional XML attribute with
unmodified piglit test name?  I expect that Jenkins junit parser will
just outright ignore it.


My recollection is that we found the junit parser to be strict:

https://svn.jenkins-ci.org/trunk/hudson/dtkit/dtkit-format/dtkit-junit-model/src/main/resources/com/thalesgroup/dtkit/junit/model/xsd/junit-4.xsd



Jose


In that case, we could try using the id attribute with the full 
qualified test-path + name, which should unique.


Jose
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] Allow junit to be used for summary generation

[Dylan Baker]

On Thu, Oct 08, 2015 at 06:09:16PM -0700, Mark Janes wrote:
> baker.dyla...@gmail.com writes:
> 
> > This series updates the junit backend to allow it to properly load junit
> > and convert it back into piglit's internal representation, thus allowing
> > it to be summarized using the piglit summary tools
> >
> > There is still some work that needs to be done beyond this, most of the
> > platform metadata isn't stored yet and restored, but I have a plan for
> > that. I have some other refactoring work that I think will make that
> > easier, and I'd like to get there before landing that.
> >
> > This is enough to be able to compare junit and json results using the
> > console and html summaries.
> 
> I don't have a use case for comparing junit and json.  If anyone tried
> to compare json to our junit, they would see lots of differences because
> json does not respect the "expected-failures" filter.  That filter will
> also be unusable for json, because of the same test name character
> conversion.
> 
> > There is a caveat here, and that's patch 3. To compare json and junit we
> > need to be able to restore the names of the junit tests to *exactly*
> > what they were before, and currently we don't have a way to reverse the
> > '.' -> '_' conversion. My proposal is to change '.' into '___', which is
> > very unlikely in a real test name (though we could change it to almost
> > anything that would be unique). This may break some existing setup
> > (Mark, I think this will probably break some of our expected fail/crash
> > data).
> 
> It seems to me that it will be simpler for everyone to disallow
> junit/json comparisons.  I just need a way for users to visualize a
> dozen junit test files for disparate platforms and test suites.


In the mean time, how do you feel about patches 1 and 2, or at least 2?

Getting 2 landed would allow junit to be summarized correctly right now,
and one would give more accurate timestamps (if people care about
those).

Dylan


signature.asc
Description: PGP signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] Allow junit to be used for summary generation

[Dylan Baker]

On Fri, Oct 09, 2015 at 09:44:01AM -0700, Mark Janes wrote:
> Jose Fonseca  writes:
> 
> > On 09/10/15 01:21, baker.dyla...@gmail.com wrote:
> >> This series updates the junit backend to allow it to properly load junit
> >> and convert it back into piglit's internal representation, thus allowing
> >> it to be summarized using the piglit summary tools
> >>
> >> There is still some work that needs to be done beyond this, most of the
> >> platform metadata isn't stored yet and restored, but I have a plan for
> >> that. I have some other refactoring work that I think will make that
> >> easier, and I'd like to get there before landing that.
> >>
> >> This is enough to be able to compare junit and json results using the
> >> console and html summaries.
> >>
> >> There is a caveat here, and that's patch 3. To compare json and junit we
> >> need to be able to restore the names of the junit tests to *exactly*
> >> what they were before, and currently we don't have a way to reverse the
> >> '.' -> '_' conversion. My proposal is to change '.' into '___', which is
> >> very unlikely in a real test name (though we could change it to almost
> >> anything that would be unique). This may break some existing setup
> >> (Mark, I think this will probably break some of our expected fail/crash
> >> data).
> >>
> >
> > I don't object this, but instead of this brittle testname (de)munge, 
> > have you considered/tried using an additional XML attribute with 
> > unmodified piglit test name?  I expect that Jenkins junit parser will 
> > just outright ignore it.
> 
> My recollection is that we found the junit parser to be strict:
> 
> https://svn.jenkins-ci.org/trunk/hudson/dtkit/dtkit-format/dtkit-junit-model/src/main/resources/com/thalesgroup/dtkit/junit/model/xsd/junit-4.xsd
> 
> >
> > Jose

I don't know if we tried, It definately allows some not strictly valid
junit to be passed to it (IIRC we didn't originally pass the total
number of tests in, and that is required per the SPEC).

If it will accept arbitrary tags that would be very nice. We might want
to guard them behind a --non-strict-junit switch (or inversely provide a
--strict-junit flag) in case that changes or someone whats to us a
consumer of junit that is strict.

I think I'll give it a try and see what happens.


signature.asc
Description: PGP signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] Allow junit to be used for summary generation

[Dylan Baker]

On Fri, Oct 09, 2015 at 10:33:35AM -0700, Dylan Baker wrote:
> On Fri, Oct 09, 2015 at 09:44:01AM -0700, Mark Janes wrote:
> > Jose Fonseca  writes:
> > 
> > > On 09/10/15 01:21, baker.dyla...@gmail.com wrote:
> > >> This series updates the junit backend to allow it to properly load junit
> > >> and convert it back into piglit's internal representation, thus allowing
> > >> it to be summarized using the piglit summary tools
> > >>
> > >> There is still some work that needs to be done beyond this, most of the
> > >> platform metadata isn't stored yet and restored, but I have a plan for
> > >> that. I have some other refactoring work that I think will make that
> > >> easier, and I'd like to get there before landing that.
> > >>
> > >> This is enough to be able to compare junit and json results using the
> > >> console and html summaries.
> > >>
> > >> There is a caveat here, and that's patch 3. To compare json and junit we
> > >> need to be able to restore the names of the junit tests to *exactly*
> > >> what they were before, and currently we don't have a way to reverse the
> > >> '.' -> '_' conversion. My proposal is to change '.' into '___', which is
> > >> very unlikely in a real test name (though we could change it to almost
> > >> anything that would be unique). This may break some existing setup
> > >> (Mark, I think this will probably break some of our expected fail/crash
> > >> data).
> > >>
> > >
> > > I don't object this, but instead of this brittle testname (de)munge, 
> > > have you considered/tried using an additional XML attribute with 
> > > unmodified piglit test name?  I expect that Jenkins junit parser will 
> > > just outright ignore it.
> > 
> > My recollection is that we found the junit parser to be strict:
> > 
> > https://svn.jenkins-ci.org/trunk/hudson/dtkit/dtkit-format/dtkit-junit-model/src/main/resources/com/thalesgroup/dtkit/junit/model/xsd/junit-4.xsd
> > 
> > >
> > > Jose
> 
> I don't know if we tried, It definately allows some not strictly valid
> junit to be passed to it (IIRC we didn't originally pass the total
> number of tests in, and that is required per the SPEC).
> 
> If it will accept arbitrary tags that would be very nice. We might want
> to guard them behind a --non-strict-junit switch (or inversely provide a
> --strict-junit flag) in case that changes or someone whats to us a
> consumer of junit that is strict.
> 
> I think I'll give it a try and see what happens.

Mark is correct, I added an extra tag and jenkins ignored the result. So
it is at least too strict for this use.


signature.asc
Description: PGP signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

[Piglit] Use subprocess32 for test timeouts, or fake it

[baker . dylan . c]

This changes the way that test timeouts work to be more robust than the
current implementation. This uses the external module subprocess32 which
provides the same interface as python 3.2+, a timeout parameter for
Popen.communicate. This is much simpler than our implementation, and has
the advantage of working correctly with threads, unlike our
implementation.

Since the interface is like python 3.x, this provides us a path forward
for building piglit has a hybrid python 2.x/3.x application, as we look
toward a python 3 only future.

Thomas, Daniel:
Will this work for you? I would really like to move forward with
building a hybrid python 2/3 codebase for piglit, and this would be
really nice, since it would make timeouts work very nicely in python 3.

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

[Piglit] [PATCH 1/2] framework/tests: add helper for checking for 3rd party modules

[baker . dylan . c]

From: Dylan Baker 

This little helper skips a test if a module isn't available.

Signed-off-by: Dylan Baker 
---
 framework/tests/junit_backends_tests.py | 3 +--
 framework/tests/utils.py| 9 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/framework/tests/junit_backends_tests.py 
b/framework/tests/junit_backends_tests.py
index 96335f3..40240c7 100644
--- a/framework/tests/junit_backends_tests.py
+++ b/framework/tests/junit_backends_tests.py
@@ -107,8 +107,7 @@ class TestJUnitSingleTest(TestJunitNoTests):
 
 def test_xml_valid(self):
 """backends.junit.JUnitBackend.write_test(): (once) produces valid 
xml"""
-if etree.__name__ != 'lxml.etree':
-raise SkipTest('Test requires lxml features')
+utils.module_check('lxml')
 schema = etree.XMLSchema(file=JUNIT_SCHEMA)
 with open(self.test_file, 'r') as f:
 nt.ok_(schema.validate(etree.parse(f)), msg='xml is not valid')
diff --git a/framework/tests/utils.py b/framework/tests/utils.py
index 2a9370c..aaf782c 100644
--- a/framework/tests/utils.py
+++ b/framework/tests/utils.py
@@ -34,6 +34,7 @@ import tempfile as tempfile_
 import functools
 import subprocess
 import errno
+import importlib
 from contextlib import contextmanager
 
 try:
@@ -314,6 +315,14 @@ def binary_check(bin_, errno_=None):
 e.returncode, errno_))
 
 
+def module_check(module):
+"""Check that an external module is available or skip."""
+try:
+importlib.import_module(module)
+except ImportError:
+raise SkipTest('Required module {} not available.'.format(module))
+
+
 def platform_check(plat):
 """If the platform is not in the list specified skip the test."""
 if not sys.platform.startswith(plat):
-- 
2.6.1

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

[Piglit] [PATCH 2/2] framework/test/base.py: try to use subprocess32

[baker . dylan . c]

From: Dylan Baker 

This adds subprocess32 as an optional dependency to get timeouts. This
module is a backport of the feature from python 3.2 that adds the
timeout argument to Popen.communicate (among others).

There is a caveat to this feature. Unlike the python 3.x version, this
doesn't work on windows, although that isn't a change from the current
state of affairs, as the current timeouts don't work on windows

What this does do though, is get us timeouts that work with concurrency
on posix systems (like Linux and OSX).

Signed-off-by: Dylan Baker 
---
 README|   5 +-
 framework/test/base.py| 123 +-
 framework/tests/base_tests.py |   2 +
 tox.ini   |   1 +
 4 files changed, 43 insertions(+), 88 deletions(-)

diff --git a/README b/README
index be8f4df..5813af5 100644
--- a/README
+++ b/README
@@ -47,9 +47,12 @@ Optionally, you can install the following:
   - lxml. An accelerated python xml library using libxml2 (http://lxml.de/)
   - simplejson. A fast C based implementation of the python json library.
 (https://simplejson.readthedocs.org/en/latest/)
-  - backports.lzma. A packport of python3's lzma module to python2,
+  - backports.lzma. A backport of python3's lzma module to python2,
 this enables fast native xz (de)compression in piglit for results files
 (https://github.com/peterjc/backports.lzma)
+  - subprocess32. A backport of python 3.2's subprocess module to python 2.
+This provides the timeout mechanism.
+(https://pypi.python.org/pypi/subprocess32/)
 
 Now configure the build system:
 
diff --git a/framework/test/base.py b/framework/test/base.py
index bf00396..0f01ccc 100644
--- a/framework/test/base.py
+++ b/framework/test/base.py
@@ -29,13 +29,33 @@ import subprocess
 import time
 import sys
 import traceback
-from datetime import datetime
-import threading
-import signal
 import itertools
 import abc
 import copy
 
+try:
+from subprocess32 import Popen, TimeoutExpired
+except ImportError:
+from subprocess import Popen as _Popen
+
+class Popen(_Popen):
+"""Wrapper to provide timout on communicate.
+
+This allows us to use the same code without a plethora of if statements
+to special case windows an non subprocess32 cases.
+
+"""
+def communicate(self, *args, **kwargs):
+try:
+del kwargs['timeout']
+except KeyError:
+pass
+
+super(Popen, self).communicate(*args, **kwargs)
+
+class TimeoutExpired(Exception):
+pass
+
 from framework import exceptions
 from framework.core import Options
 from framework.results import TestResult
@@ -62,56 +82,6 @@ class TestRunError(exceptions.PiglitException):
 self.status = status
 
 
-class ProcessTimeout(threading.Thread):
-""" Timeout class for test processes
-
-This class is for terminating tests that run for longer than a certain
-amount of time. Create an instance by passing it a timeout in seconds
-and the Popen object that is running your test. Then call the start
-method (inherited from Thread) to start the timer. The Popen object is
-killed if the timeout is reached and it has not completed. Wait for the
-outcome by calling the join() method from the parent.
-
-"""
-
-def __init__(self, timeout, proc):
-threading.Thread.__init__(self)
-self.proc = proc
-self.timeout = timeout
-self.status = 0
-
-def run(self):
-start_time = datetime.now()
-delta = 0
-
-# poll() returns the returncode attribute, which is either the return
-# code of the child process (which could be zero), or None if the
-# process has not yet terminated.
-
-while delta < self.timeout and self.proc.poll() is None:
-time.sleep(1)
-delta = (datetime.now() - start_time).total_seconds()
-
-# if the test is not finished after timeout, first try to terminate it
-# and if that fails, send SIGKILL to all processes in the test's
-# process group
-
-if self.proc.poll() is None:
-self.status = 1
-self.proc.terminate()
-time.sleep(5)
-
-if self.proc.poll() is None:
-self.status = 2
-if hasattr(os, 'killpg'):
-os.killpg(self.proc.pid, signal.SIGKILL)
-self.proc.wait()
-
-def join(self):
-threading.Thread.join(self)
-return self.status
-
-
 def _is_crash_returncode(returncode):
 """Determine whether the given process return code correspond to a
 crash.
@@ -147,8 +117,7 @@ class Test(object):
 """
 OPTS = Options()
 __metaclass__ = abc.ABCMeta
-__slots__ = ['run_concurrent', 'env', 'result', 'cwd', '_command',
- '__proc_timeout']
+__slots__ = ['run_concurrent', 'env', 

Re: [Piglit] [PATCH] gles-3.0: NV_read_depth extension test

[Ian Romanick]

On 10/08/2015 05:13 AM, Tapani Pälli wrote:
> Signed-off-by: Tapani Pälli 
> ---
>  tests/all.py |   5 +
>  tests/spec/gles-3.0/CMakeLists.gles3.txt |   1 +
>  tests/spec/gles-3.0/read-depth.c | 176 
> +++
>  3 files changed, 182 insertions(+)
>  create mode 100644 tests/spec/gles-3.0/read-depth.c
> 
> diff --git a/tests/all.py b/tests/all.py
> index 5bc36d0..79d6314 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -4179,6 +4179,11 @@ with profile.group_manager(
>  g(['khr_compressed_astc-miptree_gles2'], 'miptree-gles')
>  
>  with profile.group_manager(
> + PiglitGLTest,
> + grouptools.join('spec', 'nv_read_depth')) as g:
> +g(['read_depth_gles3'])
> +
> +with profile.group_manager(
>  PiglitGLTest,
>  grouptools.join('spec', 'oes_compressed_paletted_texture')) as g:
>  g(['oes_compressed_paletted_texture-api'], 'basic API')
> diff --git a/tests/spec/gles-3.0/CMakeLists.gles3.txt 
> b/tests/spec/gles-3.0/CMakeLists.gles3.txt
> index d56a43e..864c862 100644
> --- a/tests/spec/gles-3.0/CMakeLists.gles3.txt
> +++ b/tests/spec/gles-3.0/CMakeLists.gles3.txt
> @@ -6,5 +6,6 @@ piglit_add_executable (gles-3.0-drawarrays-vertexid 
> drawarrays-vertexid.c)
>  piglit_add_executable(minmax_${piglit_target_api} minmax.c)
>  piglit_add_executable(oes_compressed_etc2_texture-miptree_gles3 
> oes_compressed_etc2_texture-miptree.c)
>  piglit_add_executable(texture-immutable-levels_gles3 
> texture-immutable-levels.c)
> +piglit_add_executable(read_depth_gles3 read-depth.c)
>  
>  # vim: ft=cmake:
> diff --git a/tests/spec/gles-3.0/read-depth.c 
> b/tests/spec/gles-3.0/read-depth.c
> new file mode 100644
> index 000..456c7e0
> --- /dev/null
> +++ b/tests/spec/gles-3.0/read-depth.c
> @@ -0,0 +1,176 @@
> +/*
> + * Copyright © 2015 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.
> + */
> +
> +/** @file read-depth.c
> + *
> + * Tests NV_read_depth implementation
> + *
> + * Test iterates over table of depth buffer formats and expected types to
> + * read values back from each format. For each format it renders a rectangle 
> at
> + * different depth levels, reads back a pixel and verifies expected depth 
> value.
> + */
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + config.supports_gl_es_version = 30;

Do we actually need 3.0?  The spec appears to be written against ES2,
and I didn't notice anything in the test that would need 3.0.

> + config.window_visual = PIGLIT_GL_VISUAL_DEPTH;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +static GLint prog;
> +
> +const char *vs_source =
> +"attribute vec4 vertex;\n"
> + "uniform float depth;\n"

Weird indentation here.

> +"void main()\n"
> +"{\n"
> +"   gl_Position = vec4(vertex.xy, depth, 1.0);\n"
> +"}\n";
> +
> +const char *fs_source =
> +"void main()\n"
> +"{\n"
> +"   gl_FragColor = vec4(1.0, 1.0, 1.0, 1.0);\n"
> +"}\n";

I'm pretty sure that GLES2 can do depth-only rendering, so maybe we
don't need a fragment shader?

> +
> +const GLenum tests[] = {
> + GL_DEPTH_COMPONENT16, GL_UNSIGNED_INT_24_8_OES,
> + GL_DEPTH_COMPONENT24, GL_UNSIGNED_INT_24_8_OES,
> + GL_DEPTH_COMPONENT32F, GL_FLOAT,
> +};
> +#define TESTS_SIZE (sizeof(tests)/sizeof(tests[0]))

Just ARRAY_SIZE(), right?

> +
> +
> +static bool
> +equals(float a, float b)
> +{
> +   if (fabs(a - b) < 0.1)
> +  return true;
> +   return false;

Why not just 'return fabs(a - b) < 0.1;' ?

> +}
> +
> +static GLuint
> +create_depth_fbo(GLenum depth_type)
> +{
> + GLuint fbo, buffer;
> + GLenum status;
> +
> + glGenRenderbuffers(1, );
> + glBindRenderbuffer(GL_RENDERBUFFER, buffer);
> +