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...@gmail.com> wrote: > > > > Hi Simon, > > > > On Mon, Feb 22, 2021 at 12:24 AM Simon Glass <s...@chromium.org> wrote: > > > > > > Hi Bin, > > > > > > On Sat, 20 Feb 2021 at 06:01, Bin Meng <bmeng...@gmail.com> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Sat, Feb 20, 2021 at 7:55 PM Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > On Thu, 18 Feb 2021 at 08:59, Bin Meng <bmeng...@gmail.com> wrote: > > > > > > > > > > > > This adds a basic test for the newly introduced 'addrmap' command. > > > > > > > > > > > > Signed-off-by: Bin Meng <bmeng...@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 <s...@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
signature.asc
Description: PGP signature
