[PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Glenn Washburn]

Thomas Schmitt proposed variations on these fixes[1]. I've broken his
patch into several patches with improvements.

Glenn

[1] https://lore.kernel.org/all/9956308756800479343@scdbackup.webframe.org/

Glenn Washburn (4):
  tests/util/grub-shell-luks-tester: Add missing line to create RET
    variable in cleanup
  tests: Cleaup the cryptsetup script in grub_cmd_cryptomount unless
    debug is enabled
  tests: Default TMPDIR to /tmp in grub_cmd_cryptomount tests
  tests/util/grub-shell: Remove the work directory on successful run and
    debug is not on

 tests/grub_cmd_cryptomount.in        | 8 ++++++--
 tests/util/grub-shell-luks-tester.in | 1 +
 tests/util/grub-shell.in             | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 1/4] tests/util/grub-shell-luks-tester: Add missing line to create RET variable in cleanup

[Glenn Washburn]

Set the RET variable to the exit status of the script, as was assumed in the
cleanup() function.

Reported-by: Thomas Schmitt <scdbackup@gmx.net>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell-luks-tester.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/util/grub-shell-luks-tester.in b/tests/util/grub-shell-luks-tester.in
index b2a8a91b41f2..855ad39c19c3 100644
--- a/tests/util/grub-shell-luks-tester.in
+++ b/tests/util/grub-shell-luks-tester.in
@@ -143,6 +143,7 @@ fi
 
 # Make sure that the dm-crypto device is shutdown
 cleanup() {
+    RET=$?
     if [ -e "$luksdev" ]; then
         cryptsetup close "$luksdev"
     fi
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/4] tests/util/grub-shell-luks-tester: Add missing line to create RET variable in cleanup

[Thomas Schmitt via Grub-devel]

Hi,

(adding Vladimir Serbinenko to Cc)

Glenn Washburn wrote:
> --- a/tests/util/grub-shell-luks-tester.in
> +++ b/tests/util/grub-shell-luks-tester.in
> @@ -143,6 +143,7 @@ fi
>
>  # Make sure that the dm-crypto device is shutdown
>  cleanup() {
> +    RET=$?
>      if [ -e "$luksdev" ]; then
>          cryptsetup close "$luksdev"
>      fi

The change itself looks good to me.


But i think that there is
  set -e
missing in
  tests/util/grub-shell-luks-tester.in
and possibly in
  tests/grub_cmd_cryptomount.in

At least inserted commands:
  echo "grub_cmd_cryptomount.in: $(set -o | grep errexit)"
  echo "grub-shell-luks-tester.in: $(set -o | grep errexit)" >>/tmp/grub-shell-luks-tester.errexit.log
say in grub_cmd_cryptomount.log and grub-shell-luks-tester.errexit.log:
  grub_cmd_cryptomount: errexit           off
  grub-shell-luks-tester.in: errexit              off

The neighbors of grub-shell-luks-tester.in have such a "set -e":
  tests/util/grub-fs-tester.in
  tests/util/grub-shell-tester.in
  tests/util/grub-shell.in

Many tests/*.in have it too. So i propose to add it to
  tests/grub_cmd_cryptomount.in
as well.


I came to this while trying whether it is it guaranteed that "$?" is
always meaningful, given the fact that cleanup() can be called from
various places in the script by
  trap cleanup EXIT INT TERM KILL QUIT

My experiment was to press Ctrl+C while this shell code was sleeping

  cleanup() {
    RET=$?
    echo "RET=$RET"
  }
  trap cleanup EXIT INT TERM KILL QUIT
  sleep 10
  ls /notexisting

I got two output lines:

  RET=130
  RET=2

So without "set -e" cleanup() can be called more than once.
I think that grub-shell-luks-tester.in does not expects this.


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/4] tests/util/grub-shell-luks-tester: Add missing line to create RET variable in cleanup

[Thomas Schmitt via Grub-devel]

Hi,

please ignore my proposal about "set -e".

My assessment of "set -e" in
  tests/util/grub-shell-luks-tester.in
is obviously wrong, probably because "set -e" is not inherited by
sub-shells.

The line
  #! @BUILD_SHEBANG@ -e
enables script abort on command error, but causes reply
  errexit         on
only with
  echo set -o | grep errexit
but not with
  echo "grub-shell-luks-tester.in: $(set -o | grep errexit)" >>/tmp/grub-shell-luks-tester.errexit.log

Sorry for the noise.


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 2/4] tests: Cleaup the cryptsetup script in grub_cmd_cryptomount unless debug is enabled

[Glenn Washburn]

This fixes an issue where the grub_cmd_cryptomount test leaves a file
with an ambiguous name in the / directory when TMPDIR is not set.

Reported-by: Thomas Schmitt <scdbackup@gmx.net>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/grub_cmd_cryptomount.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/grub_cmd_cryptomount.in b/tests/grub_cmd_cryptomount.in
index f4d8f35473d0..5ddf89c08756 100644
--- a/tests/grub_cmd_cryptomount.in
+++ b/tests/grub_cmd_cryptomount.in
@@ -37,6 +37,8 @@ fi
 
 COMMON_OPTS='${V:+--debug=$V} --cs-opts="--pbkdf-force-iterations 1000"'
 
+debug=${GRUB_SHELL_DEFAULT_DEBUG:-$GRUB_TEST_DEFAULT_DEBUG}
+
 _testcase() {
     local EXPECTEDRES=$1
     local LOGPREFIX=$2
@@ -182,4 +184,5 @@ eval testcase "'LUKS2 test with second key slot and first slot using different p
     @builddir@/grub-shell-luks-tester $LUKS2_COMMON_OPTS $COMMON_OPTS \
         "--cs-script='$csscript'"
 
+test -n "$debug" || rm "$csscript"
 exit 0
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 3/4] tests: Default TMPDIR to /tmp in grub_cmd_cryptomount tests

[Glenn Washburn]

This fixes behavior where grub_cmd_cryptomount temporary files, which are
some times not cleaned up, are left in the / directory. Set TMPDIR if your
system does not have /tmp or it can not be used for some reason.

Reported-by: Thomas Schmitt <scdbackup@gmx.net>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/grub_cmd_cryptomount.in | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/grub_cmd_cryptomount.in b/tests/grub_cmd_cryptomount.in
index 5ddf89c08756..0566a5bcc101 100644
--- a/tests/grub_cmd_cryptomount.in
+++ b/tests/grub_cmd_cryptomount.in
@@ -47,8 +47,9 @@ _testcase() {
     shift 2
 
     # Create a subdir in TMPDIR for each testcase
-    _TMPDIR=$TMPDIR
-    TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
+    _TMPDIR=${TMPDIR:-/tmp}
+    TMPDIR=${_TMPDIR}/`echo -n "$(date +%s).${LOGPREFIX}" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
+    export TMPDIR
     mkdir -p "$TMPDIR"
 
     output=`"$@" 2>&1` || res=$?
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 4/4] tests/util/grub-shell: Remove the work directory on successful run and debug is not on

[Glenn Washburn]

This removes alot of empty grub-shell working directories in the TMPDIR
directory.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index ae5f711fe26c..98f7e7394778 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -711,6 +711,7 @@ test -n "$debug" || rm -f "${isofile}"
 test -n "$debug" || rm -rf "${rom_directory}"
 test -n "$debug" || rm -f "${tmpfile}" "${cfgfile}" "${goutfile}"
 test -n "$debug" || rm -f "$work_directory/run.sh"
+test -n "$debug" || rmdir "$work_directory"
 
 exit $ret
 
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Thomas Schmitt via Grub-devel]

Hi,

The patches apply without complaints by "git am".

But when i run (again as superuser, shudder):

  make check TESTS=grub_cmd_cryptomount

i still get in /tmp the empty direcories /tmp/17*.LUKS2_*.


Minor nitpick:
> [PATCH 2/4] tests: Cleaup the cryptsetup script in grub_cmd_cryptomount unless debug is enabled

Typo in subject: "Cleaup".


I think the reason for the remaining empty directories is that i do not
find in the patches any equivalent of this proposal of mine:

--- a/tests/grub_cmd_cryptomount.in
+++ b/tests/grub_cmd_cryptomount.in
@@ -46,10 +48,22 @@ _testcase() {
...
>     mkdir -p "$TMPDIR"
>
>     output=`"$@" 2>&1` || res=$?
> +
> +    if [ -z "$debug" ]; then
> +        if ! rmdir "$TMPDIR" >/dev/null 2>&1; then
> +            echo
> +            echo "Note: Temporary directory cannot be removed:"
> +            echo "        $TMPDIR"
> +            echo "      Please inspect and remove manually."
> +            echo
> +        fi
> +    fi
> +    unset TMPDIR
>      TMPDIR=$_TMPDIR
>
>     if [ "$res" -eq "$EXPECTEDRES" ]; then

Essential would be some  rmdir  to remove the directory created by
    mkdir -p "$TMPDIR"


If i add above proposed code to tests/grub_cmd_cryptomount.in then no
empty directories of form /tmp/17*.LUKS2_* remain.
See below for non-empty /tmp/1727105940.LUKS2_test_with_argon2_pbkdf .

I meanwhile forgot why i proposed
> +    unset TMPDIR
but there was some theoretical reason for this ...

----------------------------------------------------------------------
Possibly another problem to tackle:

One of the debris directories is not empty and thus would trigger the
"cannot be removed" warning:
  /tmp/1727104207.LUKS2_test_with_argon2_pbkdf
contains
  grub-shell-luks-tester.Pb6yqhgZSr

The offender is itself a directory with this content:

  -rw-r--r-- 1 root root 20971520 Sep 23 17:10 luks.disk
  -rw-r--r-- 1 root root        8 Sep 23 17:10 luks.key
  drwxr-xr-x 2 root root     4096 Sep 23 17:10 mnt
  -rw-r--r-- 1 root root     1203 Sep 23 17:10 testcase.cfg
  -rw-r--r-- 1 root root       23 Sep 23 17:10 testoutput
  -rw-r--r-- 1 root root      243 Sep 23 17:10 testvars

Subdirectory ./mnt is empty.

File
  grub_cmd_cryptomount.log
contains among many "PASS" lines:

  LUKS2 test with argon2 pbkdf: XFAIL

but nothing more enlightening.
The last line says
  PASS grub_cmd_cryptomount (exit status: 0)

-----------------------------------------------------------------------


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Glenn Washburn]

On Mon, 23 Sep 2024 17:52:00 +0200
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> Hi,
> 
> The patches apply without complaints by "git am".
> 
> But when i run (again as superuser, shudder):
> 
>   make check TESTS=grub_cmd_cryptomount
> 
> i still get in /tmp the empty direcories /tmp/17*.LUKS2_*.
> 
> 
> Minor nitpick:
> > [PATCH 2/4] tests: Cleaup the cryptsetup script in grub_cmd_cryptomount unless debug is enabled
> 
> Typo in subject: "Cleaup".

Thanks, I'll fix that.

> 
> 
> I think the reason for the remaining empty directories is that i do not
> find in the patches any equivalent of this proposal of mine:
> 
> --- a/tests/grub_cmd_cryptomount.in
> +++ b/tests/grub_cmd_cryptomount.in
> @@ -46,10 +48,22 @@ _testcase() {
> ...
> >     mkdir -p "$TMPDIR"
> >
> >     output=`"$@" 2>&1` || res=$?
> > +
> > +    if [ -z "$debug" ]; then
> > +        if ! rmdir "$TMPDIR" >/dev/null 2>&1; then
> > +            echo
> > +            echo "Note: Temporary directory cannot be removed:"
> > +            echo "        $TMPDIR"
> > +            echo "      Please inspect and remove manually."
> > +            echo
> > +        fi
> > +    fi
> > +    unset TMPDIR
> >      TMPDIR=$_TMPDIR
> >
> >     if [ "$res" -eq "$EXPECTEDRES" ]; then
> 
> Essential would be some  rmdir  to remove the directory created by
>     mkdir -p "$TMPDIR"

Yes, I forgot about the rmdir. I wasn't convinced I wanted that message,
but I'm warming up to the idea.

> 
> 
> If i add above proposed code to tests/grub_cmd_cryptomount.in then no
> empty directories of form /tmp/17*.LUKS2_* remain.
> See below for non-empty /tmp/1727105940.LUKS2_test_with_argon2_pbkdf .
> 
> I meanwhile forgot why i proposed
> > +    unset TMPDIR
> but there was some theoretical reason for this ...

My guess was that you wanted to unexport TMPDIR. I'm not convinced I
want to do that.

> 
> ----------------------------------------------------------------------
> Possibly another problem to tackle:
> 
> One of the debris directories is not empty and thus would trigger the
> "cannot be removed" warning:
>   /tmp/1727104207.LUKS2_test_with_argon2_pbkdf
> contains
>   grub-shell-luks-tester.Pb6yqhgZSr
> 
> The offender is itself a directory with this content:
> 
>   -rw-r--r-- 1 root root 20971520 Sep 23 17:10 luks.disk
>   -rw-r--r-- 1 root root        8 Sep 23 17:10 luks.key
>   drwxr-xr-x 2 root root     4096 Sep 23 17:10 mnt
>   -rw-r--r-- 1 root root     1203 Sep 23 17:10 testcase.cfg
>   -rw-r--r-- 1 root root       23 Sep 23 17:10 testoutput
>   -rw-r--r-- 1 root root      243 Sep 23 17:10 testvars
> 
> Subdirectory ./mnt is empty.

Yes, the mnt subdir is created by grub-shell-luks-tester is not deleted
by it. I'll add that to the next patch series iteration.

> 
> File
>   grub_cmd_cryptomount.log
> contains among many "PASS" lines:
> 
>   LUKS2 test with argon2 pbkdf: XFAIL

XFAIL means "expected fail", so its okay.

Thanks for looking at this.

Glenn

> 
> but nothing more enlightening.
> The last line says
>   PASS grub_cmd_cryptomount (exit status: 0)
> 
> -----------------------------------------------------------------------
> 
> 
> Have a nice day :)
> 
> Thomas
> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Thomas Schmitt via Grub-devel]

Hi,

i wrote:
> > I meanwhile forgot why i proposed
> > > +    unset TMPDIR
> > but there was some theoretical reason for this ...

Glenn Washburn wrote:
> My guess was that you wanted to unexport TMPDIR.

Yes. The reason was that i wanted to limit the effect of exporting to
the run of grub-shell-luks-tester, just in case that any further code
does not expect exported TMPDIR.


> >   LUKS2 test with argon2 pbkdf: XFAIL

> XFAIL means "expected fail", so its okay.

But why doesn't it clean up then ?
This particular sub-test leaves not only the empty ./mnt firectory but
also a 20 MB device image (?) and some small data files.

All other sub-tests of tests/grub_cmd_cryptomount leave a neatly empty
directory which grub_cmd_cryptomount can rmdir after the sub-test is done.


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Glenn Washburn]

On Thu, 26 Sep 2024 22:44:20 +0200
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> Hi,
> 
> i wrote:
> > > I meanwhile forgot why i proposed
> > > > +    unset TMPDIR
> > > but there was some theoretical reason for this ...
> 
> Glenn Washburn wrote:
> > My guess was that you wanted to unexport TMPDIR.
> 
> Yes. The reason was that i wanted to limit the effect of exporting to
> the run of grub-shell-luks-tester, just in case that any further code
> does not expect exported TMPDIR.

I think its fine in this case. But might be good for future proofing.
I'll think about it.

> 
> 
> > >   LUKS2 test with argon2 pbkdf: XFAIL
> 
> > XFAIL means "expected fail", so its okay.
> 
> But why doesn't it clean up then ?

This was an oversight. So grub-shell-luks-tester cleans up after
itself, if it returns success. grub_cmd_cryptomount has a test that
expects failure. But grub-shell-luks-tester doesn't know that this is
an expected failure and should cleanup and grub_cmd_cryptomount doesn't
ever cleanup after grub-shell-luks-tester. Perhaps
grub-shell-luks-tester should be passed a parameter to indicate
expected failure (eg. --xfail). Have any other ideas?

Glenn

> This particular sub-test leaves not only the empty ./mnt firectory but
> also a 20 MB device image (?) and some small data files.
> 
> All other sub-tests of tests/grub_cmd_cryptomount leave a neatly empty
> directory which grub_cmd_cryptomount can rmdir after the sub-test is done.
> 
> 
> Have a nice day :)
> 
> Thomas
> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Thomas Schmitt via Grub-devel]

Hi,

Glenn Washburn wrote:
> [...] grub-shell-luks-tester cleans up after
> itself, if it returns success. grub_cmd_cryptomount has a test that
> expects failure. But grub-shell-luks-tester doesn't know that this is
> an expected failure and should cleanup and grub_cmd_cryptomount doesn't
> ever cleanup after grub-shell-luks-tester. Perhaps
> grub-shell-luks-tester should be passed a parameter to indicate
> expected failure (eg. --xfail). Have any other ideas?

I agree to the idea of an option to invert the effect of
[ "$RET:-1" -eq 0 ] in cleanup() of tests/util/grub-shell-luks-tester.in .

Assuming variable "xfail" is be set to a non-empty string exactly if
argument "--xfail" is given, i'd replace:

    if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
        rm -rf "$lukstestdir" || :
    fi

by:

    if [ -z "$debug" ]; then
        if [ -n "$xfail" ]; then
            if [ "${RET:-0}" -ne 0 ]; then
                rm -rf "$lukstestdir" || :
            fi
        else
            if [ "${RET:-1}" -eq 0 ]; then
                rm -rf "$lukstestdir" || :
            fi
        fi
    fi

Equivalent, but heavily economizing on line count would be:

    if [ -z "$debug" ] && [ -n "$xfail" ] && [ "${RET:-0}" -ne 0 ]; then
        rm -rf "$lukstestdir" || :
    elif [ -z "$debug" ] && [ -z "$xfail" ] && [ "${RET:-1}" -eq 0 ]; then
        rm -rf "$lukstestdir" || :
    fi

I tested both code pieces in a dry-run script with the 12 variations of
debug={"", "1"} , xfail={"", "1"} , RET={undefined, "0", "1"} .
Decision for removal happened only with:
  (debug="", xfail="", RET="0")
  (debug="", xfail="1", RET="1")

The first code performs less []-expressions and seems clearer to me at
the price of doubled line count.


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Glenn Washburn]

On Fri, 27 Sep 2024 13:59:35 +0200
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> Hi,
> 
> Glenn Washburn wrote:
> > [...] grub-shell-luks-tester cleans up after
> > itself, if it returns success. grub_cmd_cryptomount has a test that
> > expects failure. But grub-shell-luks-tester doesn't know that this is
> > an expected failure and should cleanup and grub_cmd_cryptomount doesn't
> > ever cleanup after grub-shell-luks-tester. Perhaps
> > grub-shell-luks-tester should be passed a parameter to indicate
> > expected failure (eg. --xfail). Have any other ideas?
> 
> I agree to the idea of an option to invert the effect of
> [ "$RET:-1" -eq 0 ] in cleanup() of tests/util/grub-shell-luks-tester.in .
> 
> Assuming variable "xfail" is be set to a non-empty string exactly if
> argument "--xfail" is given, i'd replace:
> 
>     if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
>         rm -rf "$lukstestdir" || :
>     fi
> 
> by:
> 
>     if [ -z "$debug" ]; then
>         if [ -n "$xfail" ]; then
>             if [ "${RET:-0}" -ne 0 ]; then
>                 rm -rf "$lukstestdir" || :
>             fi
>         else
>             if [ "${RET:-1}" -eq 0 ]; then
>                 rm -rf "$lukstestdir" || :
>             fi
>         fi
>     fi
> 
> Equivalent, but heavily economizing on line count would be:
> 
>     if [ -z "$debug" ] && [ -n "$xfail" ] && [ "${RET:-0}" -ne 0 ]; then
>         rm -rf "$lukstestdir" || :
>     elif [ -z "$debug" ] && [ -z "$xfail" ] && [ "${RET:-1}" -eq 0 ]; then
>         rm -rf "$lukstestdir" || :
>     fi
> 
> I tested both code pieces in a dry-run script with the 12 variations of
> debug={"", "1"} , xfail={"", "1"} , RET={undefined, "0", "1"} .

RET should never be undefined because $? always has a numerical value.
And in my change I default xfail to 0. So I change the if statement to:

  if [ -z "$debug" ] && [ "$RET" -eq "$xfail" ]; then

This is a lot simpler, though perhaps a tad less clear. Do you see a
reason why this is less desirable?

Glenn

> Decision for removal happened only with:
>   (debug="", xfail="", RET="0")
>   (debug="", xfail="1", RET="1")
> 
> The first code performs less []-expressions and seems clearer to me at
> the price of doubled line count.
> 
> 
> Have a nice day :)
> 
> Thomas
> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Thomas Schmitt via Grub-devel]

Hi,

i wrote:
> > Assuming variable "xfail" is be set to a non-empty string exactly if
> > argument "--xfail" is given, i'd replace:
> >
> >     if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
> >         rm -rf "$lukstestdir" || :
> >     fi

Glenn Washburn wrote:
> RET should never be undefined because $? always has a numerical value.
> And in my change I default xfail to 0. So I change the if statement to:
>
>  if [ -z "$debug" ] && [ "$RET" -eq "$xfail" ]; then
>
> This is a lot simpler, though perhaps a tad less clear. Do you see a
> reason why this is less desirable?

If indeed in a successful --xfail test RET must have the value of "$xfail"
(i guess 1), then it is concise and sufficient.

If other non-zero values of RET are acceptable as indication of an
intended failure then your proposal is too sparse.
In this case i would propose to flatly enumerate the two cases which shall
lead to removal of the test data:

    if [ -z "$debug" ] && [ "$xfail" -eq 1 ] && [ "$RET" -ne 0 ]; then
        rm -rf "$lukstestdir" || :
    elif [ -z "$debug" ] && [ "$xfail" -eq 0 ] && [ "$RET" -eq 0 ]; then
        rm -rf "$lukstestdir" || :
    fi


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Glenn Washburn]

On Wed, 02 Oct 2024 09:08:23 +0200
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> Hi,
> 
> i wrote:
> > > Assuming variable "xfail" is be set to a non-empty string exactly if
> > > argument "--xfail" is given, i'd replace:
> > >
> > >     if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
> > >         rm -rf "$lukstestdir" || :
> > >     fi
> 
> Glenn Washburn wrote:
> > RET should never be undefined because $? always has a numerical value.
> > And in my change I default xfail to 0. So I change the if statement to:
> >
> >  if [ -z "$debug" ] && [ "$RET" -eq "$xfail" ]; then
> >
> > This is a lot simpler, though perhaps a tad less clear. Do you see a
> > reason why this is less desirable?
> 
> If indeed in a successful --xfail test RET must have the value of "$xfail"
> (i guess 1), then it is concise and sufficient.
> 
> If other non-zero values of RET are acceptable as indication of an
> intended failure then your proposal is too sparse.

Yes, after sending my last reply, I thought that maybe I'd been too
hasty in not considering other non-zero values of RET. However,
thinking about it more, the only acceptable xfail case is for RET=1. If
RET is greater than 1, that should be considered a hard error (iow a
failure of the test itself and not the thing being tested).

Glenn

> In this case i would propose to flatly enumerate the two cases which shall
> lead to removal of the test data:
> 
>     if [ -z "$debug" ] && [ "$xfail" -eq 1 ] && [ "$RET" -ne 0 ]; then
>         rm -rf "$lukstestdir" || :
>     elif [ -z "$debug" ] && [ "$xfail" -eq 0 ] && [ "$RET" -eq 0 ]; then
>         rm -rf "$lukstestdir" || :
>     fi
> 
> 
> Have a nice day :)
> 
> Thomas
> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/4] Various test fixes proposed by Thomas Schmitt

[Thomas Schmitt via Grub-devel]

Hi,

Glenn Washburn wrote:
> > >  if [ -z "$debug" ] && [ "$RET" -eq "$xfail" ]; then

I wrote:
> > If indeed in a successful --xfail test RET must have the value of "$xfail"
> > (i guess 1), then it is concise and sufficient.

> [...] the only acceptable xfail case is for RET=1. If
> RET is greater than 1, that should be considered a hard error (iow a
> failure of the test itself and not the thing being tested).

How about calling the variable "expected_ret" instead of "xfail" ?
This would express in the code that rogue return values are really
intended to prevent deletion of the test data.


Have a nice day

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel