{"thread":{"id":"61925","subject":"[PATCH] tests: drop use of 'tee' that hides exit status","startedAt":"2024-08-08T21:19:31Z","lastAt":"2024-08-13T17:23:28Z","messageCount":6,"participants":["Junio C Hamano","Rubén Justo","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"500525","messageId":"xmqq4j7uhfvm.fsf@gitster.g","threadId":"61925","inReplyTo":null,"subject":"[PATCH] tests: drop use of 'tee' that hides exit status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-08T21:19:25Z","receivedAt":"2024-08-08T21:19:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A few tests have \"| tee output\" downstream of a git command, and\nthen inspect the contents of the file.  The net effect is that we\nuse an extra process, and hide the exit status from the upstream git\ncommand.\n\nIn none of these tests, I do not see a reason why we want to hide a\npossible failure from these git commands.  Replace the use of tee\nwith a plain simple redirection.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t1001-read-tree-m-2way.sh | 2 +-\n t/t5523-push-upstream.sh    | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh\nindex 88c524f655..48a1550371 100755\n--- c/t/t1001-read-tree-m-2way.sh\n+++ w/t/t1001-read-tree-m-2way.sh\n@@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' '\n \n test_expect_success 'a/b vs a, plus c/d case test.' '\n \tread_tree_u_must_succeed -u -m \"$treeH\" \"$treeM\" &&\n-\tgit ls-files --stage | tee >treeMcheck.out &&\n+\tgit ls-files --stage >treeMcheck.out &&\n \ttest_cmp treeM.out treeMcheck.out\n '\n \ndiff --git c/t/t5523-push-upstream.sh w/t/t5523-push-upstream.sh\nindex 1f859ade16..4ad36a31e1 100755\n--- c/t/t5523-push-upstream.sh\n+++ w/t/t5523-push-upstream.sh\n@@ -124,14 +124,14 @@ test_expect_success TTY 'push --no-progress suppresses progress' '\n test_expect_success TTY 'quiet push' '\n \tensure_fresh_upstream &&\n \n-\ttest_terminal git push --quiet --no-progress upstream main 2>&1 | tee output &&\n+\ttest_terminal git push --quiet --no-progress upstream main >output 2>&1 &&\n \ttest_must_be_empty output\n '\n \n test_expect_success TTY 'quiet push -u' '\n \tensure_fresh_upstream &&\n \n-\ttest_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output &&\n+\ttest_terminal git push --quiet -u --no-progress upstream main >output 2>&1 &&\n \ttest_must_be_empty output\n '\n \n"},{"id":"500528","messageId":"93e6fdd3-4a15-4cc5-929a-aa5e79f44e1c@gmail.com","threadId":"61925","inReplyTo":"xmqq4j7uhfvm.fsf@gitster.g","subject":"Re: [PATCH] tests: drop use of 'tee' that hides exit status","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-08-09T00:37:29Z","receivedAt":"2024-08-09T00:37:32Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Thu, Aug 08, 2024 at 02:19:25PM -0700, Junio C Hamano wrote:\n> A few tests have \"| tee output\" downstream of a git command, and\n> then inspect the contents of the file.  The net effect is that we\n> use an extra process, and hide the exit status from the upstream git\n> command.\n> \n> In none of these tests, I do not see a reason why we want to hide a\n\nThis double negative caught my attention.  The message is\nunderstandable; perhaps \"In none of these tests I see ...\" would have\nbeen clearer.\n\n> possible failure from these git commands.  Replace the use of tee\n> with a plain simple redirection.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  t/t1001-read-tree-m-2way.sh | 2 +-\n>  t/t5523-push-upstream.sh    | 4 ++--\n>  2 files changed, 3 insertions(+), 3 deletions(-)\n\nA simple search only points to these two files:\n\n   $ git grep '\\s*git.*|\\s*tee' \"t/t[0-9]*.sh\"\n   t/t1001-read-tree-m-2way.sh:400:        git ls-files --stage | tee >treeMcheck.out &&\n   t/t5523-push-upstream.sh:127:   test_terminal git push --quiet --no-progress upstream main 2>&1 | tee output &&\n   t/t5523-push-upstream.sh:134:   test_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output &&\n\nAnd the following three changes are in line with the result:\n\n> \n> diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh\n> index 88c524f655..48a1550371 100755\n> --- c/t/t1001-read-tree-m-2way.sh\n> +++ w/t/t1001-read-tree-m-2way.sh\n> @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' '\n>  \n>  test_expect_success 'a/b vs a, plus c/d case test.' '\n>  \tread_tree_u_must_succeed -u -m \"$treeH\" \"$treeM\" &&\n> -\tgit ls-files --stage | tee >treeMcheck.out &&\n> +\tgit ls-files --stage >treeMcheck.out &&\n>  \ttest_cmp treeM.out treeMcheck.out\n>  '\n>  \n> diff --git c/t/t5523-push-upstream.sh w/t/t5523-push-upstream.sh\n> index 1f859ade16..4ad36a31e1 100755\n> --- c/t/t5523-push-upstream.sh\n> +++ w/t/t5523-push-upstream.sh\n> @@ -124,14 +124,14 @@ test_expect_success TTY 'push --no-progress suppresses progress' '\n>  test_expect_success TTY 'quiet push' '\n>  \tensure_fresh_upstream &&\n>  \n> -\ttest_terminal git push --quiet --no-progress upstream main 2>&1 | tee output &&\n> +\ttest_terminal git push --quiet --no-progress upstream main >output 2>&1 &&\n>  \ttest_must_be_empty output\n>  '\n>  \n>  test_expect_success TTY 'quiet push -u' '\n>  \tensure_fresh_upstream &&\n>  \n> -\ttest_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output &&\n> +\ttest_terminal git push --quiet -u --no-progress upstream main >output 2>&1 &&\n>  \ttest_must_be_empty output\n>  '\n>  \n\nLooks good.\n\nThanks.\n"},{"id":"500530","messageId":"xmqqjzgqfpxo.fsf@gitster.g","threadId":"61925","inReplyTo":"93e6fdd3-4a15-4cc5-929a-aa5e79f44e1c@gmail.com","subject":"Re: [PATCH] tests: drop use of 'tee' that hides exit status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-09T01:25:07Z","receivedAt":"2024-08-09T01:25:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>> In none of these tests, I do not see a reason why we want to hide a\n>\n> This double negative caught my attention.  The message is\n> understandable; perhaps \"In none of these tests I see ...\" would have\n> been clearer.\n\nYou're absolutely right, and it is not just \"would have been\nclearer\"---your fix makes a nonsense double-negation into a correct\nsentence.  I'd go with \"In any of ... I do not see\" while queuing.\n\nThanks.\n"},{"id":"500771","messageId":"f5d0510c-d455-5e80-08b7-e08c81df4adb@gmx.de","threadId":"61925","inReplyTo":"xmqq4j7uhfvm.fsf@gitster.g","subject":"Re: [PATCH] tests: drop use of 'tee' that hides exit status","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-08-13T12:22:11Z","receivedAt":"2024-08-13T12:22:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 8 Aug 2024, Junio C Hamano wrote:\n\n> diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh\n> index 88c524f655..48a1550371 100755\n> --- c/t/t1001-read-tree-m-2way.sh\n> +++ w/t/t1001-read-tree-m-2way.sh\n> @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' '\n>\n>  test_expect_success 'a/b vs a, plus c/d case test.' '\n>  \tread_tree_u_must_succeed -u -m \"$treeH\" \"$treeM\" &&\n> -\tgit ls-files --stage | tee >treeMcheck.out &&\n> +\tgit ls-files --stage >treeMcheck.out &&\n\nWhile this obviously fixes the bug where the test case was incorrectly\nallowed to continue after a failing `git ls-files --stage` call, I will\nnote that I interpret the intention of the `| tee` as showing the output\nin the logs in addition to redirecting it to a file for the benefit of\nadditional checks in the same test case.\n\nI know that I investigate CI failures a lot more than most, therefore many\nmight disagree that removing such output makes future debugging\npotentially a lot harder.\n\nSo, what to do here? I don't really know. The easiest option that most\nother people would likely be happy with would be to go with the `| tee`\ndropping.\n\nA more complex solution (and hence inherently more fragile, which I would\nlove to avoid) would be to introduce a helper function that redirects the\noutput but makes sure that it is shown even in case of a failure.\nSomething like this:\n\n\tredirect_and_show () {\n\t\tlocal file=\"$1\"\n\t\tshift\n\n\t\t\"$@\" >\"$file\"\n\t\tlocal ret=$?\n\n\t\tcat \"$file\"\n\t\treturn $ret\n\t}\n\nAs I said, I loathe the complexity of this construct. Hopefully the switch\nto the more powerful unit testing framework will make these sort of\nconsiderations moot at some point.\n\nCiao,\nJohannes\n"},{"id":"500775","messageId":"20240813125900.GD968816@coredump.intra.peff.net","threadId":"61925","inReplyTo":"f5d0510c-d455-5e80-08b7-e08c81df4adb@gmx.de","subject":"Re: [PATCH] tests: drop use of 'tee' that hides exit status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-13T12:59:00Z","receivedAt":"2024-08-13T12:59:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 13, 2024 at 02:22:11PM +0200, Johannes Schindelin wrote:\n\n> >  test_expect_success 'a/b vs a, plus c/d case test.' '\n> >  \tread_tree_u_must_succeed -u -m \"$treeH\" \"$treeM\" &&\n> > -\tgit ls-files --stage | tee >treeMcheck.out &&\n> > +\tgit ls-files --stage >treeMcheck.out &&\n> \n> While this obviously fixes the bug where the test case was incorrectly\n> allowed to continue after a failing `git ls-files --stage` call, I will\n> note that I interpret the intention of the `| tee` as showing the output\n> in the logs in addition to redirecting it to a file for the benefit of\n> additional checks in the same test case.\n> \n> I know that I investigate CI failures a lot more than most, therefore many\n> might disagree that removing such output makes future debugging\n> potentially a lot harder.\n\nI also look at a lot of CI failures. IMHO this isn't that big an issue.\n\nIf \"ls-files\" succeeds, then the file is fed to test_cmp(), where we\nreport any differences from the expected output. I find this way more\nuseful than a dump of the file.\n\nIf \"ls-files\" fails, we won't see the output. But we will see its\nstderr, which is probably the more useful stream. Though for cases where\nwe redirect stderr, I do think it can sometimes be confusing (this is\njust not one of those).\n\nBut even in that case, we tar up the trash directory of the failing\nscripts and save them separately. So the output is still recorded, but\nin a less expensive way than dumping it into the log (i.e., it is only\nwhen we see a failure).\n\nSometimes that isn't sufficient, if later tests overwrite the logs. I\nusually run with \"--immediate\" when locally debugging tests. And I've\nvery occasionally done the same with CI, pushing up a commit on top to\ncustomize the test environment.\n\nI do wonder if \"--immediate\" mode would be a better default for CI,\nsince we expect things to pass. But it it comes at the cost of giving no\ninformation about subsequent tests. So I think it really depends on\nwhether you are debugging tests you just wrote, or chasing down a flake\nor other issue introduced in a script that was not recently touched.\n\nI guess we could record the trash directory state for every failing test\nwithin a script. That would need some cooperation from test-lib.sh, but\nprobably wouldn't be too hard.\n\n-Peff\n"},{"id":"500809","messageId":"xmqqcymc5oc5.fsf@gitster.g","threadId":"61925","inReplyTo":"f5d0510c-d455-5e80-08b7-e08c81df4adb@gmx.de","subject":"Re: [PATCH] tests: drop use of 'tee' that hides exit status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-13T17:23:22Z","receivedAt":"2024-08-13T17:23:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Junio,\n>\n> On Thu, 8 Aug 2024, Junio C Hamano wrote:\n>\n>> diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh\n>> index 88c524f655..48a1550371 100755\n>> --- c/t/t1001-read-tree-m-2way.sh\n>> +++ w/t/t1001-read-tree-m-2way.sh\n>> @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' '\n>>\n>>  test_expect_success 'a/b vs a, plus c/d case test.' '\n>>  \tread_tree_u_must_succeed -u -m \"$treeH\" \"$treeM\" &&\n>> -\tgit ls-files --stage | tee >treeMcheck.out &&\n>> +\tgit ls-files --stage >treeMcheck.out &&\n>\n> While this obviously fixes the bug where the test case was incorrectly\n> allowed to continue after a failing `git ls-files --stage` call, I will\n> note that I interpret the intention of the `| tee` as showing the output\n> in the logs in addition to redirecting it to a file for the benefit of\n> additional checks in the same test case.\n\nIf we really want to do that, we'd do\n\n    git ls-files --stage >treeMcheck.out &&\n    cat treeMcheck.out &&\n\ninstead.  You wouldn't lose the exit status, and the output would\nshow the contents just like \"tee\" would.\n\nWe however tend to also remove a leftover debugging \"cat\", so the\n\"cat\" is likely to be removed during such a clean-up.\n\n> So, what to do here? I don't really know. The easiest option that most\n> other people would likely be happy with would be to go with the `| tee`\n> dropping.\n\nAs long as we treat the CI as a \"batch\" environment, the postmortem\ntarball of the trash directory is probably the best we can do I can\nthink of.\n"}]}