| Message ID | 0d0d50d14c557f8313747e7d0e104c2c0819dab9.1682593865.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: introduce machine-parseable output | expand |
On Thu, Apr 27, 2023 at 01:13:08PM +0200, Patrick Steinhardt wrote: > We're about to introduce a new porcelain mode for the output of > git-fetch(1). As part of that we'll be introducing a set of new tests > that only relate to the output of this command. > > Split out tests that exercise the output format of git-fetch(1) so that > it becomes easier to verify this functionality as a standalone unit. As > the tests assume that the default branch is called "main" we set up the > corresponding GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME environment variable > accordingly. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > t/t5510-fetch.sh | 53 ---------------------------------- > t/t5574-fetch-output.sh | 63 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 63 insertions(+), 53 deletions(-) > create mode 100755 t/t5574-fetch-output.sh > diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh > new file mode 100755 > index 0000000000..f91b654d38 > --- /dev/null > +++ b/t/t5574-fetch-output.sh > @@ -0,0 +1,63 @@ > +#!/bin/sh > + > +test_description='git fetch output format' > + > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > + > +. ./test-lib.sh > + > +test_expect_success 'fetch aligned output' ' > + git clone . full-output && > + test_commit looooooooooooong-tag && > + ( > + cd full-output && > + git -c fetch.output=full fetch origin >actual 2>&1 && Why is that 2>&1 redirection used here? If the output the test case is interested in goes to the command's standard output, then it's unnecessary. However, if it goes to standard error, then why is standard output redirected as well? I understand that this patch just moves existing test cases around as-is, so this is not something you introduced, but I point it out here, because later patches of this series add several new test cases following this anti-pattern. Since this series creates a new test script, perhaps this might be the right time to clean this up. > + grep -e "->" actual | cut -c 22- >../actual > + ) && > + cat >expect <<-\EOF && > + main -> origin/main > + looooooooooooong-tag -> looooooooooooong-tag > + EOF > + test_cmp expect actual > +' > + > +test_expect_success 'fetch compact output' ' > + git clone . compact && > + test_commit extraaa && > + ( > + cd compact && > + git -c fetch.output=compact fetch origin >actual 2>&1 && > + grep -e "->" actual | cut -c 22- >../actual > + ) && > + cat >expect <<-\EOF && > + main -> origin/* > + extraaa -> * > + EOF > + test_cmp expect actual > +' > + > +test_expect_success '--no-show-forced-updates' ' > + mkdir forced-updates && > + ( > + cd forced-updates && > + git init && > + test_commit 1 && > + test_commit 2 > + ) && > + git clone forced-updates forced-update-clone && > + git clone forced-updates no-forced-update-clone && > + git -C forced-updates reset --hard HEAD~1 && > + ( > + cd forced-update-clone && > + git fetch --show-forced-updates origin 2>output && Oh, look, there are some good examples to follow here as well :) > + test_i18ngrep "(forced update)" output > + ) && > + ( > + cd no-forced-update-clone && > + git fetch --no-show-forced-updates origin 2>output && > + test_i18ngrep ! "(forced update)" output > + ) > +' > + > +test_done > -- > 2.40.1 >
On Sat, Apr 29, 2023 at 07:34:48PM +0200, SZEDER Gábor wrote: > On Thu, Apr 27, 2023 at 01:13:08PM +0200, Patrick Steinhardt wrote: > > We're about to introduce a new porcelain mode for the output of > > git-fetch(1). As part of that we'll be introducing a set of new tests > > that only relate to the output of this command. > > > > Split out tests that exercise the output format of git-fetch(1) so that > > it becomes easier to verify this functionality as a standalone unit. As > > the tests assume that the default branch is called "main" we set up the > > corresponding GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME environment variable > > accordingly. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > t/t5510-fetch.sh | 53 ---------------------------------- > > t/t5574-fetch-output.sh | 63 +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 63 insertions(+), 53 deletions(-) > > create mode 100755 t/t5574-fetch-output.sh > > > > diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh > > new file mode 100755 > > index 0000000000..f91b654d38 > > --- /dev/null > > +++ b/t/t5574-fetch-output.sh > > @@ -0,0 +1,63 @@ > > +#!/bin/sh > > + > > +test_description='git fetch output format' > > + > > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > + > > +. ./test-lib.sh > > + > > +test_expect_success 'fetch aligned output' ' > > + git clone . full-output && > > + test_commit looooooooooooong-tag && > > + ( > > + cd full-output && > > + git -c fetch.output=full fetch origin >actual 2>&1 && > > Why is that 2>&1 redirection used here? > If the output the test case is interested in goes to the command's > standard output, then it's unnecessary. However, if it goes to > standard error, then why is standard output redirected as well? > > I understand that this patch just moves existing test cases around > as-is, so this is not something you introduced, but I point it out > here, because later patches of this series add several new test cases > following this anti-pattern. > > Since this series creates a new test script, perhaps this might be the > right time to clean this up. I feel like this patch series is big enough on its own already, so for now I'd like to refrain from touching up the old tests. But you've got a good point here, and I'll make sure that any new tests don't followw the same pattern. That being said, I think we should keep on testing both stdout and stderr. The tests here are explicitly about the output format, so it does make sense to verify that both of them match what we expect. We should treat them as separate things though. Patrick
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index dc44da9c79..4f289063ce 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -1118,59 +1118,6 @@ test_expect_success 'fetching with auto-gc does not lock up' ' ) ' -test_expect_success 'fetch aligned output' ' - git clone . full-output && - test_commit looooooooooooong-tag && - ( - cd full-output && - git -c fetch.output=full fetch origin >actual 2>&1 && - grep -e "->" actual | cut -c 22- >../actual - ) && - cat >expect <<-\EOF && - main -> origin/main - looooooooooooong-tag -> looooooooooooong-tag - EOF - test_cmp expect actual -' - -test_expect_success 'fetch compact output' ' - git clone . compact && - test_commit extraaa && - ( - cd compact && - git -c fetch.output=compact fetch origin >actual 2>&1 && - grep -e "->" actual | cut -c 22- >../actual - ) && - cat >expect <<-\EOF && - main -> origin/* - extraaa -> * - EOF - test_cmp expect actual -' - -test_expect_success '--no-show-forced-updates' ' - mkdir forced-updates && - ( - cd forced-updates && - git init && - test_commit 1 && - test_commit 2 - ) && - git clone forced-updates forced-update-clone && - git clone forced-updates no-forced-update-clone && - git -C forced-updates reset --hard HEAD~1 && - ( - cd forced-update-clone && - git fetch --show-forced-updates origin 2>output && - test_i18ngrep "(forced update)" output - ) && - ( - cd no-forced-update-clone && - git fetch --no-show-forced-updates origin 2>output && - test_i18ngrep ! "(forced update)" output - ) -' - for section in fetch transfer do test_expect_success "$section.hideRefs affects connectivity check" ' diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh new file mode 100755 index 0000000000..f91b654d38 --- /dev/null +++ b/t/t5574-fetch-output.sh @@ -0,0 +1,63 @@ +#!/bin/sh + +test_description='git fetch output format' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + +test_expect_success 'fetch aligned output' ' + git clone . full-output && + test_commit looooooooooooong-tag && + ( + cd full-output && + git -c fetch.output=full fetch origin >actual 2>&1 && + grep -e "->" actual | cut -c 22- >../actual + ) && + cat >expect <<-\EOF && + main -> origin/main + looooooooooooong-tag -> looooooooooooong-tag + EOF + test_cmp expect actual +' + +test_expect_success 'fetch compact output' ' + git clone . compact && + test_commit extraaa && + ( + cd compact && + git -c fetch.output=compact fetch origin >actual 2>&1 && + grep -e "->" actual | cut -c 22- >../actual + ) && + cat >expect <<-\EOF && + main -> origin/* + extraaa -> * + EOF + test_cmp expect actual +' + +test_expect_success '--no-show-forced-updates' ' + mkdir forced-updates && + ( + cd forced-updates && + git init && + test_commit 1 && + test_commit 2 + ) && + git clone forced-updates forced-update-clone && + git clone forced-updates no-forced-update-clone && + git -C forced-updates reset --hard HEAD~1 && + ( + cd forced-update-clone && + git fetch --show-forced-updates origin 2>output && + test_i18ngrep "(forced update)" output + ) && + ( + cd no-forced-update-clone && + git fetch --no-show-forced-updates origin 2>output && + test_i18ngrep ! "(forced update)" output + ) +' + +test_done
We're about to introduce a new porcelain mode for the output of git-fetch(1). As part of that we'll be introducing a set of new tests that only relate to the output of this command. Split out tests that exercise the output format of git-fetch(1) so that it becomes easier to verify this functionality as a standalone unit. As the tests assume that the default branch is called "main" we set up the corresponding GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME environment variable accordingly. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t5510-fetch.sh | 53 ---------------------------------- t/t5574-fetch-output.sh | 63 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 53 deletions(-) create mode 100755 t/t5574-fetch-output.sh