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@xxxxxxxxx/ 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.
