[Fuego] [PATCH 09/19] rebuild: make it simpler for test developers

Bird, Timothy Tim.Bird at sony.com
Tue May 16 04:52:45 UTC 2017



> -----Original Message-----
> From: fuego-bounces at lists.linuxfoundation.org [mailto:fuego-
> bounces at lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> Sent: Monday, May 15, 2017 9:35 PM
> To: Bird, Timothy <Tim.Bird at sony.com>; fuego at lists.linuxfoundation.org
> Subject: Re: [Fuego] [PATCH 09/19] rebuild: make it simpler for test
> developers
> 
> Hi Tim,
> 
> > -----Original Message-----
> > From: fuego-bounces at lists.linuxfoundation.org [mailto:fuego-
> bounces at lists.linuxfoundation.org] On Behalf Of Daniel Sangorrin
> > Sent: Tuesday, May 16, 2017 11:35 AM
> > > >  function test_build {
> > > > -    patch -p0 -N -s < $TEST_HOME/dhry_1.c.patch || return 1
> > > > +    patch -p0 -N -s < $TEST_HOME/dhry_1.c.patch || return
> > >
> > > Does this end up returning the return value from patch?
> >
> > Yes, return does not alter the $? variable as far as I know.
> >
> > > How does this interact with 'set -e'? I think this should work
> > > since bash doesn't trap command errors when the command
> > > is in a logic expression.  But does an error trap from the
> > > non-zero return from test_build?
> >
> > Before this patch, jobs were calling the "build_error" which
> > ended up aborting the job.
> >
> > Now, the return value of test_build is handled in the function "build"
> > using a logic expression (no error trap expected) which calls
> > aboart_job in case of error. So the way it works hasn't really changed.
> >
> > set -e will still trap errors in test_build that do not have a "|| return"
> expression.
> 
> I double checked this, an set -e will NOT trap them because the build has a ||
> expression.
> Therefore test writers need to use "|| return" to handle errors within
> test_build.
> 
> One alternative is to change
> call_if_present test_build && ret=0 || ret=$?
> for
> call_if_present test_build
> ret=$?
> 
> In this case, errors in test_build will be trap and marked as failure by the
> shell.
> Should I change it?

I think so.  I think a build error should result in the test being aborted, unless the
test author specifically captures it for some reason.

BTW - I'm not that comfortable with the error handling we have in the code.
set -e has all kinds of gotchas and special cases that IMHO are confusing and
hard to remember for most developers.  (see http://mywiki.wooledge.org/BashFAQ/105)
 I don't really want test developers to have become shell programming gurus.

However, I do think that that most of the time, we want to stop the test any time
a command or function called by the user fails, and it appears to me that either we
use 'set -e', or we require the user to constantly check return codes in their
own code (in the base script).  Maybe we just need to write up some guidelines
for test authors, to let them know the "rules" and some of the gotchas.

We should definitely document somewhere that if the user sees the signal
handler in their console log, something went wrong.  Maybe the signal handler
should have better and more prominent error messages to help inform the
user what happened, and maybe we should split the handlers (use different ones
for ERR, INT, etc.) so that we can give more detailed messages about what happened.

What do you think?
 -- Tim



More information about the Fuego mailing list