ci(dockerized): do show the result of failing tests again
PATCH 1/1 ci(dockerized): do show the result of failing tests again
From: Johannes Schindelin <[email protected]>
The quality of tests/test suites does not show as much when there are no breakages as in the amount of time required after bugs trigger test failures before the bugs can be identified, analyzed and resolved.
As such, it is an unfortunate side effect of 2a21098b98a (github: adapt containerized jobs to be rootless, 2025-01-10) that the output of failed test cases, which was shown before that change directly in the build logs, is now no longer shown at all.
The reason is a side effect of trying to run the build and the tests with permissions other than the root user, but without providing the prerequisite permissions to signal what tests failed and whose output hence needs to be included in the logs.
The way this signaling works is for the workflow to write into special-purpose files whose path is specific to the current workflow step and which can be accessed via the $GITHUB_ENV environment variable, which differs between workflow steps. It is this file that is missing write permission for the builder user that was introduced in above-mentioned commit.
The solution is simple: make the file world-writable.
Technically, this write permission should be removed after the step has completed, if proper security practices were to be upheld, but since nothing uses that file again, it does not matter, and the fix is more succinct this way.
This commit is best viewed with --color-words.
---
ci(dockerized): do show the result of failing tests again
It has become quite hard to debug CI failures when they happen in one of the Dockerized jobs, as the actual test failures are now hidden. This was most likely an oversight when 2a21098b98a (github: adapt containerized jobs to be rootless, 2025-01-10) was merged in 2bf3c7fab19 (Merge branch 'ps/ci-misc-updates', 2025-02-06), v2.49.0-rc0~55, and I had reported this as a regression in
https://lore.kernel.org/git/[email protected]/.
Seeing no movement on my report, and having the pressure of newly-failing tests during the v2.52.0-rc0 rebase of Git for Windows, I was kind of forced into fixing this in Git for Windows. Here I upstream the fix.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2003%2Fdscho%2Ffix-failure-reporting-in-dockerized-ci-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2003/dscho/fix-failure-reporting-in-dockerized-ci-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2003
.github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
.github/workflows/main.yml +1 -1
@@ -433,7 +433,7 @@ jobs: - run: ci/install-dependencies.sh - run: useradd builder --create-home - run: chown -R builder . - - run: sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh + - run: chmod o+w $GITHUB_ENV && sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh - name: print test failures if: failure() && env.FAILED_TEST_ARTIFACTS != '' run: sudo --preserve-env --set-home --user=builder ci/print-test-failures.sh
base-commit: 621415c8b5371a4734315232a780dd8282f6fe4f
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 16 lines · writes:
From: Johannes Schindelin <[email protected]>
The quality of tests/test suites does not show as much when there are no breakages as in the amount of time required after bugs trigger test failures before the bugs can be identified, analyzed and resolved.
As such, it is an unfortunate side effect of
2a21098b98a(github: adapt containerized jobs to be rootless, 2025-01-10) that the output of failed test cases, which was shown before that change directly in the build logs, is now no longer shown at all.The reason is a side effect of trying to run the build and the tests with permissions other than the
rootuser, but without providing the prerequisite permissions to signal what tests failed and whose output hence needs to be included in the logs.
Quoted 8 lines
The way this signaling works is for the workflow to write into special-purpose files whose path is specific to the current workflow step and which can be accessed via the
$GITHUB_ENVenvironment variable, which differs between workflow steps. It is this file that is missing write permission for thebuilderuser that was introduced in above-mentioned commit.The solution is simple: make the file world-writable.
I expected to see a+w not o+w from this statement; as long as it works I have no strong objections, but if I saw o+w without the above explanation I would probably have wondered who are in the group that we do not want this file touched by.
Quoted 22 lines
Technically, this write permission should be removed after the step has completed, if proper security practices were to be upheld, but since nothing uses that file again, it does not matter, and the fix is more succinct this way.
This commit is best viewed with
--color-words.Signed-off-by Johannes Schindelin <[email protected]>---
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2003%2Fdscho%2Ffix-failure-reporting-in-dockerized-ci-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2003/dscho/fix-failure-reporting-in-dockerized-ci-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2003.github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-).github/workflows/main.yml +0
-0
.github/workflows/main.yml jobs: · line 439
Quoted 8 lines
.github/workflows/main.yml +1
-1@@ -433,7 +433,7 @@ jobs: - run: ci/install-dependencies.sh - run: useradd builder --create-home - run: chown -R builder . - - run: sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh + - run: chmod o+w $GITHUB_ENV && sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh - name: print test failures if: failure() && env.FAILED_TEST_ARTIFACTS != '' run: sudo --preserve-env --set-home --user=builder ci/print-test-failures.sh
Thanks. Will apply.
Quoted 12 lines · Junio C Hamano
Quoted 1 line
The solution is simple: make the file world-writable.
I expected to see a+w not o+w from this statement; as long as it works I have no strong objections, but if I saw o+w without the above explanation I would probably have wondered who are in the group that we do not want this file touched by.
...Quoted 5 lines
- run: useradd builder --create-home
- run: chown -R builder ..github/workflows/main.yml +1
-1- - run: sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh + - run: chmod o+w $GITHUB_ENV && sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh - name: print test failures
Unless I hear that "user X belongs to the same group as our user that runs 'chmod' on $GITHUB_ENV, and we do not want that user to be writing into the file", I'll amend the patch text to match the "solution" described in the proposed log message to "chmod a+w", before we mark the topic for 'next'.
Thanks.
Quoted 6 lines · Johannes Schindelin via GitGitGadget
The way this signaling works is for the workflow to write into special-purpose files whose path is specific to the current workflow step and which can be accessed via the
$GITHUB_ENVenvironment variable, which differs between workflow steps. It is this file that is missing write permission for thebuilderuser that was introduced in above-mentioned commit.
Thanks for fixing this. It bit me recently, but I hadn't had time to look at it yet.
BTW, I ran into a similar issue (no useful output from a failed test) in the windows-meson job, but the cause is totally different there:
https://lore.kernel.org/git/[email protected]/
-Peff
Quoted 6 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
The quality of tests/test suites does not show as much when there are no breakages as in the amount of time required after bugs trigger test failures before the bugs can be identified, analyzed and resolved.
I found this paragraph hard to parse. After re-reading a couple times, does the following convey the same meaning?:
The quality of tests and test suites is most apparent not when everything passes, but in how quickly bugs can be identified, analyzed, and resolved after test failures occur.
Quoted 9 lines · Elijah Newren
Quoted 6 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
The quality of tests/test suites does not show as much when there are no breakages as in the amount of time required after bugs trigger test failures before the bugs can be identified, analyzed and resolved.
Quoted 6 lines
I found this paragraph hard to parse. After re-reading a couple times, does the following convey the same meaning?:
The quality of tests and test suites is most apparent not when everything passes, but in how quickly bugs can be identified, analyzed, and resolved after test failures occur.
FWIW, I had the same "I cannot quite figure out how this paragraph really wants to help readers by saying this" reaction to the paragraph. Your rewrite finally helped me understand the intention (if that is what the originall wanted to say, that is).
Thanks.
Hi Elijah,
Quoted 9 lines · Elijah Newren
Quoted 6 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
The quality of tests/test suites does not show as much when there are no breakages as in the amount of time required after bugs trigger test failures before the bugs can be identified, analyzed and resolved.
Quoted 6 lines
I found this paragraph hard to parse. After re-reading a couple times, does the following convey the same meaning?:
The quality of tests and test suites is most apparent not when everything passes, but in how quickly bugs can be identified, analyzed, and resolved after test failures occur.
Yes, this reflects what I tried to say.
Ciao,
Johannes
Quoted 13 lines · Johannes Schindelin
Hi Elijah,
Quoted 9 lines · Elijah Newren
Quoted 6 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
The quality of tests/test suites does not show as much when there are no breakages as in the amount of time required after bugs trigger test failures before the bugs can be identified, analyzed and resolved.
Quoted 8 lines
Quoted 6 lines
I found this paragraph hard to parse. After re-reading a couple times, does the following convey the same meaning?:
The quality of tests and test suites is most apparent not when everything passes, but in how quickly bugs can be identified, analyzed, and resolved after test failures occur.
Yes, this reflects what I tried to say.
So, do you mind if I locally amended the log message, or should we expect an updated patch sent to the list? For a small thing like this, either is fine by me.
Thanks.
Hi Junio,
Quoted 20 lines · Junio C Hamano
Quoted 18 lines · Johannes Schindelin
Quoted 15 lines · Elijah Newren
Quoted 6 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
The quality of tests/test suites does not show as much when there are no breakages as in the amount of time required after bugs trigger test failures before the bugs can be identified, analyzed and resolved.
I found this paragraph hard to parse. After re-reading a couple times, does the following convey the same meaning?:
The quality of tests and test suites is most apparent not when everything passes, but in how quickly bugs can be identified, analyzed, and resolved after test failures occur.
Quoted 5 lines
Quoted 1 line
Yes, this reflects what I tried to say.
So, do you mind if I locally amended the log message, or should we expect an updated patch sent to the list? For a small thing like this, either is fine by me.
Sure, I saw that you amended the log message and also changed the chmod to be a bit more robust.
Ciao,
Johannes