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@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