[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. <mr.nuke.me at gmail.com> 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