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

[Simon Glass]

Hi Heinrich,

On Wed, 31 May 2023 at 14:22, Heinrich Schuchardt
<heinrich.schuchardt at 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.schuchardt at 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 <trini at konsulko.com> wrote:
> >>>>>>
> >>>>>> On Wed, May 31, 2023 at 04:25:46PM +0200, Heinrich Schuchardt wrote:
> >>>>>>> Tom Rini <trini at 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.schuchardt at 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?

>
> 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.

Regards,
Simon