[PATCH v2 0/2] More test fixes

[PATCH v2 0/2] More test fixes

[Glenn Washburn]

Updates since v1:
* Rebase onto latest master

The first patch changes the test exit status from skipped to hard error, for
tests which fail due to missing dependencies in the environment.

The second is a trivial patch to parameterize all uses of parted in the
partmap_test.

Glenn

Glenn Washburn (2):
  tests: Test aborts due to missing requirements should be marked as
    error instead of skipped
  tests: In partmap_test, use ${parted} variable when checking for
    binary

 tests/btrfs_test.in       | 4 ++--
 tests/cpio_test.in        | 2 +-
 tests/exfat_test.in       | 4 ++--
 tests/ext234_test.in      | 8 ++++----
 tests/f2fs_test.in        | 4 ++--
 tests/fat_test.in         | 4 ++--
 tests/gzcompress_test.in  | 2 +-
 tests/hfs_test.in         | 6 +++---
 tests/hfsplus_test.in     | 4 ++--
 tests/iso9660_test.in     | 2 +-
 tests/jfs_test.in         | 4 ++--
 tests/lzocompress_test.in | 2 +-
 tests/minixfs_test.in     | 8 ++++----
 tests/nilfs2_test.in      | 4 ++--
 tests/ntfs_test.in        | 6 +++---
 tests/partmap_test.in     | 6 +++---
 tests/reiserfs_test.in    | 4 ++--
 tests/romfs_test.in       | 2 +-
 tests/squashfs_test.in    | 2 +-
 tests/tar_test.in         | 2 +-
 tests/udf_test.in         | 4 ++--
 tests/xfs_test.in         | 4 ++--
 tests/xzcompress_test.in  | 2 +-
 tests/zfs_test.in         | 4 ++--
 24 files changed, 47 insertions(+), 47 deletions(-)

Range-diff against v1:
1:  e2f9f26f2 ! 1:  6234f860a tests: Test aborts due to missing requirements should be marked as error instead of skipped
    @@ tests/gzcompress_test.in: grubshell=@builddir@/grub-shell
     +   exit 99
      fi
      
    - if [ "$(echo hello | "${grubshell}" --mkrescue-arg=--compress=gz)" != "Hello World" ]; then
    + v=$(echo hello | "${grubshell}" --mkrescue-arg=--compress=gz)
     
      ## tests/hfs_test.in ##
     @@ tests/hfs_test.in: if [ "x$EUID" = "x" ] ; then
    @@ tests/lzocompress_test.in: grubshell=@builddir@/grub-shell
     +   exit 99
      fi
      
    - if [ "$(echo hello | "${grubshell}" --mkrescue-arg=--compress=lzo)" != "Hello World" ]; then
    + v=$(echo hello | "${grubshell}" --mkrescue-arg=--compress=lzo)
     
      ## tests/minixfs_test.in ##
     @@ tests/minixfs_test.in: if [ "x$EUID" = "x" ] ; then
    @@ tests/xzcompress_test.in: grubshell=@builddir@/grub-shell
     +   exit 99
      fi
      
    - if [ "$(echo hello | "${grubshell}" --mkrescue-arg=--compress=xz)" != "Hello World" ]; then
    + v=$(echo hello | "${grubshell}" --mkrescue-arg=--compress=xz)
     
      ## tests/zfs_test.in ##
     @@ tests/zfs_test.in: if [ "x$EUID" = "x" ] ; then
2:  e35d3bb81 = 2:  d1aefde55 tests: In partmap_test, use ${parted} variable when checking for binary
-- 
2.27.0

[PATCH v2 1/2] tests: Test aborts due to missing requirements should be marked as error instead of skipped

[Glenn Washburn]

Many tests abort due to not being root or missing tools, for instance mkfs
commands for file system tests. The tests are exited with code 77, which
means they were skipped. A skipped test is a test that should not be run,
eg. a test specific to ARM64 should not be run on an x86 build. These aborts
are actually a hard error, code 99. That means that the test could not be
completed, but not because what was supposed to be tested failed, eg. in
these cases where a missing tool prevents the running of a test.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/btrfs_test.in       | 4 ++--
 tests/cpio_test.in        | 2 +-
 tests/exfat_test.in       | 4 ++--
 tests/ext234_test.in      | 8 ++++----
 tests/f2fs_test.in        | 4 ++--
 tests/fat_test.in         | 4 ++--
 tests/gzcompress_test.in  | 2 +-
 tests/hfs_test.in         | 6 +++---
 tests/hfsplus_test.in     | 4 ++--
 tests/iso9660_test.in     | 2 +-
 tests/jfs_test.in         | 4 ++--
 tests/lzocompress_test.in | 2 +-
 tests/minixfs_test.in     | 8 ++++----
 tests/nilfs2_test.in      | 4 ++--
 tests/ntfs_test.in        | 6 +++---
 tests/partmap_test.in     | 2 +-
 tests/reiserfs_test.in    | 4 ++--
 tests/romfs_test.in       | 2 +-
 tests/squashfs_test.in    | 2 +-
 tests/tar_test.in         | 2 +-
 tests/udf_test.in         | 4 ++--
 tests/xfs_test.in         | 4 ++--
 tests/xzcompress_test.in  | 2 +-
 tests/zfs_test.in         | 4 ++--
 24 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/tests/btrfs_test.in b/tests/btrfs_test.in
index 0c9bf3a68..0d098c9a2 100644
--- a/tests/btrfs_test.in
+++ b/tests/btrfs_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.btrfs >/dev/null 2>&1; then
    echo "mkfs.btrfs not installed; cannot test btrfs."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" btrfs
diff --git a/tests/cpio_test.in b/tests/cpio_test.in
index 5742cf17b..e2e668cf6 100644
--- a/tests/cpio_test.in
+++ b/tests/cpio_test.in
@@ -4,7 +4,7 @@ set -e
 
 if ! which cpio >/dev/null 2>&1; then
    echo "cpio not installed; cannot test cpio."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" cpio_bin
diff --git a/tests/exfat_test.in b/tests/exfat_test.in
index cd3cd4cb2..7939f25d2 100644
--- a/tests/exfat_test.in
+++ b/tests/exfat_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.exfat >/dev/null 2>&1; then
    echo "mkfs.exfat not installed; cannot test exFAT."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" exfat
diff --git a/tests/ext234_test.in b/tests/ext234_test.in
index 4f1eb527e..4df696710 100644
--- a/tests/ext234_test.in
+++ b/tests/ext234_test.in
@@ -7,22 +7,22 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.ext2 >/dev/null 2>&1; then
    echo "mkfs.ext2 not installed; cannot test ext2."
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.ext3 >/dev/null 2>&1; then
    echo "mkfs.ext3 not installed; cannot test ext3."
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.ext4 >/dev/null 2>&1; then
    echo "mkfs.ext4 not installed; cannot test ext4."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" ext2_old
diff --git a/tests/f2fs_test.in b/tests/f2fs_test.in
index 8c415db61..85f8cc8bc 100644
--- a/tests/f2fs_test.in
+++ b/tests/f2fs_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
- exit 77
+ exit 99
 fi
 
 if ! which mkfs.f2fs >/dev/null 2>&1; then
  echo "mkfs.f2fs not installed; cannot test f2fs."
- exit 77
+ exit 99
 fi
 
 
diff --git a/tests/fat_test.in b/tests/fat_test.in
index b6b4748ca..8a2b37c5c 100644
--- a/tests/fat_test.in
+++ b/tests/fat_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.vfat >/dev/null 2>&1; then
    echo "mkfs.vfat not installed; cannot test FAT."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" vfat16a
diff --git a/tests/gzcompress_test.in b/tests/gzcompress_test.in
index d7a594bb2..8e7e6a633 100644
--- a/tests/gzcompress_test.in
+++ b/tests/gzcompress_test.in
@@ -21,7 +21,7 @@ grubshell=@builddir@/grub-shell
 
 if ! which gzip >/dev/null 2>&1; then
    echo "gzip not installed; cannot test gzip compression."
-   exit 77
+   exit 99
 fi
 
 v=$(echo hello | "${grubshell}" --mkrescue-arg=--compress=gz)
diff --git a/tests/hfs_test.in b/tests/hfs_test.in
index 5b0c5e33e..960f1cbd0 100644
--- a/tests/hfs_test.in
+++ b/tests/hfs_test.in
@@ -7,17 +7,17 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.hfs >/dev/null 2>&1; then
    echo "mkfs.hfs not installed; cannot test HFS."
-   exit 77
+   exit 99
 fi
 
 if ! grep -q mac_roman /proc/modules && ! modprobe mac_roman; then
    echo "no mac-roman support; cannot test HFS."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" hfs
diff --git a/tests/hfsplus_test.in b/tests/hfsplus_test.in
index 85f1c37dc..f727cf0e2 100644
--- a/tests/hfsplus_test.in
+++ b/tests/hfsplus_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.hfsplus >/dev/null 2>&1; then
    echo "mkfs.hfsplus not installed; cannot test hfsplus."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" hfsplus
diff --git a/tests/iso9660_test.in b/tests/iso9660_test.in
index 571b938d7..ed0a5bf8d 100644
--- a/tests/iso9660_test.in
+++ b/tests/iso9660_test.in
@@ -4,7 +4,7 @@ set -e
 
 if ! which xorriso >/dev/null 2>&1; then
    echo "xorriso not installed; cannot test iso9660."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" joliet
diff --git a/tests/jfs_test.in b/tests/jfs_test.in
index 6cf7576b3..d13780e23 100644
--- a/tests/jfs_test.in
+++ b/tests/jfs_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.jfs >/dev/null 2>&1; then
    echo "mkfs.jfs not installed; cannot test JFS."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" jfs
diff --git a/tests/lzocompress_test.in b/tests/lzocompress_test.in
index 42e270df0..915f74bd9 100644
--- a/tests/lzocompress_test.in
+++ b/tests/lzocompress_test.in
@@ -21,7 +21,7 @@ grubshell=@builddir@/grub-shell
 
 if ! which lzop >/dev/null 2>&1; then
    echo "lzop not installed; cannot test lzo compression."
-   exit 77
+   exit 99
 fi
 
 v=$(echo hello | "${grubshell}" --mkrescue-arg=--compress=lzo)
diff --git a/tests/minixfs_test.in b/tests/minixfs_test.in
index 437d92df6..c62f56c8b 100644
--- a/tests/minixfs_test.in
+++ b/tests/minixfs_test.in
@@ -7,22 +7,22 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.minix >/dev/null 2>&1; then
    echo "mkfs.minix not installed; cannot test minixfs."
-   exit 77
+   exit 99
 fi
 
 if ! mkfs.minix -h | grep -- -v > /dev/null; then
     echo "mkfs.minix doesn't support minix2fs; cannot test minix*fs."
-   exit 77
+   exit 99
 fi
 
 if ! mkfs.minix -h | grep -- -3 > /dev/null; then
     echo "mkfs.minix doesn't support minix3fs; cannot test minix*fs."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" minix
diff --git a/tests/nilfs2_test.in b/tests/nilfs2_test.in
index ad44d5b33..8cc93754c 100644
--- a/tests/nilfs2_test.in
+++ b/tests/nilfs2_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.nilfs2 >/dev/null 2>&1; then
    echo "mkfs.nilfs2 not installed; cannot test nilfs2."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" nilfs2
diff --git a/tests/ntfs_test.in b/tests/ntfs_test.in
index 9eb7b01f6..c2b08d27f 100644
--- a/tests/ntfs_test.in
+++ b/tests/ntfs_test.in
@@ -7,17 +7,17 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.ntfs >/dev/null 2>&1; then
    echo "mkfs.ntfs not installed; cannot test ntfs."
-   exit 77
+   exit 99
 fi
 
 if ! which setfattr >/dev/null 2>&1; then
    echo "setfattr not installed; cannot test ntfs."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" ntfs
diff --git a/tests/partmap_test.in b/tests/partmap_test.in
index 0043bcf4c..bc2be78aa 100644
--- a/tests/partmap_test.in
+++ b/tests/partmap_test.in
@@ -98,7 +98,7 @@ esac
 
 if ! which parted >/dev/null 2>&1; then
    echo "parted not installed; cannot test partmap"
-   exit 77
+   exit 99
 fi
 
 imgfile="`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"`" || exit 99
diff --git a/tests/reiserfs_test.in b/tests/reiserfs_test.in
index 8ecfe740b..37226c01b 100644
--- a/tests/reiserfs_test.in
+++ b/tests/reiserfs_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.reiserfs >/dev/null 2>&1; then
    echo "mkfs.reiserfs not installed; cannot test reiserfs."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" reiserfs
diff --git a/tests/romfs_test.in b/tests/romfs_test.in
index 98bb50c32..f968e9b7d 100644
--- a/tests/romfs_test.in
+++ b/tests/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 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" romfs
diff --git a/tests/squashfs_test.in b/tests/squashfs_test.in
index 2f044f95d..15e70218f 100644
--- a/tests/squashfs_test.in
+++ b/tests/squashfs_test.in
@@ -4,7 +4,7 @@ set -e
 
 if ! which mksquashfs >/dev/null 2>&1; then
    echo "mksquashfs not installed; cannot test squashfs."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" squash4_gzip
diff --git a/tests/tar_test.in b/tests/tar_test.in
index 6e2f2de8b..97944b243 100644
--- a/tests/tar_test.in
+++ b/tests/tar_test.in
@@ -4,7 +4,7 @@ set -e
 
 if ! which tar >/dev/null 2>&1; then
    echo "tar not installed; cannot test tar."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" tarfs
diff --git a/tests/udf_test.in b/tests/udf_test.in
index fb92f0173..302b28ab2 100644
--- a/tests/udf_test.in
+++ b/tests/udf_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkudffs >/dev/null 2>&1; then
    echo "mkudffs not installed; cannot test UDF."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" udf
diff --git a/tests/xfs_test.in b/tests/xfs_test.in
index 03a351359..5e029c182 100644
--- a/tests/xfs_test.in
+++ b/tests/xfs_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which mkfs.xfs >/dev/null 2>&1; then
    echo "mkfs.xfs not installed; cannot test xfs."
-   exit 77
+   exit 99
 fi
 
 
diff --git a/tests/xzcompress_test.in b/tests/xzcompress_test.in
index cfc6ccba6..6ef73e41e 100644
--- a/tests/xzcompress_test.in
+++ b/tests/xzcompress_test.in
@@ -21,7 +21,7 @@ grubshell=@builddir@/grub-shell
 
 if ! which xz >/dev/null 2>&1; then
    echo "xz not installed; cannot test xz compression."
-   exit 77
+   exit 99
 fi
 
 v=$(echo hello | "${grubshell}" --mkrescue-arg=--compress=xz)
diff --git a/tests/zfs_test.in b/tests/zfs_test.in
index eee62c10d..58cc25b22 100644
--- a/tests/zfs_test.in
+++ b/tests/zfs_test.in
@@ -7,12 +7,12 @@ if [ "x$EUID" = "x" ] ; then
 fi
 
 if [ "$EUID" != 0 ] ; then
-   exit 77
+   exit 99
 fi
 
 if ! which zpool >/dev/null 2>&1; then
    echo "zpool not installed; cannot test zfs."
-   exit 77
+   exit 99
 fi
 
 "@builddir@/grub-fs-tester" zfs
-- 
2.27.0

[PATCH v2 2/2] tests: In partmap_test, use ${parted} variable when checking for binary

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/partmap_test.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/partmap_test.in b/tests/partmap_test.in
index bc2be78aa..a73c473b0 100644
--- a/tests/partmap_test.in
+++ b/tests/partmap_test.in
@@ -96,8 +96,8 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
 	;;
 esac
 
-if ! which parted >/dev/null 2>&1; then
-   echo "parted not installed; cannot test partmap"
+if ! which ${parted} >/dev/null 2>&1; then
+   echo "${parted} not installed; cannot test partmap"
    exit 99
 fi
 
-- 
2.27.0

Re: [PATCH v2 0/2] More test fixes

[Daniel Kiper]

On Thu, Oct 14, 2021 at 03:58:06PM -0500, Glenn Washburn wrote:
> Updates since v1:
> * Rebase onto latest master
>
> The first patch changes the test exit status from skipped to hard error, for
> tests which fail due to missing dependencies in the environment.
>
> The second is a trivial patch to parameterize all uses of parted in the
> partmap_test.
>
> Glenn
>
> Glenn Washburn (2):
>   tests: Test aborts due to missing requirements should be marked as
>     error instead of skipped
>   tests: In partmap_test, use ${parted} variable when checking for
>     binary

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v2 1/2] tests: Test aborts due to missing requirements should be marked as error instead of skipped

[Julian Andres Klode]

On Thu, Oct 14, 2021 at 03:58:07PM -0500, Glenn Washburn wrote:
> Many tests abort due to not being root or missing tools, for instance mkfs
> commands for file system tests. The tests are exited with code 77, which
> means they were skipped. A skipped test is a test that should not be run,
> eg. a test specific to ARM64 should not be run on an x86 build. These aborts
> are actually a hard error, code 99. That means that the test could not be
> completed, but not because what was supposed to be tested failed, eg. in
> these cases where a missing tool prevents the running of a test.

This change is inappropriate. We can't just fail the build in distro
packaging just because our builders don't run as root.

Similarly dependencies - it's our choice which dependencies we install,
if we don't care about stuff, it should just skip it.

Every distro will have to revert this change presumably so they can
build grub.

-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

Re: [PATCH v2 1/2] tests: Test aborts due to missing requirements should be marked as error instead of skipped

[Glenn Washburn]

On Wed, 19 Jul 2023 17:37:55 +0200
Julian Andres Klode <julian.klode@canonical.com> wrote:

> On Thu, Oct 14, 2021 at 03:58:07PM -0500, Glenn Washburn wrote:
> > Many tests abort due to not being root or missing tools, for instance mkfs
> > commands for file system tests. The tests are exited with code 77, which
> > means they were skipped. A skipped test is a test that should not be run,
> > eg. a test specific to ARM64 should not be run on an x86 build. These aborts
> > are actually a hard error, code 99. That means that the test could not be
> > completed, but not because what was supposed to be tested failed, eg. in
> > these cases where a missing tool prevents the running of a test.
> 
> This change is inappropriate.

Yes, I can imagine this change is inappropriate for the _existing_ code
that you (or your distro) has built around packaging GRUB. The previous
code was actually inappropriate for the GRUB tests. The needs of the
GRUB project and the needs of distros don't always align. I don't
believe that is the problem here though, where the issue is misplaced
blame.

Let me be clear here, the purpose of GRUB tests is not so distros can
run half of them after the build to see if it passes the smell test.
The purpose of the tests are primarily to test regressions and
functionality. To that end, the less tests that are run, the less
useful the test suite. So the testing system should be implemented to
try to run as many tests as possible and clearly note where tests are
not being run. Of course, you're free to run tests how ever you want,
but that does not mean that the GRUB project should support it,
especially when its in conflict with the interests of the project.

It occurs to me that perhaps the problematic implication of the previous
behavior was not evident, so let me try to clarify for you and anyone
else. A someone concerned with testing GRUB, I want to make sure that
"make check" does as much testing as is available and I want to know
with as much ease as possible when not all available testing is being
done. The previous approach did not cut it.

When I first started running the tests, before this change was
included, I would get a lot of skipped tests. I would then need to dig
into the individual test log to see why the test was skipped. Sometimes
the test was skipped because it was supposed to be skipped, maybe that
test did not apply to that architecture. I don't care about the skipped
tests that should be skipped, and its a waste of my time to dig into
those. Now tests that were skipped because of a misconfiguration of the
testing environment, those I do care about because the test _should_
run but is not. Now suppose I saw skipped tests due to an environment
issue and assumed they were supposed to be skipped, then I would be
unknowingly not running the full test suite, which defeats the purpose
of having those tests. The interpreting the test output should be clear
to someone not familiar with the tests. And false positive fails are
better than false negatives (not catching true failures).

This isn't a theoretical issue. There is someone on this list that has
several times graciously taken time to run the test suite and show the
test results to the list. And while I very much appreciate the
intention and work expended, in none of the cases was the output that
useful because half the tests were not run properly due to an improperly
testing environment. So in my mind it was mostly a waste of effort. By
making it clear via hard error that the test is a test that should be
run for the target, but is failing due to an environment issue, there
is more information at a glance about what's being tested or not. This
separates failed tests that are issues for GRUB, failed tests that are
the testers responsibility, and tests that are not run because they
should not be. You are advocating for going back to a time where those
last two classes are merged into one. In the interests of testing GRUB,
that is inappropriate.

> We can't just fail the build in distro
> packaging just because our builders don't run as root.

Then don't do that. The implication that you _need_ to revert to
improper test behavior is erroneous. Let assume that packaging GRUB
implies running the tests because really that is a good idea and that
your builders run unprivileged, also a good idea. The best option I see,
with the limited knowledge I have of your situation and to resolve your
issue without reverting the patch, is to check the test-suite.log file
for ERRORs. This is incredibly simple. Assuming you have a line in the
your debian rules file for "make check", you could changes this to
something like:

  make check || grep '^# ERROR: 0$' test-suite.log

This should get you the previous behavior. If you actually are serious
about running the tests, I've built a system which runs the root
required tests in UML. So I've got all the tests running as they should
on a build box where I have no access to root.

I'd also like to point out that even without this change, "make check"
failed, continues to fail, and will likely fail for the foreseeable
future. As I'm sure you're aware, the functional tests have been
failing for a long time, its a known issue, and no one has stepped up
to fix them (though a few have looked into it). So unless you have
patches fixing this (patches welcome!) or you're already patching GRUB
to not run this test, then "make check" is already failing for you.

> Similarly dependencies - it's our choice which dependencies we install,
> if we don't care about stuff, it should just skip it.

I agree, up to the last 5 words. If you want to control which tests are
run, it should not be done by abusing the automake testing system.
Non-existence of an executable or package should not be a signal for
disabling tests. Instead more appropriate way would be to specify
exactly which tests you want to run via the TESTS variable[1]. It comes
to mind that it might be nice to have an EXCLUDE_TESTS make variable
for specifying tests to exclude, to be more explicit that just having a
TESTS variable with some missing tests. Alternatively, propose a patch
to automake for a variable IGNORE_HARD_ERRORS, a complement to the
exiting DISABLE_HARD_ERRORS, which causes hard errors to affect the
return code of make check.

> Every distro will have to revert this change presumably so they can
> build grub.

Distros are free to modify upstream as they see fit, and do to the tune
of over a hundred patches at times. However, your presumption that there
is only one way to fix this is incorrect. As I noted above, this change
need not incur another patch to GRUB source, if the tooling is fixed.

Glenn

[1]
https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html