[Piglit] [PATCH] framework/exectest.py: Set returncode to 0 when executable is missing.

[Jose Fonseca]

----- Original Message -----
> On Tuesday, June 10, 2014 08:16:04 PM jfonseca at vmware.com wrote:
> > From: José Fonseca <jfonseca at vmware.com>
> > 
> > Just like piglit_report_result(PIGLIT_SKIP) does.
> > 
> > Otherwise these tests are considered failures or -- with my commit
> > 058e0f8a1e536b68ef43d27ada80645845a39e19 -- crashes.
> > ---
> >  framework/exectest.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/framework/exectest.py b/framework/exectest.py
> > index a833066..a5e06ae 100644
> > --- a/framework/exectest.py
> > +++ b/framework/exectest.py
> > @@ -205,7 +205,7 @@ class Test(object):
> >                  out = ("PIGLIT: {'result': 'skip'}\n"
> >                         "Test executable not found.\n")
> >                  err = ""
> > -                returncode = None
> > +                returncode = 0
> >              else:
> >                  raise e
> 
> I'm not excited by this, tests that don't run shouldn't have return codes,
> especially not the 'everything is fine!' return code.

All tests that call `piglit_report_result(PIGLIT_SKIP)` return the 'everything is fine!' return code.  I don't see any difference.

What these lines of code were trying to do is "fake" that a test with missing executable was run and skipped: it fakes the stdout of a skipped test, but it didn't fake the returncode of a skipped test properly (which is 0).

And because the stdout fakes as skipped result, it will be clearly marked as skipped tests -- which is clearly different from the "passed" already -- it needs no further separation IMO

> I'd prefer to fix the check that 058e0f8a1e536b68ef43d27ada80645845a39e19
> broke by replacing:
> if self.result['returncode'] < 0
> with
> if self.result['returncode'] is not None and self.result['returncode'] < 0

I find this quite inconsistent, and onerous (one now needs to check returncode against None all over the place, due to the peuliar None semantics it's quite easy for bugs to creep in).

IMO skipped tests should be consistent with the other skipped tests.   If you are afraid of lumping skipped tests with zero return code, then you should really defined a new return code just for skipped tests (I recommend 125 like used in `git bisect`), and return it everywhere, including when `piglit_report_result(PIGLIT_SKIP)` is called.

Or, if you think that missing test executables are such a special class of its own, then we should create a new status.MISSING_EXECUTABLE for those tests.

That's my 2c.  But if you really want None returncodes, I can do that too...

Jose