Re: [PATCH] test: report summary even when aborting

[Daniel Kahn Gillmor <dkg@fifthhorseman.net>]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

Thanks for the feedback, Tomi!

On Sat 2019-05-25 22:41:58 +0300, Tomi Ollila wrote:
> On Sat, May 25 2019, Daniel Kahn Gillmor wrote:
>
>> In certain cases of test suite failure, the summary report was not
>> being printed.  In particular, any failure on the parallel test suite,
>> and any aborted test in the serialized test suite would end up hiding
>> the summary.
>>
>> It's better to always show the summary where we can (while preserving
>> the return code).
>
> This is an improvement, but it looks like it is possible
> aggregate-results.sh prints everything good (like all 3 tests passed)
> since it may be that there are only good logs in test-results/.
>
> That might, in some cases, be confusing (exit value of notmuch-test
> is not always visible). 

Yes, i share your concern about confusion, and i tried to stave off that
confusion by making sure that there was something printed in each of the
fallthrough cases.  But you're right that i didn't do that well
enough. v2 of my patch also moves the printed warning to after
aggregate-results.sh output, and prefixes it with "ERROR:", so that it's
more obvious that this is why we are terminating non-zero.

> One option could be giving $RES to aggregate-results.sh and if it is
> not zero then that could print different text -- just that it requires
> a bit more coding...

I'm not convinced that's a worthwhile tradeoff. i don't want to add an
option parser to aggregate-results.sh, and it uses all of "$@" already.
So in the simplest implementation we'd be passing some sort of
environment variable to it -- but that variable wouldn't do anything
other than print what notmuch-test is already printing.  certainly if we
just passed $RES we'd have even less information about the context of
where/how the meta-failure happened.  My v2 patch does a better job of
this, hopefully.

So if someone has strong feelings about this and wants to update
aggregate-results.sh instead, feel free to do so; i wouldn't block it,
but hopefully the current proposal is acceptable.

thanks for the review,

       --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]