Re: [PATCH v2 4/8] tests: Fail immediately when grub-shell fails and do not occlude the error code

[Glenn Washburn <development@efficientek.com>]

On Thu, 7 Oct 2021 14:37:16 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Wed, Oct 06, 2021 at 03:05:57PM -0500, Glenn Washburn wrote:
> > On Wed, 6 Oct 2021 15:57:24 +0200
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Wed, Aug 25, 2021 at 02:03:58AM -0500, Glenn Washburn wrote:
> > > > If grub-shell fails, that means that whatever was being tested was not
> > > > actually tested. So fail immediately. Sometimes grub-shell is not the last
> > > > command in a pipeline of several commands, and in this case the failed error
> > > > code can be hidden by a later failing command or hidden when 'set -e' is not
> > > > set and there are subsequent successful commands. When the test script fails
> > > > because of a failure in grub-shell, then test script should exit with the
> > > > failed exit code of grub-shell.
> > > >
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > ---
> > > >  tests/ahci_test.in             | 6 +++++-
> > > >  tests/cdboot_test.in           | 3 ++-
> > > >  tests/core_compress_test.in    | 6 ++++--
> > > >  tests/ehci_test.in             | 6 +++++-
> > > >  tests/fddboot_test.in          | 3 ++-
> > > >  tests/grub_cmd_date.in         | 3 ++-
> > > >  tests/grub_cmd_test.in         | 1 +
> > > >  tests/grub_script_blockarg.in  | 2 +-
> > > >  tests/grub_script_expansion.in | 3 ++-
> > > >  tests/gzcompress_test.in       | 3 ++-
> > > >  tests/hddboot_test.in          | 3 ++-
> > > >  tests/lzocompress_test.in      | 3 ++-
> > > >  tests/netboot_test.in          | 3 ++-
> > > >  tests/ohci_test.in             | 6 +++++-
> > > >  tests/partmap_test.in          | 4 ++--
> > > >  tests/pata_test.in             | 3 ++-
> > > >  tests/test_sha512sum.in        | 1 +
> > > >  tests/uhci_test.in             | 6 +++++-
> > > >  tests/xzcompress_test.in       | 3 ++-
> > > >  19 files changed, 49 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/tests/ahci_test.in b/tests/ahci_test.in
> > > > index d844fe680..30dc9d31a 100644
> > > > --- a/tests/ahci_test.in
> > > > +++ b/tests/ahci_test.in
> > > > @@ -41,7 +41,11 @@ echo "hello" > "$outfile"
> > > >
> > > >  tar cf "$imgfile" "$outfile"
> > > >
> > > > -if [ "$(echo "nativedisk; source '(ahci0)/$outfile';" | "${grubshell}" --qemu-opts="-drive id=disk,file=$imgfile,if=none -device ahci,id=ahci -device ide-hd,drive=disk,bus=ahci.0 " | tail -n 1)" != "Hello World" ]; then
> > > > +echo "nativedisk; source '(ahci0)/$outfile';" |
> > > > +    "${grubshell}" --qemu-opts="-drive id=disk,file=$imgfile,if=none
> > > > +				-device ahci,id=ahci
> > > > +				-device ide-hd,drive=disk,bus=ahci.0" >$outfile
> > > > +if [ "$(cat "$outfile" | tail -n 1)" != "Hello World" ]; then
> > >
> > > This change is in line with the commit message...
> > >
> > > >     rm "$imgfile"
> > > >     rm "$outfile"
> > > >     exit 1
> > > > diff --git a/tests/cdboot_test.in b/tests/cdboot_test.in
> > > > index 75acdfedb..7229f79fb 100644
> > > > --- a/tests/cdboot_test.in
> > > > +++ b/tests/cdboot_test.in
> > > > @@ -34,6 +34,7 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
> > > >  	exit 0;;
> > > >  esac
> > > >
> > > > -if [ "$(echo hello | "${grubshell}" --boot=cd)" != "Hello World" ]; then
> > > > +v=`echo hello | "${grubshell}" --boot=cd`
> > > > +if [ "$v" != "Hello World" ]; then
> > >
> > > ... but this one is not. The grub-shell call is last one here. Hmmm...
> > > Am I missing something?
> >
> > This is the case where the error code is hidden, so 'set -e' doesn't
> > fail as it should.
> >
> > Note how this will not cause the script to error:
> >
> > $ bash -e -c 'if [ "$(echo XXX | ( cat; false ))" == "XXX" ]; then echo
> > `echo $$`; fi'
> >
> > But this will, which is what we want.
> >
> > $ bash -e -c 'v=`echo XXX | ( cat; false )`; if [ "$v" == "XXX" ]; then
> > echo `echo $$`; fi'
> 
> OK, AIUI error code is occluded by '['/test. Right?

Yes, although its not the fault of test. I believe it would be the case
for any "simple command" which uses subshells and one of the subshells
is returning an error exit code. For simple commands, only the exit
code of the primary command is checked for an error code.

For example:

  bash -e -c 'echo "$(echo XXX | ( cat; false ))"; echo ret=$?'

and

  bash -e -c 'cat <<<"$(echo XXX | ( cat; false ))"; echo ret=$?'

> 
> > So yes, this case, where error code is occluded with 'set -e', is not
> > described in the commit message. Should I update the commit message?
> 
> I think this patch should be split into two because these issues are two
> different things.
> 
> Daniel

Ok, I'll split it up.

Glenn