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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Victoria Dye <vdye@github.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Eric Sunshine <sunshine@sunshineco.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v2 0/9] ci: make Git's GitHub workflow output much more helpful
Date: Fri, 25 Mar 2022 10:02:07 +0100	[thread overview]
Message-ID: <220325.864k3mmm2x.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <6b83bb83-32b9-20c9-fa02-c1c3170351c3@github.com>

On Thu, Mar 24 2022, Victoria Dye wrote:

> Johannes Schindelin via GitGitGadget wrote:
> [...]
>> Is this the best UI we can have for test failures in CI runs? I hope we can
>> do better. Having said that, this patch series presents a pretty good start,
>> and offers a basis for future improvements.
>> 
>
> I think these are really valuable improvements over our current state, but I
> also understand the concerns about performance elsewhere in this thread
> (it's really slow to load for me as well, and scrolling/expanding the log
> groups can be a bit glitchy in my browser). That said, I think there are a
> couple ways you could improve the load time without sacrificing the (very
> helpful) improvements you've made to error log visibility. I experimented a
> bit (example result [1]) and came up with some things that may help:
>
> * group errors by test file, rather than by test case (to reduce
>   parsing/rendering time for lots of groups).
> * print the verbose logs only for the failed test cases (to massively cut
>   down on the size of the log, particularly when there's only a couple
>   failures in a test file with a lot of passing tests).
> * skip printing the full text of the test in 'finalize_test_case_output'
>   when creating the group, i.e., use '$1' instead of '$*' (in both passing
>   and failing tests, this information is already printed via some other
>   means).
>
> If you wanted to make sure a user could still access the full failure logs
> (i.e., including the "ok" test results), you could print a link to the
> artifacts page as well - that way, all of the information we currently
> provide to users can still be found somewhere.
>
> [1] https://github.com/vdye/git/runs/5666973267

Thanks a lot for trying to address those concerns.

I took a look at this and it definitely performs better, although in
this case the overall output is ~3k lines.

I'd be curious to see how it performs on some of the cases discussed in
earlier threads of >~50k lines, although it looks like in this case that
would require failures to be really widespread in the test suite.

I just looked at this briefly, but looking at the branch I see you
removed the "checking known breakage of[...]" etc. from the non-GitHub
markdown output, I didn't spot how that was related/needed.

>> Johannes Schindelin (9):
>>   ci: fix code style
>>   ci/run-build-and-tests: take a more high-level view
>>   ci: make it easier to find failed tests' logs in the GitHub workflow
>>   ci/run-build-and-tests: add some structure to the GitHub workflow
>>     output
>>   tests: refactor --write-junit-xml code
>>   test(junit): avoid line feeds in XML attributes
>>   ci: optionally mark up output in the GitHub workflow
>>   ci: use `--github-workflow-markup` in the GitHub workflow
>>   ci: call `finalize_test_case_output` a little later
>> 
>
> The organization of these commits makes the series a bit confusing to read,
> mainly due to the JUnit changes in the middle. Patches 5-6 don't appear to
> be dependent on patches 1-4, so they could be moved to the beginning (after
> patch 1). With that change, I think this series would flow more smoothly:
> "Cleanup/non-functional change" -> "JUnit XML improvements" -> "Log UX
> improvements".

Have you had a change to look at the approach my suggestion of an
alternate approach to the early part of this series takes?:
https://lore.kernel.org/git/cover-00.25-00000000000-20220221T143936Z-avarab@gmail.com/

I.e. to not build up ci/lib.sh to know to group the "build" etc. within
the "run-build-and-test" step, but instead just to pull those to the
top-level by running separate build & test steps.