| Message ID | xmqq4j7uhfvm.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0d66f601a9f82a6f3b4240cffa2b02ed5393f1ee |
| Headers | show |
| Series | tests: drop use of 'tee' that hides exit status | expand |
On Thu, Aug 08, 2024 at 02:19:25PM -0700, Junio C Hamano wrote: > A few tests have "| tee output" downstream of a git command, and > then inspect the contents of the file. The net effect is that we > use an extra process, and hide the exit status from the upstream git > command. > > In none of these tests, I do not see a reason why we want to hide a This double negative caught my attention. The message is understandable; perhaps "In none of these tests I see ..." would have been clearer. > possible failure from these git commands. Replace the use of tee > with a plain simple redirection. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > t/t1001-read-tree-m-2way.sh | 2 +- > t/t5523-push-upstream.sh | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) A simple search only points to these two files: $ git grep '\s*git.*|\s*tee' "t/t[0-9]*.sh" t/t1001-read-tree-m-2way.sh:400: git ls-files --stage | tee >treeMcheck.out && t/t5523-push-upstream.sh:127: test_terminal git push --quiet --no-progress upstream main 2>&1 | tee output && t/t5523-push-upstream.sh:134: test_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output && And the following three changes are in line with the result: > > diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh > index 88c524f655..48a1550371 100755 > --- c/t/t1001-read-tree-m-2way.sh > +++ w/t/t1001-read-tree-m-2way.sh > @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' ' > > test_expect_success 'a/b vs a, plus c/d case test.' ' > read_tree_u_must_succeed -u -m "$treeH" "$treeM" && > - git ls-files --stage | tee >treeMcheck.out && > + git ls-files --stage >treeMcheck.out && > test_cmp treeM.out treeMcheck.out > ' > > diff --git c/t/t5523-push-upstream.sh w/t/t5523-push-upstream.sh > index 1f859ade16..4ad36a31e1 100755 > --- c/t/t5523-push-upstream.sh > +++ w/t/t5523-push-upstream.sh > @@ -124,14 +124,14 @@ test_expect_success TTY 'push --no-progress suppresses progress' ' > test_expect_success TTY 'quiet push' ' > ensure_fresh_upstream && > > - test_terminal git push --quiet --no-progress upstream main 2>&1 | tee output && > + test_terminal git push --quiet --no-progress upstream main >output 2>&1 && > test_must_be_empty output > ' > > test_expect_success TTY 'quiet push -u' ' > ensure_fresh_upstream && > > - test_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output && > + test_terminal git push --quiet -u --no-progress upstream main >output 2>&1 && > test_must_be_empty output > ' > Looks good. Thanks.
Rubén Justo <rjusto@gmail.com> writes: >> In none of these tests, I do not see a reason why we want to hide a > > This double negative caught my attention. The message is > understandable; perhaps "In none of these tests I see ..." would have > been clearer. You're absolutely right, and it is not just "would have been clearer"---your fix makes a nonsense double-negation into a correct sentence. I'd go with "In any of ... I do not see" while queuing. Thanks.
Hi Junio, On Thu, 8 Aug 2024, Junio C Hamano wrote: > diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh > index 88c524f655..48a1550371 100755 > --- c/t/t1001-read-tree-m-2way.sh > +++ w/t/t1001-read-tree-m-2way.sh > @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' ' > > test_expect_success 'a/b vs a, plus c/d case test.' ' > read_tree_u_must_succeed -u -m "$treeH" "$treeM" && > - git ls-files --stage | tee >treeMcheck.out && > + git ls-files --stage >treeMcheck.out && While this obviously fixes the bug where the test case was incorrectly allowed to continue after a failing `git ls-files --stage` call, I will note that I interpret the intention of the `| tee` as showing the output in the logs in addition to redirecting it to a file for the benefit of additional checks in the same test case. I know that I investigate CI failures a lot more than most, therefore many might disagree that removing such output makes future debugging potentially a lot harder. So, what to do here? I don't really know. The easiest option that most other people would likely be happy with would be to go with the `| tee` dropping. A more complex solution (and hence inherently more fragile, which I would love to avoid) would be to introduce a helper function that redirects the output but makes sure that it is shown even in case of a failure. Something like this: redirect_and_show () { local file="$1" shift "$@" >"$file" local ret=$? cat "$file" return $ret } As I said, I loathe the complexity of this construct. Hopefully the switch to the more powerful unit testing framework will make these sort of considerations moot at some point. Ciao, Johannes
On Tue, Aug 13, 2024 at 02:22:11PM +0200, Johannes Schindelin wrote: > > test_expect_success 'a/b vs a, plus c/d case test.' ' > > read_tree_u_must_succeed -u -m "$treeH" "$treeM" && > > - git ls-files --stage | tee >treeMcheck.out && > > + git ls-files --stage >treeMcheck.out && > > While this obviously fixes the bug where the test case was incorrectly > allowed to continue after a failing `git ls-files --stage` call, I will > note that I interpret the intention of the `| tee` as showing the output > in the logs in addition to redirecting it to a file for the benefit of > additional checks in the same test case. > > I know that I investigate CI failures a lot more than most, therefore many > might disagree that removing such output makes future debugging > potentially a lot harder. I also look at a lot of CI failures. IMHO this isn't that big an issue. If "ls-files" succeeds, then the file is fed to test_cmp(), where we report any differences from the expected output. I find this way more useful than a dump of the file. If "ls-files" fails, we won't see the output. But we will see its stderr, which is probably the more useful stream. Though for cases where we redirect stderr, I do think it can sometimes be confusing (this is just not one of those). But even in that case, we tar up the trash directory of the failing scripts and save them separately. So the output is still recorded, but in a less expensive way than dumping it into the log (i.e., it is only when we see a failure). Sometimes that isn't sufficient, if later tests overwrite the logs. I usually run with "--immediate" when locally debugging tests. And I've very occasionally done the same with CI, pushing up a commit on top to customize the test environment. I do wonder if "--immediate" mode would be a better default for CI, since we expect things to pass. But it it comes at the cost of giving no information about subsequent tests. So I think it really depends on whether you are debugging tests you just wrote, or chasing down a flake or other issue introduced in a script that was not recently touched. I guess we could record the trash directory state for every failing test within a script. That would need some cooperation from test-lib.sh, but probably wouldn't be too hard. -Peff
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Thu, 8 Aug 2024, Junio C Hamano wrote: > >> diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh >> index 88c524f655..48a1550371 100755 >> --- c/t/t1001-read-tree-m-2way.sh >> +++ w/t/t1001-read-tree-m-2way.sh >> @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' ' >> >> test_expect_success 'a/b vs a, plus c/d case test.' ' >> read_tree_u_must_succeed -u -m "$treeH" "$treeM" && >> - git ls-files --stage | tee >treeMcheck.out && >> + git ls-files --stage >treeMcheck.out && > > While this obviously fixes the bug where the test case was incorrectly > allowed to continue after a failing `git ls-files --stage` call, I will > note that I interpret the intention of the `| tee` as showing the output > in the logs in addition to redirecting it to a file for the benefit of > additional checks in the same test case. If we really want to do that, we'd do git ls-files --stage >treeMcheck.out && cat treeMcheck.out && instead. You wouldn't lose the exit status, and the output would show the contents just like "tee" would. We however tend to also remove a leftover debugging "cat", so the "cat" is likely to be removed during such a clean-up. > So, what to do here? I don't really know. The easiest option that most > other people would likely be happy with would be to go with the `| tee` > dropping. As long as we treat the CI as a "batch" environment, the postmortem tarball of the trash directory is probably the best we can do I can think of.
diff --git c/t/t1001-read-tree-m-2way.sh w/t/t1001-read-tree-m-2way.sh index 88c524f655..48a1550371 100755 --- c/t/t1001-read-tree-m-2way.sh +++ w/t/t1001-read-tree-m-2way.sh @@ -397,7 +397,7 @@ test_expect_success 'a/b vs a, plus c/d case setup.' ' test_expect_success 'a/b vs a, plus c/d case test.' ' read_tree_u_must_succeed -u -m "$treeH" "$treeM" && - git ls-files --stage | tee >treeMcheck.out && + git ls-files --stage >treeMcheck.out && test_cmp treeM.out treeMcheck.out ' diff --git c/t/t5523-push-upstream.sh w/t/t5523-push-upstream.sh index 1f859ade16..4ad36a31e1 100755 --- c/t/t5523-push-upstream.sh +++ w/t/t5523-push-upstream.sh @@ -124,14 +124,14 @@ test_expect_success TTY 'push --no-progress suppresses progress' ' test_expect_success TTY 'quiet push' ' ensure_fresh_upstream && - test_terminal git push --quiet --no-progress upstream main 2>&1 | tee output && + test_terminal git push --quiet --no-progress upstream main >output 2>&1 && test_must_be_empty output ' test_expect_success TTY 'quiet push -u' ' ensure_fresh_upstream && - test_terminal git push --quiet -u --no-progress upstream main 2>&1 | tee output && + test_terminal git push --quiet -u --no-progress upstream main >output 2>&1 && test_must_be_empty output '
A few tests have "| tee output" downstream of a git command, and then inspect the contents of the file. The net effect is that we use an extra process, and hide the exit status from the upstream git command. In none of these tests, I do not see a reason why we want to hide a possible failure from these git commands. Replace the use of tee with a plain simple redirection. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t1001-read-tree-m-2way.sh | 2 +- t/t5523-push-upstream.sh | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)