[PATCH v3 06/11] binman: capsule: Add support for generating capsules

[Simon Glass]

Hi Sughosh,

On Tue, 11 Jul 2023 at 01:13, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 11 Jul 2023 at 03:08, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sun, 9 Jul 2023 at 07:34, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > Add support in binman for generating capsules. The capsule parameters
> > > can be specified either through a config file or through the capsule
> > > binman entry.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > ---
> > > Changes since V2:
> > > * New patch which generates capsules through binman replacing the
> > >   earlier make target.
> > >
> > >  tools/binman/btool/mkeficapsule.py |  91 +++++++++++++++++++++++++
> > >  tools/binman/entries.rst           |  27 ++++++++
> > >  tools/binman/etype/capsule.py      | 102 +++++++++++++++++++++++++++++
> > >  3 files changed, 220 insertions(+)
> > >  create mode 100644 tools/binman/btool/mkeficapsule.py
> > >  create mode 100644 tools/binman/etype/capsule.py
> >
> > Please do check test coverage (binman test -T). You are missing quite
> > a lot in two two files you have added.
>
> I was aware of adding tests in binman, but since the capsules
> generated through binman are getting tested in the capsule update
> functionality, I thought this would be superfluous. If this is
> mandatory, I will add the tests. Will also address the rest of your
> comments for this patch.

The binman tests are for binman, which is a self-contained program
with 100% test coverage. That is a very important feature, since lots
of random SoC-specific features depend on it. If something breaks, it
can be a bit of a disaster.

Also, 100% test coverage allows refactoring of binman's code without
worry about breaking something. This is also very important, since we
will find problems which the current code base cannot solve. People
need the confidence to refactor

In practice, the tests define the behaviour. Code which is not tested
is therefore considered 'dead' code and should be deleted.

A notable point with binman tests is that all failure modes are
tested, not just 'happy path'.

[..]

Regards,
Simon