[Piglit] [PATCH 2/2] clipflat: Fix subtest reporting

[Brian Paul]

On 09/30/2015 05:51 PM, Dylan Baker wrote:
> There are two problems with the way this test reports subtests:
>
> First, it only reports subtests when they fail. This has all kinds of
> problems. It's not at all what the summary code expects, the
> summary code expects that subtests are always present, which means that
> they will show up under disabled/enabled pages instead of
> fixes/regressions, which in turn, will confuse developers.
>
> Second, it tries to report lots of information as the subtest name. This
> causes the JSON parser in python (JSON is used by the tests to
> communicate with the runner) to choke and die.
>
> This patch makes a couple of changes to correct this.
> First, it reports the subtest result always, whether pass or fail.
> Second, it simplifies the names of the subtests, and passes the other
> information to stdout, which the runner will scoop up and record for a
> developer to go look at.
>
> cc: Brian Paul <brianp at vmware.com>
> Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
> ---
>
> Brian: I've tried to make the subtest reporting consistant, but I want
> to make sure that I'm not screwing up the logic of the test. I
> understand the idea of what it's doing, but not the details.
>
>
>   tests/general/clipflat.c | 44 ++++++++++++++++++++++++++------------------
>   1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/tests/general/clipflat.c b/tests/general/clipflat.c
> index 91e410e..f195185 100644
> --- a/tests/general/clipflat.c
> +++ b/tests/general/clipflat.c
> @@ -1,5 +1,6 @@
>   /*
>    * Copyright © 2009, 2013 VMware, Inc.
> + * 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"),
> @@ -317,12 +318,12 @@ checkResult(GLfloat badColor[3])
>
>
>   static void
> -reportFailure(GLenum mode, int drawMode, GLuint facing,
> +reportSubtest(GLenum mode, int drawMode, GLuint facing,
>                 GLuint fill,
> -              const GLfloat badColor[3], GLfloat x, GLfloat y)
> +              const GLfloat badColor[3], GLfloat x, GLfloat y,
> +              const bool fail)
>   {
>   	const char *m, *d, *f, *p;
> -	char msg1[100], msg2[100], msg3[100];
>
>   	switch (mode) {
>   	case GL_TRIANGLES:
> @@ -372,22 +373,23 @@ reportFailure(GLenum mode, int drawMode, GLuint facing,
>   	else
>   		p = "GL_LINE";
>
> -	snprintf(msg1, sizeof(msg1), "clipflat: Failure for %s(%s),"
> -		 " glFrontFace(%s), glPolygonMode(%s)\n",
> -		 d, m, f, p);
> +	if (fail == true) {

"if (fail)" would be sufficient.


> +		printf("clipflat: Failure for %s(%s), glFrontFace(%s), "
> +		       "glPolygonMode(%s)\n",
> +		       d, m, f, p);
>
> -	if (testing_first_pv)
> -		snprintf(msg2, sizeof(msg2),
> -			 "\tGL_EXT_provoking_vertex test: "
> -			 "GL_FIRST_VERTEX_CONVENTION_EXT mode\n");
> -	else
> -		msg2[0] = 0;
> +		if (testing_first_pv) {
> +		    printf("\tGL_EXT_provoking_vertex test: "
> +			   "GL_FIRST_VERTEX_CONVENTION_EXT mode\n");
> +		}
>
> -	snprintf(msg3, sizeof(msg3),
> -		 "\tExpected color (0, 1, 0) but found (%g, %g, %g)\n",
> -		 badColor[0], badColor[1], badColor[2]);
> +		printf("Expected color (0, 1, 0) but found (%g, %g, %g)\n",
> +		       badColor[0], badColor[1], badColor[2]);
> +	}
>
> -	piglit_report_subtest_result(PIGLIT_FAIL, "%s%s%s", msg1, msg2, msg3);
> +	piglit_report_subtest_result(fail ? PIGLIT_FAIL : PIGLIT_PASS,
> +				     "%s(%s), glFrontFace(%s), glPolygonMode(%s)",
> +				     d, m, f, p);
>   }
>
>
> @@ -449,11 +451,17 @@ testPrim(GLenum mode, const GLfloat *verts, GLuint count)
>   						glPopMatrix();
>
>   						if (!checkResult(badColor)) {
> -							reportFailure(mode, drawMode, facing, fill, badColor, x, y);
> -							return false;
> +							reportSubtest(mode, drawMode, facing, fill,
> +							  	      badColor, x, y, true);
> +							goto failed;
>   						}
>   					}
>   				}
> +
> +				GLfloat badColor[3];
> +				reportSubtest(mode, drawMode, facing, fill,
> +					      badColor, x, y, false);
> +				failed: continue;
>   			}
>   		}
>   	}
>

This part and the goto seem a bit clunky.  Could you try this instead:

pass = checkResult(badColor);

reportSubtest(mode, drawMode, facing, fill,
	      badColor, x, y, !pass);
if (!pass)
    return false;


Alternately, maybe we should keep testing the 'fill', 'drawMode', 
'facing' and position combinations in testPrim() even when there's one 
failure (so we produce the same number of subresults regardless of any 
failures.  In that case, we'd replace the conditional with:

bool result = true;   -- declare this at top of function

if (!pass)
    result = false;
...

return result;   -- at end of function.


What do you think?

-Brian