Re: [PATCH v2 0/9] ci: make Git's GitHub workflow output much more helpful

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 9, 2022, 13:20 UTC
Message-ID: <nycvar.QRO.7.76.6.2203091404470.357@tvgsbejvaqbjf.bet>
In-Reply-To: <220307.86tuc9yc4b.gmgdl@evledraar.gmail.com>

Hi Ævar,

On Mon, 7 Mar 2022, Ævar Arnfjörð Bjarmason wrote:

Show 27 quoted lines

> On Mon, Mar 07 2022, Johannes Schindelin wrote:
>
> > On Wed, 2 Mar 2022, Ævar Arnfjörð Bjarmason wrote:
> >
> >>
> >> On Tue, Mar 01 2022, Johannes Schindelin via GitGitGadget wrote:
> >>
> >> > Changes since v1:
> >> >
> >> >  * In the patch that removed MAKE_TARGETS, a stale comment about that
> >> >    variable is also removed.
> >> >  * The comment about set -x has been adjusted because it no longer applies
> >> >    as-is.
> >> >  * The commit message of "ci: make it easier to find failed tests' logs in
> >> >    the GitHub workflow" has been adjusted to motivate the improvement
> >> >    better.
> >>
> >> Just briefly: Much of the feedback I had on v1 is still unanswered,
> >
> > Yes, because I got the sense that your feedback ignores the goal of making
> > it easier to diagnose test failures.
>
> I think that's a rather strange conclusion given that I've submitted a
> parallel series that makes some of those failures easier to diagnose
> than the same changes in this series. I.e. the failures in build
> v.s. test phases, not the individual test format output (but those are
> orthagonal).

I do not know what your parallel series implements, as I did not have the time to read it yet (and it contains about double the number of patches of my series, hopefully not intended to make it impossible for me to spare the time to even taking a glimpse at it).

Also: I thought we had the rule of trying to be mindful of other
contributors and avoid interfering with patch series that are in flight?
It could be viewed as unnecessarily adversarial.

Show 6 quoted lines

> I think it's a fair summary of our differences that we're just placing
> different values on UX responsiveness. I'm pretty sure there's some
> amount of UX slowdown you'd consider unacceptable, no matter how much
> the output was improved.
>
> Clearly we just use it differently.

I would gladly trade my convenience in return for making it easier for others to diagnose why their PR builds fail.

At the moment, the way our CI/PR builds present test failures very likely makes every new contributor feel stupid for not knowing how to proceed. But they are not stupid. The failure is not theirs. The fault lies squarely with us, for making it so frigging hard.

Show 11 quoted lines

> >> or in the case of the performance issues I think just saying that this
> >> output is aimed at non-long-time-contributors doesn't really justify the
> >> large observed slowdowns.
> >
> > What good is a quickly-loading web site when it is less than useful?
>
> For all the flaws in the current output there are cases now where you
> can click on a failure, see a summary of the 1-2 tests that failed, and
> even find your way through the right place in the rather verbose raw log
> output in 1/4 or 1/2 the time it takes the initial page on the new UX to
> loda.

I wonder where the hard data is that backs up these numbers.

I do not have hard data, either, except for one: apart from you and Junio, I have yet to talk to any contributor who said "oh yeah, I found the logs right away" rather than things like "when I finally figured out that the logs were in `print-test-failures`, I had a chance to make sense of the failures" or even "zOMG _that_ is where the logs are?". And let me tell you that I heard this from a lot of people. Way more than a mere two. Far, far more.

Show 13 quoted lines

> > I'd much rather have a slow-loading web site that gets me to where I need
> > to be than a fast-loading one that leaves me as puzzled as before.
>
> I think it's clear that we're going to disagree on this point, but I'd
> still think that:
>
>  * In a re-roll, you should amend these patches to clearly note that's a
>    UX trade-off you're making, perhaps with rough before/after timings
>    similar to the ones I've posted.
>
>    I.e. now those patches say nothing about the UX change resulting in
>    UX that's *much* slower than before. Clearly noting that trade-off
>    for reviewers is not the same as saying the trade-off can't be made.

Sure, I can do that.

>  * I don't see why the changes here can't be made configurable (and
>    perhaps you'd argue they should be on by default) via the ci-config
>    phase.

I do see why. If my goal is to unhide the logs by default, so that new contributors can find them more easily, I will not hide that new behavior behind a hard-to-find config option, an option that new contributors are even less likely to find. That would be highly counterproductive. If your goal is to help new contributors, I am certain that you will agree.

Ciao, Johannes