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

[Daniel Sangorrin]

> -----Original Message-----
> From: Bird, Timothy [mailto:Tim.Bird at sony.com]
> Sent: Tuesday, May 16, 2017 1:53 PM
> To: Daniel Sangorrin; fuego at lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 09/19] rebuild: make it simpler for test developers
> 
> 
> 
> > -----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.

OK, I just sent a patch and pushed it to my next branch.

> 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.

Yes, I agree. I'm not a shell programming guru either, and I don't want to become 
one to write code that no one else can read.

> 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?

In general I think that errors caused by the tests themselves need be handled properly
and written in the final json output.

Errors caused by set -e should normally be BUGs, either in the core code or in the test code, and
happen as rarely as possible. I think that set -e should be only enabled when FUEGO_DEBUG is defined.

Thanks,
Daniel