Re: [PATCH 3/3] binman: Add a tutorial on resolving test-coverage bugs

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 sjg@chromium.org 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 sjg@chromium.org

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 sjg@chromium.org M: Alper Nebi Yasak alpernebiyasak@gmail.com 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 mkorpershoek@baylibre.com

+What is a test?

[...]