Hi Simon, Thank you for the patch.
It's really great to have this documented. I found a couple of small typos, hope that helps. On lun., sept. 30, 2024 at 12:51, Simon Glass <[email protected]> wrote: > Provide a short description of how tests work, why they are so critical > and how to resolve gaps in Binman's test coverage. > > Signed-off-by: Simon Glass <[email protected]> > --- > > MAINTAINERS | 1 + > doc/develop/binman_tests.rst | 734 +++++++++++++++++++++++++++++++++++ > doc/develop/index.rst | 1 + > tools/binman/binman.rst | 5 + > 4 files changed, 741 insertions(+) > create mode 100644 doc/develop/binman_tests.rst > > diff --git a/MAINTAINERS b/MAINTAINERS > index 7ab39d91a55..65a9ea1face 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -909,6 +909,7 @@ BINMAN > M: Simon Glass <[email protected]> > M: Alper Nebi Yasak <[email protected]> > S: Maintained > +F: doc/develop/binman_tests.rst > F: tools/binman/ > > BLKMAP > diff --git a/doc/develop/binman_tests.rst b/doc/develop/binman_tests.rst > new file mode 100644 > index 00000000000..a632694a6fe > --- /dev/null > +++ b/doc/develop/binman_tests.rst > @@ -0,0 +1,734 @@ > +.. SPDX-License-Identifier: GPL-2.0+ > + > +.. toctree:: > + :maxdepth: 1 > + > +Binman Tests > +============ > + > +.. contents:: > + :depth: 2 > + :local: > + > +There is some material on writing tests in the main Binman documentation > +(see :doc:`package/index`). This short guide is separate so people don't > +feel they have to read as much. > + > +Code and output is mostly included verbatim, which makes the doc longer, but > +avoids its becoming confusing when the output or referenced code changes in > the > +future. > + > +Purpose > +------- > + > +The main purpose of tests in Binman is to make sure that Binman actually does > +what it is supposed to. Various people contribute code, refactoring is done > +over time, but U-Boot users (developers, SoC vendors, board vendors) rely on > +Binman producing images which function correctly. Without tests, a one-line > +change could unintentionally break a corner-case and the problem might not be > +noticed for months. Debugging an image-generation problem with a board you > +don't have can be very hard. > + > +A secondary purpose is productivity. U-Boot contributors are busy and often > +have too much on their plate. Trying to figure out why their patch broke > +some other vendor's workflow can be very time-consuming and frustrating. By > +building in tests from the start, this is largely avoided. If your change has > +full test coverage and doesn't break any test, all is well and no one can > +complain. > + > +A lessor purpose is to document what Binman actually does. If a test covers a Lessor? I think this should be lesser ? > +feature, it works. If there is no test coverage, no one can say for sure > +whether it works in all expected situations, certainly not wihout manual s/wihout/without > +effort. > + > +In fact, strictly speaking it isn't completely clear what 'works' even means > in > +the case where these is no test to cover the code. We are often left guessing s/these/there > +as to what the documentation means, what was actually intended, etc. > + > +Finally, code-coverage helps to remove 'zombie code', copied from elsewhere > +because it looks reasonable, but not actually needed. The same situation > arises > +in silicon-chip design, where a part of the chip is not validated. If it > isn't > +validated, it can be assumed not to work, either now or later, so it is best > to > +remove that logic to avoid it causing problems. > + > +Setting up > +---------- > + > +Binman tests use various utility programs. Most of these are documented in > +:doc:`../build/gcc`. But some are SoC-specific. To fetch these, tell Binman > to > +fetch or build any missing tools: > + > +.. code-block:: bash > + > + $ binman tool -f missing > + > +When this completes successfully, you can list the tools. You should see > +something like this: > + > +.. code-block:: bash > + > + $ binman tool -l > + Name Version Description Path > + --------------- ----------- ------------------------- > ------------------------------ > + bootgen ****** Bootg Xilinx Bootgen > /home/sglass/.binman-tools/bootgen > + bzip2 1.0.8 bzip2 compression /usr/bin/bzip2 > + cbfstool unknown Manipulate CBFS files > /home/sglass/bin/cbfstool > + fdt_add_pubkey unknown Generate image for U-Boot > /home/sglass/bin/fdt_add_pubkey > + fdtgrep unknown Grep devicetree files > /home/sglass/bin/fdtgrep > + fiptool v2.11.0(rele Manipulate ATF FIP files > /home/sglass/.binman-tools/fiptool > + futility v0.0.1-9f2e9 Chromium OS firmware utili > /home/sglass/.binman-tools/futility > + gzip 1.12 gzip compression /usr/bin/gzip > + ifwitool unknown Manipulate Intel IFWI file > /home/sglass/.binman-tools/ifwitool > + lz4 v1.9.4 lz4 compression /usr/bin/lz4 > + lzma_alone 9.22 beta lzma_alone compression > /usr/bin/lzma_alone > + lzop v1.04 lzo compression /usr/bin/lzop > + mkeficapsule 2024.10-rc5- mkeficapsule tool for gene > /home/sglass/bin/mkeficapsule > + mkimage 2024.10-rc5- Generate image for U-Boot > /home/sglass/bin/mkimage > + openssl 3.0.13 30 Ja openssl cryptography toolk /usr/bin/openssl > + xz 5.4.5 xz compression /usr/bin/xz > + zstd v1.5.5 zstd compression /usr/bin/zstd > + > +The tools are written to ``~/.binman-tools`` so add that to your ``PATH``. > +It's fine to have some of the tools elsewhere (e.g. ``~/bin``) so long as > they > +are up-to-date. This allows you use the version of the tools intended for > +running tests. > + > +Now you should be able to actually run the tests: > + > +.. code-block:: bash > + > + $ binman test > + ======================== Running binman tests ======================== > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ........ > + ---------------------------------------------------------------------- > + Ran 568 tests in 2.578s > + > + OK > + > +If this doesn't work, see if you can have some missing tools. Check that the > +dependencies are all there as above. If it is very slow, try installing > +concurrencytest so that the tests run in parallel. > + > +The next thing to set up is code coverage, using the -T flag: > + > +.. code-block:: bash > + > + $ binman test -T > + ======================== Running binman tests ======================== > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ...................................................................... > + ........ > + ---------------------------------------------------------------------- > + Ran 568 tests in 17.367s > + > + OK > + > + 99% > + Name Stmts Miss > Cover > + > --------------------------------------------------------------------------- > + tools/binman/__init__.py 0 0 > 100% > + tools/binman/bintool.py 263 0 > 100% > + tools/binman/btool/bootgen.py 21 0 > 100% > + tools/binman/btool/btool_gzip.py 5 0 > 100% > + tools/binman/btool/bzip2.py 5 0 > 100% > + tools/binman/btool/cbfstool.py 24 0 > 100% > + tools/binman/btool/cst.py 15 4 > 73% > + tools/binman/btool/fdt_add_pubkey.py 21 0 > 100% > + tools/binman/btool/fdtgrep.py 26 0 > 100% > + tools/binman/btool/fiptool.py 19 0 > 100% > + tools/binman/btool/futility.py 19 0 > 100% > + tools/binman/btool/ifwitool.py 22 0 > 100% > + tools/binman/btool/lz4.py 22 0 > 100% > + tools/binman/btool/lzma_alone.py 34 0 > 100% > + tools/binman/btool/lzop.py 5 0 > 100% > + tools/binman/btool/mkeficapsule.py 27 0 > 100% > + tools/binman/btool/mkimage.py 23 0 > 100% > + tools/binman/btool/openssl.py 42 0 > 100% > + tools/binman/btool/xz.py 5 0 > 100% > + tools/binman/btool/zstd.py 5 0 > 100% > + tools/binman/cbfs_util.py 376 0 > 100% > + tools/binman/cmdline.py 90 0 > 100% > + tools/binman/control.py 409 0 > 100% > + tools/binman/elf.py 241 0 > 100% > + tools/binman/entry.py 548 0 > 100% > + tools/binman/etype/alternates_fdt.py 58 0 > 100% > + tools/binman/etype/atf_bl31.py 5 0 > 100% > + tools/binman/etype/atf_fip.py 67 0 > 100% > + tools/binman/etype/blob.py 49 0 > 100% > + tools/binman/etype/blob_dtb.py 46 0 > 100% > + tools/binman/etype/blob_ext.py 9 0 > 100% > + tools/binman/etype/blob_ext_list.py 32 0 > 100% > + tools/binman/etype/blob_named_by_arg.py 9 0 > 100% > + tools/binman/etype/blob_phase.py 22 0 > 100% > + tools/binman/etype/cbfs.py 101 0 > 100% > + tools/binman/etype/collection.py 30 0 > 100% > + tools/binman/etype/cros_ec_rw.py 5 0 > 100% > + tools/binman/etype/efi_capsule.py 59 0 > 100% > + tools/binman/etype/efi_empty_capsule.py 33 0 > 100% > + tools/binman/etype/encrypted.py 34 0 > 100% > + tools/binman/etype/fdtmap.py 62 0 > 100% > + tools/binman/etype/files.py 35 0 > 100% > + tools/binman/etype/fill.py 13 0 > 100% > + tools/binman/etype/fit.py 311 0 > 100% > + tools/binman/etype/fmap.py 37 0 > 100% > + tools/binman/etype/gbb.py 37 0 > 100% > + tools/binman/etype/image_header.py 53 0 > 100% > + tools/binman/etype/intel_cmc.py 4 0 > 100% > + tools/binman/etype/intel_descriptor.py 39 0 > 100% > + tools/binman/etype/intel_fit.py 12 0 > 100% > + tools/binman/etype/intel_fit_ptr.py 17 0 > 100% > + tools/binman/etype/intel_fsp.py 4 0 > 100% > + tools/binman/etype/intel_fsp_m.py 4 0 > 100% > + tools/binman/etype/intel_fsp_s.py 4 0 > 100% > + tools/binman/etype/intel_fsp_t.py 4 0 > 100% > + tools/binman/etype/intel_ifwi.py 67 0 > 100% > + tools/binman/etype/intel_me.py 4 0 > 100% > + tools/binman/etype/intel_mrc.py 6 0 > 100% > + tools/binman/etype/intel_refcode.py 6 0 > 100% > + tools/binman/etype/intel_vbt.py 4 0 > 100% > + tools/binman/etype/intel_vga.py 4 0 > 100% > + tools/binman/etype/mkimage.py 84 0 > 100% > + tools/binman/etype/null.py 9 0 > 100% > + tools/binman/etype/nxp_imx8mcst.py 78 59 > 24% > + tools/binman/etype/nxp_imx8mimage.py 38 6 > 84% > + tools/binman/etype/opensbi.py 5 0 > 100% > + tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py 6 0 > 100% > + tools/binman/etype/pre_load.py 76 0 > 100% > + tools/binman/etype/rockchip_tpl.py 5 0 > 100% > + tools/binman/etype/scp.py 5 0 > 100% > + tools/binman/etype/section.py 418 0 > 100% > + tools/binman/etype/tee_os.py 31 0 > 100% > + tools/binman/etype/text.py 21 0 > 100% > + tools/binman/etype/ti_board_config.py 139 0 > 100% > + tools/binman/etype/ti_dm.py 5 0 > 100% > + tools/binman/etype/ti_secure.py 65 0 > 100% > + tools/binman/etype/ti_secure_rom.py 117 0 > 100% > + tools/binman/etype/u_boot.py 7 0 > 100% > + tools/binman/etype/u_boot_dtb.py 9 0 > 100% > + tools/binman/etype/u_boot_dtb_with_ucode.py 51 0 > 100% > + tools/binman/etype/u_boot_elf.py 19 0 > 100% > + tools/binman/etype/u_boot_env.py 27 0 > 100% > + tools/binman/etype/u_boot_expanded.py 4 0 > 100% > + tools/binman/etype/u_boot_img.py 7 0 > 100% > + tools/binman/etype/u_boot_nodtb.py 7 0 > 100% > + tools/binman/etype/u_boot_spl.py 8 0 > 100% > + tools/binman/etype/u_boot_spl_bss_pad.py 14 0 > 100% > + tools/binman/etype/u_boot_spl_dtb.py 9 0 > 100% > + tools/binman/etype/u_boot_spl_elf.py 8 0 > 100% > + tools/binman/etype/u_boot_spl_expanded.py 12 0 > 100% > + tools/binman/etype/u_boot_spl_nodtb.py 8 0 > 100% > + tools/binman/etype/u_boot_spl_pubkey_dtb.py 32 0 > 100% > + tools/binman/etype/u_boot_spl_with_ucode_ptr.py 8 0 > 100% > + tools/binman/etype/u_boot_tpl.py 8 0 > 100% > + tools/binman/etype/u_boot_tpl_bss_pad.py 14 0 > 100% > + tools/binman/etype/u_boot_tpl_dtb.py 9 0 > 100% > + tools/binman/etype/u_boot_tpl_dtb_with_ucode.py 8 0 > 100% > + tools/binman/etype/u_boot_tpl_elf.py 8 0 > 100% > + tools/binman/etype/u_boot_tpl_expanded.py 12 0 > 100% > + tools/binman/etype/u_boot_tpl_nodtb.py 8 0 > 100% > + tools/binman/etype/u_boot_tpl_with_ucode_ptr.py 12 0 > 100% > + tools/binman/etype/u_boot_ucode.py 33 0 > 100% > + tools/binman/etype/u_boot_vpl.py 8 0 > 100% > + tools/binman/etype/u_boot_vpl_bss_pad.py 14 0 > 100% > + tools/binman/etype/u_boot_vpl_dtb.py 9 0 > 100% > + tools/binman/etype/u_boot_vpl_elf.py 8 0 > 100% > + tools/binman/etype/u_boot_vpl_expanded.py 12 0 > 100% > + tools/binman/etype/u_boot_vpl_nodtb.py 8 0 > 100% > + tools/binman/etype/u_boot_with_ucode_ptr.py 42 0 > 100% > + tools/binman/etype/vblock.py 38 0 > 100% > + tools/binman/etype/x86_reset16.py 7 0 > 100% > + tools/binman/etype/x86_reset16_spl.py 7 0 > 100% > + tools/binman/etype/x86_reset16_tpl.py 7 0 > 100% > + tools/binman/etype/x86_start16.py 7 0 > 100% > + tools/binman/etype/x86_start16_spl.py 7 0 > 100% > + tools/binman/etype/x86_start16_tpl.py 7 0 > 100% > + tools/binman/etype/x509_cert.py 71 0 > 100% > + tools/binman/etype/xilinx_bootgen.py 72 0 > 100% > + tools/binman/fip_util.py 202 0 > 100% > + tools/binman/fmap_util.py 49 0 > 100% > + tools/binman/image.py 181 0 > 100% > + tools/binman/state.py 201 0 > 100% > + > --------------------------------------------------------------------------- > + TOTAL 5954 69 > 99% > + > + To get a report in 'htmlcov/index.html', type: python3-coverage html > + Coverage error: 99%, but should be 100% > + ValueError: Test coverage failure > + > +Unfortunately the run failed. As it suggests, create a report: > + > +.. code-block:: bash > + > + $ python3-coverage html > + Wrote HTML report to htmlcov/index.html > + > +If you open that file in the browser, you can see which files are not > reaching > +100% and click on them. Here is ``nxp_imx8mimage.py``, for example: > + > +.. code-block:: python > + > + 43 # Generate mkimage configuration file similar to imx8mimage.cfg > + 44 # and pass it to mkimage to generate SPL image for us here. > + 45 cfg_fname = tools.get_output_filename('nxp.imx8mimage.cfg.%s' > % uniq) > + 46 with open(cfg_fname, 'w') as outf: > + 47 print('ROM_VERSION v%d' % self.rom_version, file=outf) > + 48 print('BOOT_FROM %s' % self.boot_from, file=outf) > + 49 print('LOADER %s %#x' % (input_fname, > self.loader_address), file=outf) > + 50 > + 51 output_fname = tools.get_output_filename(f'cfg-out.{uniq}') > + 52 args = ['-d', input_fname, '-n', cfg_fname, '-T', 'imx8mimage', > + 53 output_fname] > + 54 if self.mkimage.run_cmd(*args) is not None: > + 55 return tools.read_file(output_fname) > + 56 else: > + 57 # Bintool is missing; just use the input data as the output > + 58 x self.record_missing_bintool(self.mkimage) > + 59 x return data > + 60 > + 61 def SetImagePos(self, image_pos): > + 62 # Customized SoC specific SetImagePos which skips the mkimage > etype > + 63 # implementation and removes the 0x48 offset introduced there. > That > + 64 # offset is only used for uImage/fitImage, which is not the > case in > + 65 # here. > + 66 upto = 0x00 > + 67 for entry in super().GetEntries().values(): > + 68 x entry.SetOffsetSize(upto, None) > + 69 > + 70 # Give up if any entries lack a size > + 71 x if entry.size is None: > + 72 x return > + 73 x upto += entry.size > + 74 > + 75 Entry_section.SetImagePos(self, image_pos) > + > +Most of the file is covered, but the lines marked with ``x`` indicate missing > +coverage. The will show up red in your browser. The what? The line ? With those addressed, feel free to add: Reviewed-by: Mattijs Korpershoek <[email protected]> > + > +What is a test? [...]
