Re: [PATCH 2/2] build: Let users know not all tests might run
On Mon, Feb 19, 2024 at 03:01:29PM +0100, Martin Kletzander wrote:
On Mon, Feb 19, 2024 at 04:47:52AM -0800, Andrea Bolognani wrote:
> On Mon, Feb 19, 2024 at 10:35:14AM +0100, Martin Kletzander wrote:
> > +if missing_optional_programs.length() > 0
> > + misc_summary += {'Some programs are missing, not all tests will be
executed':
> > + missing_optional_programs}
> > +endif
>
> I like it, but I'm going to suggest a slightly tweaked
> implementation.
>
> With the diff below squashed in, the output will turn into
>
> Optional programs
> Missing : black (some tests will be skipped!)
>
> which is less busy and more readable IMO. I think it's more likely to
> catch the user's eye compared to being yet another line at the bottom
> of the Miscellaneous section.
I liked that the first column got wider, but to be honest it catches
your eye only if you are used to the output.
And I like your version better. If you have it squashed locally, then
feel free to push that.
I think it's probably easier if you just squash it in yourself. You
can also add my
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
to both patches when you do that.
Maybe give Michal a chance to object to the change before pushing,
just in case it would make him retract his R-b.
> + summary(test_programs_summary, section: 'Optional
programs', bool_yn: true)
the bool_yn doesn't do anything here, but that's fine
Yeah, I copied it from the existing invocations but you're right that
it's unnecessary here. By all means drop it.
--
Andrea Bolognani / Red Hat / Virtualization