Re: [coreboot] <vendor> commit messages and their suitability for coreboot.org

[Carl-Daniel Hailfinger]

Hi,

given that testing is something I value a lot, I'd like to chip in.

On 23.12.2015 17:06, Aaron Durbin via coreboot wrote:
> On Tue, Dec 22, 2015 at 7:12 PM, Alex G.  wrote:
> > [...]For the
> > sake of example, I will pick on a RABDOM commit message which follows
> > the format:
> >
> > BUG=chrome-os-partner:48412
> > BUG=chromium:445938
> > BRANCH=None
> > TEST=TBD
> > [...]
> > ### TEST tags ###
> >
> > The general attitude towards a patch is "if people have to ask how you
> > tested it, then you probably need to include it in the commit message".
> > There's no hard rule towards the format of the commit message body, so
> > TEST tags are perfectly fine.
> >
> > What is not fine is TEST tags that have no useful information. In this
> > case, a feature is introduced, and some later patch, the feature is
> > used. Where should the feature get tested? The patch that adds the
> > feature? The patch that uses the feature? There are cases where patches
> > have to be tested in bulk, and that's fine. Please describe the testing
> > methodology only on the last patch of the series.
>
> Indeed good feedback on the code review, but it's not there. Actually,
> it looks like I didn't backfill what I did. Thanks for the pointer.
> > A very common TEST tag says "built and booted <board>". That's lazy.
> > First, every coreboot commit is build-tested build jenkins for each
> > board. Restating that the patch builds for a specific board is redundant.
>
> But it's not booted so in that sense it's actually a positive signal.
> Jenkins building also doesn't cover all the combinations of options
> that something could be impacted by.

One of the problems of jenkins is that it didn't detect that qemu normal
image (instead of simple fallback-only) didn't compile for half a year.
With the expected combinatorial explosion, this is expected, but it
still doesn't make me happy.
Maybe it would make sense to say "build-tested in a non-jenkins config"
if that was tested.
> > What does "booted" mean? Does it mean boot to ramstage? Does it mean
> > boot to payload? Which payload? Does it mean boot to kernel? Which
> > kernel? Does kernel crash or reach shell? Does it bluescreen? Does it
> > reach a shell? Graphical shell? Console shell? Is the console over
> > serial? Is it over network? Does USB work? Can you access SATA drives?
> > From what media was your kernel loaded?
> >
> > You get the point. "booted" provides no meaningful information. If your
> > testing involved "building and booting" a specific board, then please
> > leave out the TEST tag from your commit message, or remove it before
> > pushing the commit to coreboot.org.
>
> If you juggle that many definitions of 'booted' in your head I can see
> why the pedant comes out. It is my understanding that booted means
> booting through the OS to userspace. Is there really other
> definitions? And if there are wouldn't those be less progressing?
>
> Again, feel free to ignore it as well. While it may not be beneficial
> to you it may be beneficial to others.

Would flags help? E.g. payload+linux+net+usb?

Would it make sense to include a link to the boot log (that is, coreboot
console and dmesg)? Photo of the screen (to confirm gfx)?
> From my perspective, it would be great to use REACTS output in a way

that helps people check/confirm the awesomeness of a patch.

Regards,
Carl-Daniel