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

[Daniel Kiper]

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?

> 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