[PATCH v6 08/12] efi_loader: Disable ANSI output for tests

[Simon Glass]

Hi Heinrich,

On Mon, 30 Sept 2024 at 17:44, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 26.09.24 23:59, Simon Glass wrote:
> > We don't want ANSI characters written in tests since it is a pain to
> > check the output with ut_assert_nextline() et al.
> >
> > Provide a way to tests to request that ANSI characters not be sent.
> >
> > Add a proper function comment while we are here, to encourage others.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
>
> Please, consider prior review before resubmitting patches.

I'm just coming back to this and trying one more time to resolve this problem.

>
> As responded to all prior submissions:
>
> We want to test the code running on actual machines.
> We don't want to have sandbox code everywhere.

Who is 'we'? U-Boot features have to be designed for testing. It is
not a black box. We can and do have special code for sandbox in
U-Boot. We also have the ability to adjust how real boards run in
tests, e.g. collecting console output. The whole sandbox construct is
designed for testing and development.

>
> I cannot see any test that is not passing due to the current behavior.

This series includes a test which fails due to the unwanted ANSI
output. It is the last patch in this series.

>
> The size serial console is just requested once in the live-time of the
> U-Boot session.

When I use test.py with EFI it seems to happen quite a lot.

>
> You have bunches of options in your upcoming tests. Neither of these
> requires the suggested patch:
>
> * You can set stdout=vidconsole,serial and let the vidconsole provide a
> screen size.

Do you mean manually change the default console and rebuild U-Boot?
That is not really relevant to the problem here, since I often
generally run tests without a video console.

> * You can add the ANSI sequence to your ut_assert_nextline() statement.

Yes, that is feasible, but very strange and brittle. I'll give this a
try for the next version.

> * You can filter out ANSI codes in the test framework.

Do you mean in the assert_nextline() functions, or in Python?

> * You can run an EFI command like 'printenv -e' before the command that
> you actually want to test.

How do I inject that command when running test.py?

I appreciate you taking the time to list some options. But the
approach here is just not right, unfortunately.

Here is another example. When I run EFI tests (e.g. test.py with '-k
efi') I get something like this after the tests finish:

^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R^[[36;90R✘

I have to clear it from my console, before I can do anything else. If
I press 'enter' I get:

bash: syntax error near unexpected token `;'

I really don't want this output. The problem is not about filtering
it, it is about not generating it in the first place. You can see this
appearing in CI also.

No other subsystem generates ANSI characters when U-Boot starts. Why
are we creating this problem in the EFI implementation?

So please, I very-much want to have a way to stop this output from
being generated, not to filter it afterwards. As above, we should
design U-Boot for easy testing, not treat it as a black box. Please do
seriously consider this as I believe this is an important testing
principle being violated here.

Regards,
Simon


>
> Best regards
>
> Heinrich
>
> > ---
> >
> > (no changes since v1)
> >
> >   include/efi_loader.h         | 21 ++++++++++++++++++++-
> >   lib/efi_loader/efi_console.c | 26 +++++++++++++++++---------
> >   2 files changed, 37 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index f84852e384f..82b90ee0f1d 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -531,8 +531,27 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index);
> >   efi_status_t efi_bootmgr_run(void *fdt);
> >   /* search the boot option index in BootOrder */
> >   bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *index);
> > -/* Set up console modes */
> > +
> > +/**
> > + * efi_setup_console_size() - update the mode table.
> > + *
> > + * By default the only mode available is 80x25. If the console has at least 50
> > + * lines, enable mode 80x50. If we can query the console size and it is neither
> > + * 80x25 nor 80x50, set it as an additional mode.
> > + */
> >   void efi_setup_console_size(void);
> > +
> > +/**
> > + * efi_console_set_ansi() - Set whether ANSI characters should be emitted
> > + *
> > + * These characters mess up tests which use ut_assert_nextline(). Call this
> > + * function to tell efi_loader not to emit these characters when starting up the
> > + * terminal
> > + *
> > + * @allow_ansi: Allow emitting ANSI characters
> > + */
> > +void efi_console_set_ansi(bool allow_ansi);
> > +
> >   /* Set up load options from environment variable */
> >   efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var,
> >                                     u16 **load_options);
> > diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> > index cea50c748aa..569fc9199bc 100644
> > --- a/lib/efi_loader/efi_console.c
> > +++ b/lib/efi_loader/efi_console.c
> > @@ -30,6 +30,17 @@ struct cout_mode {
> >
> >   __maybe_unused static struct efi_object uart_obj;
> >
> > +/*
> > + * suppress emission of ANSI codes for use by unit tests. Leave it as 0 for the
> > + * default behaviour
> > + */
> > +static bool no_ansi;
> > +
> > +void efi_console_set_ansi(bool allow_ansi)
> > +{
> > +     no_ansi = !allow_ansi;
> > +}
> > +
> >   static struct cout_mode efi_cout_modes[] = {
> >       /* EFI Mode 0 is 80x25 and always present */
> >       {
> > @@ -348,13 +359,6 @@ static int __maybe_unused query_vidconsole(int *rows, int *cols)
> >       return 0;
> >   }
> >
> > -/**
> > - * efi_setup_console_size() - update the mode table.
> > - *
> > - * By default the only mode available is 80x25. If the console has at least 50
> > - * lines, enable mode 80x50. If we can query the console size and it is neither
> > - * 80x25 nor 80x50, set it as an additional mode.
> > - */
> >   void efi_setup_console_size(void)
> >   {
> >       int rows = 25, cols = 80;
> > @@ -362,8 +366,12 @@ void efi_setup_console_size(void)
> >
> >       if (IS_ENABLED(CONFIG_VIDEO))
> >               ret = query_vidconsole(&rows, &cols);
> > -     if (ret)
> > -             ret = query_console_serial(&rows, &cols);
> > +     if (ret) {
> > +             if (no_ansi)
> > +                     ret = 0;
> > +             else
> > +                     ret = query_console_serial(&rows, &cols);
> > +     }
> >       if (ret)
> >               return;
> >
>