[PATCH 19/21] py/tests: test_efi_selftest: Add test for counting EFI netdevices

[Simon Glass]

Hi Tom,

On Fri, 24 Jan 2025 at 12:17, Tom Rini <trini at konsulko.com> wrote:
>
> On Fri, Jan 24, 2025 at 11:57:28AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 23 Jan 2025 at 07:42, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Thu, Jan 23, 2025 at 07:37:26AM -0700, Simon Glass wrote:
> > > > Hi Adriano,
> > > >
> > > > On Wed, 22 Jan 2025 at 10:10, Adriano Cordova <adrianox at gmail.com> wrote:
> > > > >
> > > > > check that the number of ethernet udevices matches the number of simple
> > > > > network protocols
> > > > >
> > > > > Signed-off-by: Adriano Cordova <adriano.cordova at canonical.com>
> > > > > ---
> > > > >  test/py/tests/test_efi_selftest.py | 21 +++++++++++++++++++++
> > > > >  1 file changed, 21 insertions(+)
> > > > >
> > > > > diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py
> > > > > index 310d8ed294..af9992a6f7 100644
> > > > > --- a/test/py/tests/test_efi_selftest.py
> > > > > +++ b/test/py/tests/test_efi_selftest.py
> > > > > @@ -195,3 +195,24 @@ def test_efi_selftest_tcg2(u_boot_console):
> > > > >      if u_boot_console.p.expect(['Summary: 0 failures', 'Press any key']):
> > > > >          raise Exception('Failures occurred during the EFI selftest')
> > > > >      u_boot_console.restart_uboot()
> > > > > +
> > > > > + at pytest.mark.buildconfigspec('cmd_bootefi_selftest')
> > > > > + at pytest.mark.buildconfigspec('cmd_dm')
> > > > > +def test_efi_selftest_count_netdevices(u_boot_console):
> > > > > +    """Test the EFI net device driver
> > > > > +
> > > > > +    u_boot_console -- U-Boot console
> > > > > +
> > > > > +    This function counts the number of ETH_UCLASS probed udevices
> > > > > +    calls the EFI net device selftest to check that the EFI driver
> > > > > +    sees the same.
> > > > > +    """
> > > > > +    u_boot_console.restart_uboot()
> > > >
> > > > We should avoid restarting U-Boot in a test unless there is a strong
> > > > need. It makes things a lot slower!
> > >
> > > Yes, if we can not reboot here that would be good.
> > >
> > > > > +    response = u_boot_console.run_command('dm tree')
> > > > > +    lines = response.splitlines()[2:]
> > > > > +    ethernet_count = sum(1 for line in lines if line.strip().startswith('ethernet') and '[ + ]' in line)
> > > >
> > > > This test should be written in C, e.g:
> > > >
> > > > struct udevice *dev;
> > > > int count;
> > > > uclass_foreach_dev(dev, UCLASS_ETH)
> > > >     if (device_active(dev))
> > > >          count++;
> > > >
> > > > > +
> > > > > +    u_boot_console.run_command(cmd='setenv efi_selftest netdevices')
> > > > > +    u_boot_console.run_command('bootefi selftest', wait_for_prompt=False)
> > > > > +    if u_boot_console.p.expect([f'Detected {ethernet_count} active EFI net devices']):
> > > > > +        raise Exception('Failures occurred during the EFI selftest')
> > > >
> > > > Again, this should be written in C and use run_command().
> > >
> > > No, if these are written in python already, we can leave them in python.
> >
> > This is a new test. We should not be making things any worse.
> >
> > > C is bad for strings.
> >
> > ut_assert_skip_to_line("Detected %d active EFI net devices", count);
> >
> > For C tests we don't even need 'dm tree'.
> >
> > This matters. The C test is easier to understand and runs in a tiny
> > fraction of the time. Our CI tests have been getting slower and slower
> > and it is due to this kind of test. I went to the effort of writing
> > this up carefully [1].
> >
> > So, NAK from me. Please use C tests where possible.
> >
> > Regards,
> > Simon
> >
> > [1] https://docs.u-boot.org/en/latest/develop/tests_writing.html
>
> I suppose re-doing these in C would be a good test to see if writing
> tests in C rather than Python is easier or not.

In this case it should be easy enough. Other tests are much easier to
write in Python. Some are easier to write in C, if they need to check
internal information.

> But given that our C
> tests are also randomly failing (the hash md5 thing) that's not the
> strongest endorsement of them either.

Indeed. Hopefully this is just a problem with the test system in
U-Boot, rather than a bug in the 'real' part of U-Boot.

Is it possible to bisect this problem?

Regards,
Simon