Re: [PATCH 1/1] test: bootflow test depends on CONFIG_VIDEO

[Heinrich Schuchardt]

On 5/31/23 23:16, Simon Glass wrote:
Hi Heinrich,

On Wed, 31 May 2023 at 14:22, Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:



On 5/31/23 21:52, Simon Glass wrote:
Hi Heinrich,

On Wed, 31 May 2023 at 13:27, Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:



On 5/31/23 20:24, Tom Rini wrote:
On Wed, May 31, 2023 at 08:05:18PM +0200, Heinrich Schuchardt wrote:


On 5/31/23 19:13, Simon Glass wrote:
Hi Heinrich,

On Wed, 31 May 2023 at 09:52, Tom Rini <tr...@konsulko.com> wrote:

On Wed, May 31, 2023 at 04:25:46PM +0200, Heinrich Schuchardt wrote:
Tom Rini <tr...@konsulko.com> schrieb am Mi., 31. Mai 2023, 16:02:

On Wed, May 31, 2023 at 10:50:52AM +0200, Heinrich Schuchardt wrote:

qemu_arm64_defconfig with UNIT_TEST=y does not build.

CONFIG_EXPO depends on CONFIG_VIDEO. Hence on platforms without video we
must not enable unit tests invoking expo functions.

Fixes: fb1451bec2a5 ("bootstd: Add tests for bootstd including all
uclasses")
Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
---
     test/boot/Makefile | 5 ++++-
     1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/test/boot/Makefile b/test/boot/Makefile
index 22ed61c8fa..665017ba3d 100644
--- a/test/boot/Makefile
+++ b/test/boot/Makefile
@@ -2,7 +2,10 @@
     #
     # Copyright 2021 Google LLC

-obj-$(CONFIG_BOOTSTD) += bootdev.o bootstd_common.o bootflow.o
bootmeth.o
+ifdef CONFIG_BOOTSTD
+obj-y += bootstd_common.o bootmeth.o
+obj-$(VIDEO) += bootdev.o bootflow.o
+endif
     obj-$(CONFIG_FIT) += image.o

     obj-$(CONFIG_EXPO) += expo.o

OK, but the functionality itself doesn't depend on video, it looks like
maybe only the bootflow_menu_theme test itself, so lets try and solve
this differently.

--
Tom


Yes I should have used CONFIG_EXPO not CONFIG_VIDEO .

Well no, at first glance it's 1 out of 15 tests in that file that depend
on CONFIG_EXPO, so we should just ifdef that test.

Yes, there is only one test in bootflow.o depending on EXPO.
There are multiple tests in bootdev.o depending on CONFIG_DM_MMC.

And the tests presumably check for DM_MMC, or should.

You could put something like this at the top of bootflow_cmd_menu():

/* 'bootflow menu' currently requires a video console */
if (!IS_ENABLED(CONFIG_VIDEO))
       return -EAGAIN;

With this change I still get:

       aarch64-linux-gnu-ld.bfd: boot/bootflow_menu.o: in function
`bootflow_menu_new':

Looks like there's an extra line in boot/Makefile then, we already
include bootflow_menu.o on EXPO.


Hello Simon,

the bootstd tests are in complete disarray

On the sandbox invoked with

./u-boot -T

=> ut bootstd
Can't map file 'mmc1.img': Invalid argument
mmc1: Unable to map file 'mmc1.img'
Failed to set up for bootstd tests (err=-5)

It I create a file mmc1.img:

test/boot/bootdev.c:460, bootdev_test_bootable(): -EINVAL ==
bootdev_find_in_blk(iter.dev, blk, &iter, &bflow): Expected 0xffffffea
(-22), got 0xffffffa3 (-93)
test/boot/bootdev.c:465, bootdev_test_bootable(): -ENOENT ==
bootdev_find_in_blk(iter.dev, blk, &iter, &bflow): Expected 0xfffffffe
(-2), got 0xffffffa3 (-93)
test/boot/bootdev.c:466, bootdev_test_bootable(): 1 ==
iter.first_bootable: Expected 0x1 (1), got 0x0 (0)
test/boot/bootdev.c:470, bootdev_test_bootable(): -EINVAL ==
bootdev_find_in_blk(iter.dev, blk, &iter, &bflow): Expected 0xffffffea
(-22), got 0xffffffa3 (-93)
Test: bootdev_test_bootable: bootdev.c (flat tree)
test/boot/bootdev.c:460, bootdev_test_bootable(): -EINVAL ==
bootdev_find_in_blk(iter.dev, blk, &iter, &bflow): Expected 0xffffffea
(-22), got 0xffffffa3 (-93)
test/boot/bootdev.c:465, bootdev_test_bootable(): -ENOENT ==
bootdev_find_in_blk(iter.dev, blk, &iter, &bflow): Expected 0xfffffffe
(-2), got 0xffffffa3 (-93)
test/boot/bootdev.c:466, bootdev_test_bootable(): 1 ==
iter.first_bootable: Expected 0x1 (1), got 0x0 (0)
test/boot/bootdev.c:470, bootdev_test_bootable(): -EINVAL ==
bootdev_find_in_blk(iter.dev, blk, &iter, &bflow): Expected 0xffffffea
(-22), got 0xffffffa3 (-93)
Test <NULL> failed 8 times

I normally run these tests with an alias:  pyt bootstd

# Run a pytest on sandbox
# $1: Name of test to run (optional, else run all)

function pyt {
     local tests
     local para

     if [ "$1" = "-b" ]; then
        crosfw $b || return 1
        shift
     fi

     if [ "$1" = "-p" ]; then
        para="-n$(nproc) -q"
        shift
     fi
     tests="$@"
     echo $para $tests

     shift
     test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${para}
${tests:+"-k $tests"} $@
}

Once I do that once (for setup), I can then run individual tests
directly: rt bootdev_test_bootable


function rt_get_suite_and_name() {
     local arg
     #echo arg $arg
     suite=
     name=

     if [ "$1" = "-f" ]; then
        force="-f"
        shift
     fi
     arg="$1"
     rest="$2"

     # The symbol is something like this:
     #   _u_boot_list_2_ut_bootstd_test_2_vbe_simple_test_base
     # Split it into the suite name (bootstd) and test name
     # (vbe_simple_test_base)
     read suite name < \
        <(nm /tmp/b/$exec/u-boot |grep "list_2_ut.*$arg.*" \
           | cut -d' ' -f3 \
           | head -1 \
           | sed -n 's/_u_boot_list_2_ut_\(.*\)_test_2_/\1 /p')
     #echo suite $suite
     #echo name $name
}

# Run a test
function rt() {
     local exec=sandbox
     local suite name force rest
     rt_get_suite_and_name $*

     /tmp/b/$exec/u-boot -T $rest -c "ut $suite $force $name"
}



As discussed previously:

The general expectation is that unit tests can be run successfully on
any physical or emulated board. Should a test not comply with this
requirement the test must be deactivated automatically upon build.

A test like
ut_assert_skip_to_line(
"sandbox: continuing, as we cannot run Linux");
will never succeed on other architectures.

Most of the tests are written for sandbox, since:

- they are testing logic which is not arch- or board-specific
- they can access state which is not available on other boards
- they can make use of host files
- they are 1000s of times faster to run

Why are you wanting to run these tests on 'real' boards? There has to
be a reason, beyond just an academic exercise.

Regards,
Simon

Different architectures have restrictions that the sandbox does not
have, e.g. concerning unaligned access, 32bit size. Therefore we should
be able to run most tests on any architecture.

I don't disagree in principle, but in my experience there are very few
bugs that are not found by sandbox. In fact, sandbox being able to
segfault finds quite a few bugs that are invisible on real hardware.

For the 32-bit size, we could have a 32-bit sandbox version if you
like. It does automatically build as 32-bit on 32-bit machines, but we
use 64-bit in our CI.

I completely agree there will be bugs that are found only on real
hardware, but often just running it once on hardware is enough to
flush those out and add more sandbox test coverage for them.


If a test can only be run on the sandbox, CONFIG_UNIT_TEST=y should not
build it.

At the moment UNIT_TEST is somewhat synonymous with SANDBOX. I enabled
UNIT_TEST on snow to at least avoid obvious build errors, but there is
no expectation that the unit tests all run on snow. To enforce this we
really need real boards in CI. Perhaps QEMU could be the way to go, to
resolve this?

Yes our target should be to enable CONFIG_UNIT_TEST on QEMU.



The ut command should succeed wherever it is run.

Yes that would be good. I think you will need quite a few checks added though.

For the bootstd test it obvious that they currently have to be 
restricted to the sandbox as they won't pass anywhere else.

Best regards

Heinrich


Regards,
Simon