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

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