{"thread":{"id":"59030","subject":"[PATCH] ci(github): restore \"print test failures\" step name","startedAt":"2023-01-03T10:28:17Z","lastAt":"2023-01-04T21:51:11Z","messageCount":2,"participants":["Phillip Wood via GitGitGadget","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469763","messageId":"pull.1453.git.1672741640587.gitgitgadget@gmail.com","threadId":"59030","inReplyTo":null,"subject":"[PATCH] ci(github): restore \"print test failures\" step name","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-03T10:27:20Z","receivedAt":"2023-01-03T10:28:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nAs well as removing the explicit shell setting d8b21a0fe2 (CI: don't\nexplicitly pick \"bash\" shell outside of Windows, fix regression,\n2022-12-07) also reverted the name of the print test failures step\nintroduced by 5aeb145780f (ci(github): bring back the 'print test\nfailures' step, 2022-06-08). This is unfortunate as 5aeb145780f added a\nmessage to direct contributors to the \"print test failures\" step when a\ntest fails and that step is no-longer known by that name on the\nnon-windows ci jobs.\n\nIn principle we could update the message to print the correct name for\nthe step but then we'd have to deal with having two different names for\nthe same step on different jobs. It is simpler for the implementation\nand contributors to use the same name for this step on all jobs.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    ci(github): restore \"print test failures\" step name\n    \n    Ævar seems to think the name change in 5aeb145780f was unintentional [1]\n    but looking at the original commit I don't think that's the case.\n    \n    [1]\n    https://lore.kernel.org/git/221208.86sfhq6pmg.gmgdl@evledraar.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1453%2Fphillipwood%2Fci-print-test-failures-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1453/phillipwood/ci-print-test-failures-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1453\n\n .github/workflows/main.yml | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex e67847a682c..126eac8239d 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -265,8 +265,9 @@ jobs:\n     - uses: actions/checkout@v3\n     - run: ci/install-dependencies.sh\n     - run: ci/run-build-and-tests.sh\n-    - run: ci/print-test-failures.sh\n+    - name: print test failures\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\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@v3\n@@ -298,8 +299,9 @@ jobs:\n       if: matrix.vector.jobname == 'linux32'\n     - run: ci/install-docker-dependencies.sh\n     - run: ci/run-build-and-tests.sh\n-    - run: ci/print-test-failures.sh\n+    - name: print test failures\n       if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n+      run: ci/print-test-failures.sh\n     - name: Upload failed tests' directories\n       if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname != 'linux32'\n       uses: actions/upload-artifact@v3\n\nbase-commit: 8a4e8f6a67e7fc97048d4666eec38399b88e0e3b\n-- \ngitgitgadget\n"},{"id":"469791","messageId":"230104.86mt6yt1dk.gmgdl@evledraar.gmail.com","threadId":"59030","inReplyTo":"pull.1453.git.1672741640587.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci(github): restore \"print test failures\" step name","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-04T21:41:32Z","receivedAt":"2023-01-04T21:51:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 03 2023, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> As well as removing the explicit shell setting d8b21a0fe2 (CI: don't\n> explicitly pick \"bash\" shell outside of Windows, fix regression,\n> 2022-12-07) also reverted the name of the print test failures step\n> introduced by 5aeb145780f (ci(github): bring back the 'print test\n> failures' step, 2022-06-08). This is unfortunate as 5aeb145780f added a\n> message to direct contributors to the \"print test failures\" step when a\n> test fails and that step is no-longer known by that name on the\n> non-windows ci jobs.\n>\n> In principle we could update the message to print the correct name for\n> the step but then we'd have to deal with having two different names for\n> the same step on different jobs. It is simpler for the implementation\n> and contributors to use the same name for this step on all jobs.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>     ci(github): restore \"print test failures\" step name\n>     \n>     Ævar seems to think the name change in 5aeb145780f was unintentional [1]\n>     but looking at the original commit I don't think that's the case.\n>     \n>     [1]\n>     https://lore.kernel.org/git/221208.86sfhq6pmg.gmgdl@evledraar.gmail.com/\n\nReading it again I think you're right, i.e. that the migration of\neverything to \"bash\" was unintentional & a result of copy/pasting, but\nthe initial goal was to mention the \"print test failures\" step....\n\n> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n> index e67847a682c..126eac8239d 100644\n> --- a/.github/workflows/main.yml\n> @@ -265,8 +265,9 @@ jobs:\n>      - uses: actions/checkout@v3\n>      - run: ci/install-dependencies.sh\n>      - run: ci/run-build-and-tests.sh\n> -    - run: ci/print-test-failures.sh\n> +    - name: print test failures\n>        if: failure() && env.FAILED_TEST_ARTIFACTS != ''\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@v3\n> @@ -298,8 +299,9 @@ jobs:\n>        if: matrix.vector.jobname == 'linux32'\n>      - run: ci/install-docker-dependencies.sh\n>      - run: ci/run-build-and-tests.sh\n> -    - run: ci/print-test-failures.sh\n> +    - name: print test failures\n>        if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n> +      run: ci/print-test-failures.sh\n>      - name: Upload failed tests' directories\n>        if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname != 'linux32'\n>        uses: actions/upload-artifact@v3\n>\n> base-commit: 8a4e8f6a67e7fc97048d4666eec38399b88e0e3b\n\n...but as far as moving forward, why make every other job be\ninconsistent in naming this one step, rather than just making the\nWindows ones consistent with rest?\n\nI.e. why not:\n\n\tdiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\n\tindex e67847a682c..2bfb841206e 100644\n\t--- a/.github/workflows/main.yml\n\t+++ b/.github/workflows/main.yml\n\t@@ -119,8 +119,7 @@ jobs:\n\t     - name: test\n\t       shell: bash\n\t       run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10\n\t-    - name: print test failures\n\t-      if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n\t+    - if: failure() && env.FAILED_TEST_ARTIFACTS != ''\n\t       shell: bash\n\t       run: ci/print-test-failures.sh\n\t     - name: Upload failed tests' directories\n\tdiff --git a/ci/lib.sh b/ci/lib.sh\n\tindex db7105e8a8d..d6450fd4957 100755\n\t--- a/ci/lib.sh\n\t+++ b/ci/lib.sh\n\t@@ -183,7 +183,7 @@ then\n\t \t\t\ttest_name=\"${test_exit%.exit}\"\n\t \t\t\ttest_name=\"${test_name##*/}\"\n\t \t\t\tprintf \"\\\\e[33m\\\\e[1m=== Failed test: ${test_name} ===\\\\e[m\\\\n\"\n\t-\t\t\techo \"The full logs are in the 'print test failures' step below.\"\n\t+\t\t\techo \"The full logs are in the 'ci/print-test-failures.sh' step below.\"\n\t \t\t\techo \"See also the 'failed-tests-*' artifacts attached to this run.\"\n\t \t\t\tcat \"t/test-results/$test_name.markup\"\n\t \n\nThe implicit argument being made here & in the original 5aeb145780f is\nthat it's a good thing to give explicit \"names\"'s to all of these steps.\n\nFor some of the steps I agree, e.g. \"test\" is probably better than\n\". /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10\" or whatever,\nand e.g. \"generate Visual Studio solution\" is definitely better than\nsome multi-line shellscript (I'm not sure how the UX would even turn\nthat into a collapsible title).\n\nBut in the case of \"ci/print-test-failures.sh\",\n\"ci/run-build-and-tests.sh\", \"ci/install-dependencies.sh\" etc. the\nscript name is already self-descriptive, so giving it a name just serves\nto obscure the connection between the step & the script implementing it,\nwhich is otherwise immediately apparent.\n\nSo I think it's better to just remove the \"name\" label in those cases\nwhere the Windows jobs can use such a script, which we're already not\nnaming in the *nix jobs.\n\nIt's a minor point either way, and your change would also be an\nimprovement. I just think it's better to aim for e.g. this snippet (from\nthe current main.yml):\n\n    - run: ci/install-docker-dependencies.sh\n    - run: ci/run-build-and-tests.sh\n    - run: ci/print-test-failures.sh\n\nRather than providing \"name\" fields for them all, and doubling the line\ncount for no apparent (at least to me) reason.\n"}]}