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

[Dylan Baker]

On Mon, Oct 05, 2015 at 08:03:09AM -0600, Brian Paul wrote:
> 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
> 
I think that piglit tests should always return the same number of
subtests unless they crash hard. So, I think I'll go with the second
plan. I'll try to have a V2 ready today.

Dylan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20151006/21b1c809/attachment.sig>