[CC-ing some people who've been interested in CI architechture] On Mon, Jan 24 2022, Johannes Schindelin via GitGitGadget wrote: > [...] > The current situation > ===================== > > Let me walk you through the current experience when a PR build fails: I get > a notification mail that only says that a certain job failed. There's no > indication of which test failed (or was it the build?). I can click on a > link at it takes me to the workflow run. Once there, all it says is "Process > completed with exit code 1", or even "code 2". Sure, I can click on one of > the failed jobs. It even expands the failed step's log (collapsing the other > steps). And what do I see there? > > Let's look at an example of a failed linux-clang (ubuntu-latest) job > [https://github.com/git-for-windows/git/runs/4822802185?check_suite_focus=true]: > > [...] > Test Summary Report > ------------------- > t1092-sparse-checkout-compatibility.sh (Wstat: 256 Tests: 53 Failed: 1) > Failed test: 49 > Non-zero exit status: 1 > t3701-add-interactive.sh (Wstat: 0 Tests: 71 Failed: 0) > TODO passed: 45, 47 > Files=957, Tests=25489, 645 wallclock secs ( 5.74 usr 1.56 sys + 866.28 cusr 364.34 csys = 1237.92 CPU) > Result: FAIL > make[1]: *** [Makefile:53: prove] Error 1 > make[1]: Leaving directory '/home/runner/work/git/git/t' > make: *** [Makefile:3018: test] Error 2 > Firstly I very much applaud any effort to move the CI UX forward. I know we haven't seen eye-to-eye on some of the trade-offs there, but I think something like this series is a step in the right direction. I.e. trying harder to summarize the output for the user, and making use of some CI platform-specific features. I sent a reply in this thread purely on some implementation concerns related to that in https://lore.kernel.org/git/220126.86sftbfjl4.gmgdl@xxxxxxxxxxxxxxxxxxx/, but let's leave that aside for now... > [...] > So I had a look at what standards exist e.g. when testing PowerShell > modules, in the way of marking up their test output in GitHub workflows, and > I was not disappointed: GitHub workflows support "grouping" of output lines, > i.e. marking sections of the output as a group that is then collapsed by > default and can be expanded. And it is this feature I decided to use in this > patch series, along with GitHub workflows' commands to display errors or > notices that are also shown on the summary page of the workflow run. Now, in > addition to "Process completed with exit code" on the summary page, we also > read something like: > > ⊗ linux-gcc (ubuntu-latest) > failed: t9800.20 submit from detached head > > Even better, this message is a link, and following that, the reader is > presented with something like this > [https://github.com/dscho/git/runs/4840190622?check_suite_focus=true]: This series is doing several different things, at least: 1) "Grouping" the ci/ output, i.e. "make" from "make test" 2) Doing likewise for t/test-lib.sh 3) In doing that for t/test-lib.sh, also "signalling" the GitHub CI, to e.g. get the "submit from detached head" output you quote just a few lines above I'd like to focus on just #1 here. Where I was going with that in my last CI series was to make a start at eventually being able to run simply "make" at the top-level "step". I.e. to have a recipe that looks like: - run: make - run: make test I feel strongly that that's where we should be heading, and the #1 part of this series is basically trying to emulate what you'd get for free if we simply did that. I.e. if you run single commands at the "step" level (in GitHub CI nomenclature) you'll get what you're doing with groupings in this series for free, and without any special code in ci/*, better yet if you then do want grouping *within* that step you're free to do so without having clobbered your one-level of grouping already on distinguishing "make" from "make test". IOW our CI now looks like this (pseudocode): - job: - step1: - use ci/lib.sh to set env vars - run a script like ci/run-build-and-tests.sh - step2: - use ci/lib.sh to set env vars - run a script like print-test-failures.sh But should instead look like: - job: - step1: - set variables in $GITHUB_ENV using ci/lib.sh - step2: - make - step3: - make test - step4: - run a script like print-test-failures.sh Well, we can quibble about "step4", but again, let's focus on #1 here, that's more #2-#3 territory. I had some WIP code to do that which I polished up, here's how e.g. a build failure looks like in your implementation (again, just focusing on how "make" and "make test" is divided out, not the rest): https://github.com/dscho/git/runs/4840190622?check_suite_focus=true#step:4:62 I.e. you've made "build" an expandable group at the same level as a single failed test, and still all under the opaque ci/run-build-and-test.sh script. And here's mine. This is using a semi-recent version of my patches that happened to have a failure, not quite what I've got now, but close enough for this E-Mail: https://github.com/avar/git/runs/4956260395?check_suite_focus=true#step:7:1 Now, notice two things, one we've made "make" and "make test" top-level steps, but more importantly if you expand that "make test" step on yours you'll get the full "make test" output, And two it's got something you don't have at all, which is that we're now making use of the GitHub CI feature of having pre-declared an environment for "make test", which the CI knows about (you need to click to expand it): https://github.com/avar/git/runs/4956260395?check_suite_focus=true#step:7:4 Right now that's something we hardly make use of at all, but with my changes the environment is the *only* special sauce we specify before the step, i.e. GIT_PROVE_OPTS=.. DEFAULT_TEST_TARGET=... etc. I think I've run out of my ML quota for now, but here's the branch that implements it: https://github.com/git/git/compare/master...avar:avar/ci-unroll-make-commands-to-ci-recipe That's "282 additions and 493 deletions.", much of what was required to do this was to eject the remaining support for the dead Travis and Azure CI's that we don't run, i.e. to entirely remove any sort of state management or job control from ci/lib.sh, and have it *only* be tasked with setting variables for subsequent steps to use. That makes it much simpler, my end-state of it is ~170 lines v.s. your ~270 (but to be fair some of that's deleted Travis code): https://github.com/avar/git/blob/avar/ci-unroll-make-commands-to-ci-recipe/ci/lib.sh https://github.com/gitgitgadget/git/blob/pr-1117/dscho/use-grouping-in-ci-v1/ci/lib.sh And much of the rest is just gone, e.g. ci/run-build-and-tests.sh isn't there anymore, instead you simply run "make" or "make test" (or the equivalent on Windows, which also works): https://github.com/avar/git/tree/avar/ci-unroll-make-commands-to-ci-recipe/ci https://github.com/gitgitgadget/git/tree/pr-1117/dscho/use-grouping-in-ci-v1/ci Anyway, I hope we can find some sort of joint way forward with this, because I think your #1 at least is going in the opposite direction we should be going to achieve much the same ends you'd like to achieve. We can really just do this in a much simpler way once we stop treating ci/lib.sh and friends as monolithic ball of mud entry points. But I'd really like us not to go in this direction of using markup to "sub-divide" the "steps" within a given job, when we can relatively easily just ... divide the steps. As shown above that UI plays much more naturally into the CI's native features & how it likes to arrange & present things. And again, all of this is *only* discussing the "step #1" noted above. Using "grouping" for presenting the test failures themselves or sending summaries to the CI "Summary" is a different matter. Thanks!
