[PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Masami,

On Mon, 28 Feb 2022 at 07:40, Masami Hiramatsu
<masami.hiramatsu at linaro.org> wrote:
>
> Hi Simon,
>
> 2022年2月28日(月) 6:45 Simon Glass <sjg at chromium.org>:
>
> >
> > Hi Tom,
> >
> > On Sun, 27 Feb 2022 at 13:58, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > > > >
> > > > > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > > > > Hi Masami,
> > > > > > > >
> > > > > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > > > > <masami.hiramatsu at linaro.org> wrote:
> > > > > > > >>
> > > > > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > > > > >> so that it can handle a reset while the 1st boot process after main
> > > > > > > >> boot logo before prompt correctly.
> > > > > > > >>
> > > > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu at linaro.org>
> > > > > > > >> ---
> > > > > > > >>   Changes in v5:
> > > > > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > > > > >>      the reset will happen between main boot and the command prompt.
> > > > > > > >> ---
> > > > > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > > > > >>
> > > > > > > >
> > > > > > > > Didn't I already comment on this patch? Why did it come back?
> > > > > > >
> > > > > > > Dear Simon,
> > > > > > >
> > > > > > > The discussion is in
> > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > > > > >
> > > > > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > > > > the reset driver."
> > > > > > >
> > > > > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > > > > the referenced thread.
> > > > > >
> > > > > > Why?
> > > > > >
> > > > > > The fake reset is there for use by tests. We don't need this load of
> > > > > > Python code at all for sandbox. We should worry about it later.
> > > > >
> > > > > Well, isn't this going to make the tests more sandbox-centric then, and
> > > > > then need changes later to be able to test on real hardware?
> > > >
> > > > Yes, but it keeps the sandbox case simple. At present the sandbox
> > > > tests can run within U-Boot (see the 'ut' command) and I very much
> > > > want to keep it that way. That is, after all, why I wrote the reset
> > > > driver.
> > > >
> > > > While tests on real hardware have value, I hope that all logic bugs
> > > > are found on sandbox.
> > >
> > > Yes, it's important to test the code in sandbox before testing it on
> > > hardware, to avoid "obvious" oops-it-broke changes, it's still very
> > > important to be able to easily run this on real hardware.  Ideally, I
> > > hope to see updates to the pytest hook repository to flash the hardware
> > > via capsule, as well as running a more formal pytest on hardware.  To
> >
> > Can you be specific about what bugs you are trying to catch in that
> > case? I am conscious of the nightmare that is Zephyr's thousands of
> > QEMU-based tests that take 20 minutes to run in parallel on a 64-core
> > machine, so I'd would like to make sure that real bugs are found in
> > unit tests where possible.
>
> I think this UEFI capsule update testcase is to check the capsule
> file generated by the tool can be handled as we expected. E.g.
> setting up the EFI variables and the capsule file is found in the
> specific directory in the ESP etc.

OK

> (Note that this testcase doesn't check the capsule actually update the
> firmware itself... which can not be done by sandbox because updated
> firmware needs to be reloaded from the virtual storage.)

That can be done, if needed. See os_jump_to_file(). The Chromium OS
test does this.

>
> >
> > > that end, is it not most important to make sandbox look like a real
> > > hardware platform, rather than adapt the test to know about special
> > > sandbox things?  Or am I missing something here and the test shouldn't
> >
> > The key thing is that sandbox runs essentially the same 'code under
> > test' as the real board and that we can quickly verified (using the
> > 'ut xxx' command) that it works. In this case, we want to run the EFI
> > code under sandbox and make sure that it works.
> >
> > > need changes / special handling to support both sandbox and real
> > > hardware, with what you're suggesting?
> >
> > The title of this patch refers to specific hacks in pytest to handle
> > sandbox, doesn't it? So I think this is around the wrong way...that is
> > in fact my objection. It simply should not be done that way, with
> > special code in pytest, or whatever.
>
> I agree with this point, yes, this changes the pytest ConsoleBase class
> for sandbox test. If this cause a problem if this is used for normal platform,
> it need to be changed.

I don't think you need this patch for sandbox, which is my point. I
would rather simply not have it.

>
> > It should be possible to run 'ut xxx' and have the test run from start
> > to finish, without any outside influence. Sandbox has a sysreset
> > driver, just like any other board. We can make it do whatever we
> > want...see sandbox_sysreset_request().
>
> I'm not sure this part. What would you mean the 'outside influence'?

If you see this section it tells you how to run sandbox tests
directly, without pytest:

https://u-boot.readthedocs.io/en/latest/develop/tests_sandbox.html#running-sandbox-tests-directly

It should be possible to run your tests this way on sandbox, without
needing the pytest code. Of course there are exceptions, but most
tests work this way and it seems to me that this one can also.

'Outside influence' means Python code getting involved.
>
> Thank you,
>
> >
> > We do use pytest to set things up beforehand, or to verify that things
> > worked after the run, but we should not need it to even just run a
> > unit test. In particular, it should not be necessary for sandbox to be
> > restarted by an outside influence.

Regards,
Simon