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

Wed May 31 22:22:15 CEST 2023

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.

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

The ut command should succeed wherever it is run.

Best regards

Heinrich