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 <[email protected]> > >Signed-off-by: Dylan Baker <[email protected]> > >--- > > > >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
signature.asc
Description: PGP signature
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
