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. <[email protected]> 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 -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
