[PATCH v2 14/38] test: cmd: Add a basic test for 'addrmap' command

[Bin Meng]

Hi Simon,

On Mon, Feb 22, 2021 at 5:20 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Sun, 21 Feb 2021 at 18:55, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Feb 22, 2021 at 12:24 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Sat, 20 Feb 2021 at 06:01, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sat, Feb 20, 2021 at 7:55 PM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > On Thu, 18 Feb 2021 at 08:59, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > >
> > > > > > This adds a basic test for the newly introduced 'addrmap' command.
> > > > > >
> > > > > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > > > > >
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - new patch: test: cmd: Add a basic test for 'addrmap' command
> > > > > >
> > > > > >  include/test/suites.h |  2 ++
> > > > > >  test/cmd/Makefile     |  1 +
> > > > > >  test/cmd/addrmap.c    | 38 ++++++++++++++++++++++++++++++++++++++
> > > > > >  test/cmd_ut.c         |  6 ++++++
> > > > > >  4 files changed, 47 insertions(+)
> > > > > >  create mode 100644 test/cmd/addrmap.c
> > > > >
> > > > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > > > >
> > > > > Just checking this test is enabled for sandbox?
> > > >
> > > > Not yet. I don't think sandbox has enabled CONFIG_ADDR_MAP.
> > >
> > > OK then can you please enable it so we have test coverage?
> >
> > I am not sure if Sandbox can support CONFIG_ADDR_MAP (non-identity
> > virtual-physical mapping)?
>
> Well it doesn't even really need to support it fully. Just adding the
> config and writing a test that sets a few entries and checks the
> functions in addrmap.h do the right thing should be enough. It should
> be 10 lines of code.
>

This sounds like another patch to test the library itself instead of
the command only. I think we can add the library testing in future
patches given Priyanka has reviewed almost all patches in this series.

> >
> > As Tom mentioned here [1], enabling unit tests on QEMU targets makes more sense?
> >
> > [1] https://lists.denx.de/pipermail/u-boot/2021-February/441779.html
>
> That was referring to a qemu-specific feature (called into qemu,
> actually). But in this case, if there is a failure, how will someone
> diagnose it? Run a huge functional test with qemu to see that it fails
> somewhere...? I think unit tests are far more useful for little
> features.

Regards,
Bin