Hi Tom,
On Sat, 15 Feb 2025 at 07:52, Tom Rini <tr...@konsulko.com> wrote:
>
> On Sat, Feb 15, 2025 at 06:38:27AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 24 Jan 2025 at 13:34, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Fri, Jan 24, 2025 at 12:43:29PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Fri, 24 Jan 2025 at 12:17, Tom Rini <tr...@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 <tr...@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
> > > > > > > > <adria...@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > check that the number of ethernet udevices matches the number
> > > > > > > > > of simple
> > > > > > > > > network protocols
> > > > > > > > >
> > > > > > > > > Signed-off-by: Adriano Cordova <adriano.cord...@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()
> > > > > > > > > +
> > > > > > > > > +@pytest.mark.buildconfigspec('cmd_bootefi_selftest')
> > > > > > > > > +@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?
> > >
> > > No, bisect'ing finds that the commit which added the test is the failing
> > > commit.
> >
> > I suppose I missed this problem or have forgotten about it. Is it
> > still happening? Can you point me to the failure?
>
> Yes, Marek fixed it. See:
> commit 082b41df8abd8b2d0f2dccce8ed74fe7f5c6509e
> Author: Marek Vasut <marek.vasut+rene...@mailbox.org>
> Date: Mon Jan 27 00:57:03 2025 +0100
>
> test/cmd/wget.c: Fix loadaddr rewrite
>
> The $loadaddr variable is a hexadecimal value, not a string, it must be
> assigned using env_set_hex(). This may break follow up tests, like the
> dm_test_cmd_hash_md5 in CI. To avoid any interference with other tests,
> set $wgetaddr variable which is specific to this test and use it in the
> test.
>
> Fixes: 20f641987f83 ("test/cmd/wget.c: move net_test_wget() to the cmd
> test suite")
> Signed-off-by: Marek Vasut <marek.vasut+rene...@mailbox.org>
> Acked-by: Jerome Forissier <jerome.foriss...@linaro.org>
> Reviewed-by: Fabio Estevam <feste...@gmail.com>
>
OK, good.
Regards,
Simon
