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

[Tom Rini]

On Mon, Feb 22, 2021 at 02:20:35AM -0700, Simon Glass 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.
> 
> >
> > 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.

So, we have a few different forms of checking.  We have "make check",
"make qcheck" and make "tcheck", which are handy for running pytest on
sandbox and some of the test suites to our tools.  We also have CI
running (I believe) all of the same tests as "make check", and then also
running pytest on a number of emulated platforms.  We also have
infrequent static code analysis via Coverity scan, based on sandbox
builds.  So it's good that anything that can be built on sandbox is
built on sandbox.  But when it comes to pytests, the line of where to
run what is a lot fuzzier.  addrmap should have a test, and it should be
run on whatever qemu platforms it can be tested on, and it will be
tested frequently in CI.

In the end, looking at the test we have here now too, I suppose yes, if
it's only 10 lines of code so that addrmap compiles for sandbox (for
static testing) and then this test runs/passes, we should do that, it's
fine and good to, yes.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210222/28c1252d/attachment.sig>