Re: [PATCH] Skip tests if tool/dependency is not installed

From: Glenn Washburn <development@efficientek.com>
To: Krishan Gopal <krishang@linux.ibm.com>
Cc: The development of GNU GRUB <grub-devel@gnu.org>
Subject: Re: [PATCH] Skip tests if tool/dependency is not installed
Date: Mon, 26 Feb 2024 21:31:00 -0600	[thread overview]
Message-ID: <20240226213100.049f3bf6@crass-HP-ZBook-15-G2> (raw)
In-Reply-To: <34650daf-00ff-4e0b-961e-20dbcf140121@linux.ibm.com>

On Wed, 14 Feb 2024 11:13:43 +0530
Krishan Gopal <krishang@linux.ibm.com> wrote:

> Hi Glenn,
> 
> On 02/02/24 2:59 am, Glenn Washburn wrote:
> > Hi Krishan,
> >
> > On Tue, 30 Jan 2024 04:18:13 -0600
> > Krishan Gopal Saraswat<krishang@linux.ibm.com>  wrote:
> >
> >> Currently many tests ends up in an ERROR state due to the environment not
> >> having relevant tools, majorly being due to different file systems.
> >>
> >> For example before this patch the ntfs test fails with the following error:
> >>
> >>      mkfs.ntfs not installed; cannot test ntfs.
> >>      ERROR ntfs_test (exit status: 99)
> >>
> >> This was caused due to it returning 99 exit status, which represents ERROR.
> >> If a particular tool is not installed/ dependecies not met, it should go to
> >> SKIP state.
> > This is working as intended. An exit code of 99 represents a "hard
> > error"[1], which is not a normal error and indicates a failure that
> > results from an improperly run test. This is to distinguish from errors
> > that we actually care about resulting from properly run tests. A
> > skipped test, exit code 77, are for tests that should not actually be
> > run, as in the case of tests that do not apply to the target being
> > used. This matters because those running tests should be notified that
> > they are not properly running all the tests (usually due to lack of
> > dependencies). Skipping those failing tests obscures this fact. The
> > point of the test suite is to have all the tests run, not to only run
> > the successful ones and ignore the rest.
> >
> > What problem does this patch actually solve?
> 
> Thanks for the review,
> 
> My view of SKIP/ERROR is the following.
> 
> Considering the following two conditions for SKIP: What ever not 
> supported on a particular platform [powerPC]. For example NTFS file 
> system is for Windows and not supported on Linux - in this case 
> expecting a SKIP instead of ERROR

Linux does indeed support the NTFS filesystem and has for a while. The
GRUB tests are intended to be run on a Linux kernel, and I've verified
that the NTFS tests work on an x86 build machine. I have no reason to
believe they would not work on PPC as well.

If a platform does not support a specific test, the test should be
skipped, but only for that platform. Do you have an example where a
test should be skipped and is not? If so, please send a patch.

> Considering the following for ERROR: 
> Expected tool is available but failed due to some error. This patch will 
> help in getting the correct output for a test execution that is not 
> supported.

Yes, if an expected tool exists but fails due to a bug, then the test
should return a HARD ERROR. The same should happen if the tool does not
exist.

I do not believe that this patch gives the correct output. Whether or
not test execution is supported has to do with if the test can be
supported with the state of existing tools. If you do not install the
NTFS tools that the NTFS filesystem tests need to function, that does
not mean the tests are not supported. It means you have an improper
environment for running those tests. Please read the INSTALL for the
list of packages needed on a Debian system for running  the tests.

Glenn

> 
> Thanks,
> 
> Krishan
> 
> >
> > Glenn
> >
> > [1]
> > https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html
> >
> >> After this patch, ntfs test skips if the tool is not available, with the
> >> following message:
> >>
> >>      mkfs.ntfs not installed; cannot test ntfs.
> >>      SKIP ntfs_test (exit status: 77)
> >>
> >> Many such test cases had similar failures and have been modified to SKIP state.
> >>
> >> The following test cases also had similar failures and have been modified from
> >> ERROR to SKIP state:
> >>      hfsplus_test, ntfs_test, reiserfs_test, f2fs_test, nilfs2_test, romfs_test, exfat_test, udf_test, hfs_test, jfs_test, btrfs_test, zfs_test, xfs_test
> >>
> >> Signed-off-by: Krishan Gopal Saraswat<krishang@linux.ibm.com>
> >> ---
> >>   btrfs_test.in    | 2 +-
> >>   exfat_test.in    | 2 +-
> >>   f2fs_test.in     | 2 +-
> >>   hfs_test.in      | 4 ++--
> >>   hfsplus_test.in  | 2 +-
> >>   jfs_test.in      | 2 +-
> >>   nilfs2_test.in   | 2 +-
> >>   ntfs_test.in     | 4 ++--
> >>   reiserfs_test.in | 2 +-
> >>   romfs_test.in    | 2 +-
> >>   udf_test.in      | 2 +-
> >>   xfs_test.in      | 2 +-
> >>   zfs_test.in      | 2 +-
> >>   13 files changed, 15 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/btrfs_test.in b/btrfs_test.in
> >> index 0d098c9..a07d2e5 100644
> >> --- a/btrfs_test.in
> >> +++ b/btrfs_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkfs.btrfs >/dev/null 2>&1; then
> >>      echo "mkfs.btrfs not installed; cannot test btrfs."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" btrfs
> >> diff --git a/exfat_test.in b/exfat_test.in
> >> index 7939f25..7acde19 100644
> >> --- a/exfat_test.in
> >> +++ b/exfat_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkfs.exfat >/dev/null 2>&1; then
> >>      echo "mkfs.exfat not installed; cannot test exFAT."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" exfat
> >> diff --git a/f2fs_test.in b/f2fs_test.in
> >> index 85f8cc8..a020a0f 100644
> >> --- a/f2fs_test.in
> >> +++ b/f2fs_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkfs.f2fs >/dev/null 2>&1; then
> >>    echo "mkfs.f2fs not installed; cannot test f2fs."
> >> - exit 99
> >> + exit 77
> >>   fi
> >>   
> >>   
> >> diff --git a/hfs_test.in b/hfs_test.in
> >> index 960f1cb..077683c 100644
> >> --- a/hfs_test.in
> >> +++ b/hfs_test.in
> >> @@ -12,12 +12,12 @@ fi
> >>   
> >>   if ! which mkfs.hfs >/dev/null 2>&1; then
> >>      echo "mkfs.hfs not installed; cannot test HFS."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   if ! grep -q mac_roman /proc/modules && ! modprobe mac_roman; then
> >>      echo "no mac-roman support; cannot test HFS."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" hfs
> >> diff --git a/hfsplus_test.in b/hfsplus_test.in
> >> index f727cf0..cb36a36 100644
> >> --- a/hfsplus_test.in
> >> +++ b/hfsplus_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkfs.hfsplus >/dev/null 2>&1; then
> >>      echo "mkfs.hfsplus not installed; cannot test hfsplus."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" hfsplus
> >> diff --git a/jfs_test.in b/jfs_test.in
> >> index d13780e..86f9ebe 100644
> >> --- a/jfs_test.in
> >> +++ b/jfs_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkfs.jfs >/dev/null 2>&1; then
> >>      echo "mkfs.jfs not installed; cannot test JFS."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" jfs
> >> diff --git a/nilfs2_test.in b/nilfs2_test.in
> >> index 8cc9375..719972f 100644
> >> --- a/nilfs2_test.in
> >> +++ b/nilfs2_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkfs.nilfs2 >/dev/null 2>&1; then
> >>      echo "mkfs.nilfs2 not installed; cannot test nilfs2."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" nilfs2
> >> diff --git a/ntfs_test.in b/ntfs_test.in
> >> index c2b08d2..da73c59 100644
> >> --- a/ntfs_test.in
> >> +++ b/ntfs_test.in
> >> @@ -12,12 +12,12 @@ fi
> >>   
> >>   if ! which mkfs.ntfs >/dev/null 2>&1; then
> >>      echo "mkfs.ntfs not installed; cannot test ntfs."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   if ! which setfattr >/dev/null 2>&1; then
> >>      echo "setfattr not installed; cannot test ntfs."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" ntfs
> >> diff --git a/reiserfs_test.in b/reiserfs_test.in
> >> index 37226c0..36e34c3 100644
> >> --- a/reiserfs_test.in
> >> +++ b/reiserfs_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkfs.reiserfs >/dev/null 2>&1; then
> >>      echo "mkfs.reiserfs not installed; cannot test reiserfs."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" reiserfs
> >> diff --git a/romfs_test.in b/romfs_test.in
> >> index f968e9b..98bb50c 100644
> >> --- a/romfs_test.in
> >> +++ b/romfs_test.in
> >> @@ -4,7 +4,7 @@ set -e
> >>   
> >>   if ! which genromfs >/dev/null 2>&1; then
> >>      echo "genromfs not installed; cannot test romfs."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" romfs
> >> diff --git a/udf_test.in b/udf_test.in
> >> index 302b28a..8968fb1 100644
> >> --- a/udf_test.in
> >> +++ b/udf_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkudffs >/dev/null 2>&1; then
> >>      echo "mkudffs not installed; cannot test UDF."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" udf
> >> diff --git a/xfs_test.in b/xfs_test.in
> >> index 5e029c1..8a648aa 100644
> >> --- a/xfs_test.in
> >> +++ b/xfs_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which mkfs.xfs >/dev/null 2>&1; then
> >>      echo "mkfs.xfs not installed; cannot test xfs."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   
> >> diff --git a/zfs_test.in b/zfs_test.in
> >> index 58cc25b..5d0f07d 100644
> >> --- a/zfs_test.in
> >> +++ b/zfs_test.in
> >> @@ -12,7 +12,7 @@ fi
> >>   
> >>   if ! which zpool >/dev/null 2>&1; then
> >>      echo "zpool not installed; cannot test zfs."
> >> -   exit 99
> >> +   exit 77
> >>   fi
> >>   
> >>   "@builddir@/grub-fs-tester" zfs

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