{"thread":{"id":"57970","subject":"[PATCH] ci(github): bring back the 'print test failures' step","startedAt":"2022-06-08T10:43:39Z","lastAt":"2022-07-25T20:02:50Z","messageCount":6,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"456861","messageId":"pull.1252.git.1654684998599.gitgitgadget@gmail.com","threadId":"57970","inReplyTo":null,"subject":"[PATCH] ci(github): bring back the 'print test failures' step","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-08T10:43:18Z","receivedAt":"2022-06-08T10:43:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nGit now shows better information in the GitHub workflow runs when a test\ncase failed. However, when a test case was implemented incorrectly and\ntherefore does not even run, nothing is shown.\n\nLet's bring back the step that prints the full logs of the failed tests,\nand to improve the user experience, print out an informational message\nfor readers so that they do not have to know/remember where to see the\nfull logs.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    ci(GitHub workflow): bring back the 'print test failures' step\n    \n    When an incorrectly-implemented test let a CI build fail\n    [https://github.com/git/git/runs/6703333447?check_suite_focus=true#step:4:1750]\n    without any failing test cases, the output was not helpful for readers\n    who wished to investigate the problem:\n    \n    [...]\n     751 ⏵Run tests\n    1746 === Failed test: t3105-ls-tree-output ===\n    1747 The full logs are in the artifacts attached to this run.\n    1748 Error: Process completed with exit code 1.\n    \n    \n    While this is still an improvement from before (where no output was\n    shown in the test step at all, apart from the very high-level prove\n    output), we can do better than point users to downloading the artifacts\n    (which is a bit cumbersome).\n    \n    With this patch, the print test failures step is reintroduced. To make\n    sure that readers know to look at it, we now print an explicit message\n    [https://github.com/dscho/git/runs/6790035528?check_suite_focus=true#step:4:1741]:\n    \n    [...]\n     749 ⏵Run tests\n    1737 === Failed test: t3105-ls-tree-output ===\n    1738 The full logs are in the 'print test failures' step below.\n    1739 See also the 'failed-tests-*' artifacts attached to this run.\n    1740 Error: Process completed with exit code 1.\n    \n    \n    The \"print test failures\" step is still not expanded by default, but at\n    least the output is now so nicely uncluttered that it is really easy to\n    spot it: it is right below the above-quoted error message.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1252%2Fdscho%2Fci-print-failures-again-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1252/dscho/ci-print-failures-again-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1252\n\n .github/workflows/main.yml | 16 ++++++++++++++++\n ci/lib.sh                  |  3 ++-\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex 3fa88b78b6d..cd1f52692a5 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -119,6 +119,10 @@ jobs:\n     - name: test\n       shell: bash\n       run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10\n+    - name: print test failures\n+      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n+      shell: bash\n+      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n       uses: actions/upload-artifact@v2\n@@ -200,6 +204,10 @@ jobs:\n       env:\n         NO_SVN_TESTS: 1\n       run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10\n+    - name: print test failures\n+      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n+      shell: bash\n+      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n       uses: actions/upload-artifact@v2\n@@ -253,6 +261,10 @@ jobs:\n     - uses: actions/checkout@v2\n     - run: ci/install-dependencies.sh\n     - run: ci/run-build-and-tests.sh\n+    - name: print test failures\n+      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n+      shell: bash\n+      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n       uses: actions/upload-artifact@v2\n@@ -282,6 +294,10 @@ jobs:\n     - uses: actions/checkout@v1\n     - run: ci/install-docker-dependencies.sh\n     - run: ci/run-build-and-tests.sh\n+    - name: print test failures\n+      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n+      shell: bash\n+      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n       uses: actions/upload-artifact@v1\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 2f6d9d26e40..b142d254ec1 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -177,7 +177,8 @@ then\n \t\t\ttest_name=\"${test_exit%.exit}\"\n \t\t\ttest_name=\"${test_name##*/}\"\n \t\t\tprintf \"\\\\e[33m\\\\e[1m=== Failed test: ${test_name} ===\\\\e[m\\\\n\"\n-\t\t\techo \"The full logs are in the artifacts attached to this run.\"\n+\t\t\techo \"The full logs are in the 'print test failures' step below.\"\n+\t\t\techo \"See also the 'failed-tests-*' artifacts attached to this run.\"\n \t\t\tcat \"t/test-results/$test_name.markup\"\n \n \t\t\ttrash_dir=\"t/trash directory.$test_name\"\n\nbase-commit: 3069f2a6f4c38e7e599067d2e4a8e31b4f53e2d3\n-- \ngitgitgadget\n"},{"id":"456905","messageId":"xmqqtu8uoii9.fsf@gitster.g","threadId":"57970","inReplyTo":"pull.1252.git.1654684998599.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci(github): bring back the 'print test failures' step","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-08T23:14:22Z","receivedAt":"2022-06-08T23:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> Git now shows better information in the GitHub workflow runs when a test\n> case failed. However, when a test case was implemented incorrectly and\n> therefore does not even run, nothing is shown.\n>\n> Let's bring back the step that prints the full logs of the failed tests,\n> and to improve the user experience, print out an informational message\n> for readers so that they do not have to know/remember where to see the\n> full logs.\n\nMakes quite a lot of sense.  Not all test gotchas are caught by\ntest-lint locally, and something like this would still be needed.\n\nWill queue.\n"},{"id":"456939","messageId":"220609.868rq6t0la.gmgdl@evledraar.gmail.com","threadId":"57970","inReplyTo":"pull.1252.git.1654684998599.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci(github): bring back the 'print test failures' step","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-09T13:06:43Z","receivedAt":"2022-06-09T13:42:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 08 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> Git now shows better information in the GitHub workflow runs when a test\n> case failed.\n\nThis commit message should be more on-point. \"Git\" isn't showing\nanything, and it's unclear that this is a regression fix for fc5a070f591\n(Merge branch 'js/ci-github-workflow-markup', 2022-06-07).\n\n> However, when a test case was implemented incorrectly and therefore\n> does not even run, nothing is shown.\n\nThe *report* came about because of an incorrectly implemented test (of\nmine).\n\nBut the real issue is that your recent change to the CI output is\nimplemented in such a way as to hide entire classes of errors that we'd\npreviously show.\n\nThe test being broken is besides the point, of course we'll have broken\ntests. When we do so we should be showing output relevant to the\nfailure.\n\nThe selling point of your js/ci-github-workflow-markup topic was that we\n*could* accurately summarize our failures as to make browsing the raw\nlogs unnecessary. Quoting your\nhttps://lore.kernel.org/git/pull.1117.git.1643050574.gitgitgadget@gmail.com/:\n\t\n\tI know, and I do not expect any new contributor, not even most seasoned\n\tcontributors to know, that I have to patiently collapse the \"Run\n\tci/run-build-and-tests.sh\" job's log, and instead expand the \"Run\n\tci/print-test-failures.sh\" job log (which did not fail and hence does not\n\tdraw any attention to it).\n\t\n\tI know, and again: I do not expect many others to know this, that I then\n\thave to click into the \"Search logs\" box (not the regular web browser's\n\tsearch via Ctrl+F!) and type in \"not ok\" to find the log of the failed test\n\tcase (and this might still be a \"known broken\" one that is marked via\n\ttest_expect_failure and once again needs to be ignored).\n\t\n\tTo be excessively clear: This is not a great experience!\n\nAs before we agree that it's not the ideal UX experience. But I don't\nsee how this isn't worse, especially for the proverbial new contributor.\n\nI.e. when js/ci-github-workflow-markup landed we'd gotten rid of the\nci/print-test-failures.sh output entirely, because the topic argued that\nwe *could* accurately summarize these failures, and that any remaining\nissues requiring that step were so obscure as to be left to the\nartifacts downloading step.\n\nBut per my report at\nhttps://lore.kernel.org/git/220603.86fskmxd43.gmgdl@evledraar.gmail.com/\n(which b.t.w, would be useful to link to in the commit message) this\nwill fail in cases where we'll die early in test-lib.sh itself, e.g. due\nto a syntax error.\n\nNow, I haven't fully dug into what those cases are, but a regression fix\nreally should take the time to do so. E.g. if we fail in a prereq that\nthe test needs (with exit and/or BUG) do we similarly fail, how about\ne.g. chainlint, or is it just if \"eval\" itself fails?\n\nBut getting back to the point, because of that we'll now be asking those\nusing the CI output to view the default GitHub Markup'd failure, and in\naddition knowing enough about its implementation to know if/when they\nshould be opening the \"print test failures\" step as well, in case it\nover-elided some output.\n\nAs an example, the output is now with this change, and a change on top\nto re-trigger the reported failure:\n\n\thttps://github.com/avar/git/runs/6808880749?check_suite_focus=true#step:4:1757\n\nI.e. per the report above we only show a cryptic \"Error: Process\ncompleted with exit code 1.\", but we do have a \"print test failures\"\nstep now.\n\nAnd here's the version with the \"GitHub markup\" changes reverted:\n\n\thttps://github.com/avar/git/runs/6808882949?check_suite_focus=true#step:4:1719\n\nWhich to be fair isn't much better in itself, but the greater context is\nthat *usually* this step would be sufficient to view a failure, but some\nof the time it isn't, and you need to know enough to know when that's\nthe case. That's new.\n\n> [...]Let's bring back the step that prints the full logs of the failed\n> tests[...]\n\nExcept we still have another bug in js/ci-github-workflow-markup where\nbug in it altered the raw logs themselves. To be fair in this case you\ncan see the failure itself.\n\nHere's what we used to do:\n\n    https://github.com/avar/git/runs/6808882949?check_suite_focus=true#step:5:579\n\nAnd what we do now:\n\n    https://github.com/avar/git/runs/6808880749?check_suite_focus=true#step:5:445\n\nI.e. the former prints \"expecting success of 3105.36 'setup:\nHEAD_short_* variables': \" followed by the test source itself, and then\nthe \"set -x\" output, whereas the new output only has the \"set -x\"\noutput. Both of them get to:\n\n    t3105-ls-tree-output.sh: 20: local: --abbrev: bad variable name\n\nSo it doesn't matter much in this particular case, but we'll have other\ncases where browsing the raw logs is really useful, and having the\nnow-missing context of the test source is even more useful.\n\n> , and to improve the user experience, print out an informational\n> message for readers so that they do not have to know/remember where to\n> see the full logs.\n\nI don't think we should call these \"full logs\" while the bug above\nremains unsolved. Perhaps \"more verbose logs emitted when running with\n--github-workflow-markup, omit the flag to get the full *.out logs\"?\n\n> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> index 3fa88b78b6d..cd1f52692a5 100644\n> --- a/.github/workflows/main.yml\n> +++ b/.github/workflows/main.yml\n> @@ -119,6 +119,10 @@ jobs:\n>      - name: test\n>        shell: bash\n>        run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10\n> +    - name: print test failures\n\nWhen ci/print-test-failures.sh was last in this file before 08dccc8fc1f\n(ci: make it easier to find failed tests' logs in the GitHub workflow,\n2022-05-21) there was no \"name\" field, that's an unrelated change that\nshouldn't be part of a narrow regression fix.\n\n> +      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n\nWe likewise just had \"if failure()\" then, is the distinction different\nin all these cases?\n\n> +      shell: bash\n\n...and you've made every single one of them run with \"bash\" instead of\nthe default shell, which is another \"change while at it\" that isn't\ndiscussed.\n"},{"id":"457031","messageId":"xmqqtu8sfp52.fsf@gitster.g","threadId":"57970","inReplyTo":"220609.868rq6t0la.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] ci(github): bring back the 'print test failures' step","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-10T16:40:25Z","receivedAt":"2022-06-10T16:40:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Wed, Jun 08 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n>> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>>\n>> Git now shows better information in the GitHub workflow runs when a test\n>> case failed.\n>\n> This commit message should be more on-point. \"Git\" isn't showing\n> anything, and it's unclear that this is a regression fix for fc5a070f591\n> (Merge branch 'js/ci-github-workflow-markup', 2022-06-07).\n>\n>> However, when a test case was implemented incorrectly and therefore\n>> does not even run, nothing is shown.\n>\n> The *report* came about because of an incorrectly implemented test (of\n> mine).\n>\n> But the real issue is that your recent change to the CI output is\n> implemented in such a way as to hide entire classes of errors that we'd\n> previously show.\n\nYup, I think we are all on the same page on that.  And we also all\nagree that CI output should help those developers who make mistakes\nby making it easy to see them.  The recent change may have been too\naggressive to hide stuff in the name of \"newbie friendliness\", which\nmay or may not have been a mismatch between what it claimed to aim\nat and what it actually did, but let's not grumble too much about it\nand move on in a more constructive way.\n\nThese fix-ups are to correct such earlier mistakes.\n\n> I don't think we should call these \"full logs\" while the bug above\n> remains unsolved. Perhaps \"more verbose logs emitted when running with\n> --github-workflow-markup, omit the flag to get the full *.out logs\"?\n\nYup.\n\n>> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n>> index 3fa88b78b6d..cd1f52692a5 100644\n>> --- a/.github/workflows/main.yml\n>> +++ b/.github/workflows/main.yml\n>> @@ -119,6 +119,10 @@ jobs:\n>>      - name: test\n>>        shell: bash\n>>        run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10\n>> +    - name: print test failures\n>\n> When ci/print-test-failures.sh was last in this file before 08dccc8fc1f\n> (ci: make it easier to find failed tests' logs in the GitHub workflow,\n> 2022-05-21) there was no \"name\" field, that's an unrelated change that\n> shouldn't be part of a narrow regression fix.\n>\n>> +      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n>\n> We likewise just had \"if failure()\" then, is the distinction different\n> in all these cases?\n>\n>> +      shell: bash\n>\n> ...and you've made every single one of them run with \"bash\" instead of\n> the default shell, which is another \"change while at it\" that isn't\n> discussed.\n\nIf it is so important to support all the other shells in the GitHub\nworkflows environment, we can discuss fix-up patches on top or\nreplacement patches, but does that really matter?  If this were main\nMakefile or ci/*.sh that are supposed to be usable by places other\nthan GitHub Actions environment we use for the CI there, of course\nit would be worth to try being extra portable, but it may be even\nbeneficial to \"fix\" .github/workflows/* stuff, so that we won't have\nto be affected by mistaken use of non-portable shell construct\nwritten there, perhaps?\n"},{"id":"457037","messageId":"220610.86edzws9q0.gmgdl@evledraar.gmail.com","threadId":"57970","inReplyTo":"xmqqtu8sfp52.fsf@gitster.g","subject":"Re: [PATCH] ci(github): bring back the 'print test failures' step","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-10T17:32:07Z","receivedAt":"2022-06-10T17:35:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 10 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>> When ci/print-test-failures.sh was last in this file before 08dccc8fc1f\n>> (ci: make it easier to find failed tests' logs in the GitHub workflow,\n>> 2022-05-21) there was no \"name\" field, that's an unrelated change that\n>> shouldn't be part of a narrow regression fix.\n>>\n>>> +      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n>>\n>> We likewise just had \"if failure()\" then, is the distinction different\n>> in all these cases?\n>>\n>>> +      shell: bash\n>>\n>> ...and you've made every single one of them run with \"bash\" instead of\n>> the default shell, which is another \"change while at it\" that isn't\n>> discussed.\n>\n> If it is so important to support all the other shells in the GitHub\n> workflows environment, we can discuss fix-up patches on top or\n> replacement patches, but does that really matter?  If this were main\n> Makefile or ci/*.sh that are supposed to be usable by places other\n> than GitHub Actions environment we use for the CI there, of course\n> it would be worth to try being extra portable, but it may be even\n> beneficial to \"fix\" .github/workflows/* stuff, so that we won't have\n> to be affected by mistaken use of non-portable shell construct\n> written there, perhaps?\n\nIt just looks like a mistake. The Windows sections need an explicit\n\"bash\" shell, but nothing else does, and the Windows sections had\nexplicit names for somes stuff, but the other ones did not.\n\nSo I think thas was just a case of copy/pasting the first section(s)\nrather than bringing back the pre-image. I think just bringing back the\nold behavior makes sense for a regression fix in a re-roll.\n\nAside from that I think it's very useful to not rely on bash, for future\ndirections of being able to use this tooling more portably, c.f. what I\ndid in my series where you can run \"like CI\" locally, which I'd like to\ndo on Solaris, AIX & whatever else without it being a portability\nhassle.\n\n"},{"id":"459910","messageId":"220725.86sfmpneqp.gmgdl@evledraar.gmail.com","threadId":"57970","inReplyTo":"220610.86edzws9q0.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] ci(github): bring back the 'print test failures' step","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-25T20:00:52Z","receivedAt":"2022-07-25T20:02:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 10 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, Jun 10 2022, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>>> When ci/print-test-failures.sh was last in this file before 08dccc8fc1f\n>>> (ci: make it easier to find failed tests' logs in the GitHub workflow,\n>>> 2022-05-21) there was no \"name\" field, that's an unrelated change that\n>>> shouldn't be part of a narrow regression fix.\n>>>\n>>>> +      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n>>>\n>>> We likewise just had \"if failure()\" then, is the distinction different\n>>> in all these cases?\n>>>\n>>>> +      shell: bash\n>>>\n>>> ...and you've made every single one of them run with \"bash\" instead of\n>>> the default shell, which is another \"change while at it\" that isn't\n>>> discussed.\n>>\n>> If it is so important to support all the other shells in the GitHub\n>> workflows environment, we can discuss fix-up patches on top or\n>> replacement patches, but does that really matter?  If this were main\n>> Makefile or ci/*.sh that are supposed to be usable by places other\n>> than GitHub Actions environment we use for the CI there, of course\n>> it would be worth to try being extra portable, but it may be even\n>> beneficial to \"fix\" .github/workflows/* stuff, so that we won't have\n>> to be affected by mistaken use of non-portable shell construct\n>> written there, perhaps?\n>\n> It just looks like a mistake. The Windows sections need an explicit\n> \"bash\" shell, but nothing else does, and the Windows sections had\n> explicit names for somes stuff, but the other ones did not.\n>\n> So I think thas was just a case of copy/pasting the first section(s)\n> rather than bringing back the pre-image. I think just bringing back the\n> old behavior makes sense for a regression fix in a re-roll.\n>\n> Aside from that I think it's very useful to not rely on bash, for future\n> directions of being able to use this tooling more portably, c.f. what I\n> did in my series where you can run \"like CI\" locally, which I'd like to\n> do on Solaris, AIX & whatever else without it being a portability\n> hassle.\n\nIt turns out this is also a regression for our CI, if linux-musl fails\nit'll emit:\n\n    OCI runtime exec failed: exec failed: container_linux.go:380:\n    starting container process caused: exec: \"bash\": executable file not\n    found in $PATH: unknown\n\n"}]}