| Message ID | 20200731174509.9199-1-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] t6300: fix issues related to %(contents:size) | expand |
On Fri, Jul 31, 2020 at 07:45:09PM +0200, Alban Gruin wrote: > b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16) > added a new format for ref-filter, and added a function to generate > tests for this new feature in t6300. Unfortunately, it tries to run > `test_expect_sucess' instead of `test_expect_success', and writes > $expect to `expected', but tries to read `expect'. Those two issues > were probably unnoticed because the script only printed errors, but did > not crash. This fixes these issues. Oh, this just crossed with my mail. :) Definitely fixes the issue, though I wonder: > - echo $expect >expected > - test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" ' > + echo $expect >expect > + test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" ' > git for-each-ref --format="%(contents:size)" "$ref" >actual && > test_cmp expect actual > ' Should we instead switch the test_cmp to look at "expected" to be consistent with the rest of the tests in this file? -Peff
Le 31/07/2020 à 19:47, Jeff King a écrit : > On Fri, Jul 31, 2020 at 07:45:09PM +0200, Alban Gruin wrote: > >> b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16) >> added a new format for ref-filter, and added a function to generate >> tests for this new feature in t6300. Unfortunately, it tries to run >> `test_expect_sucess' instead of `test_expect_success', and writes >> $expect to `expected', but tries to read `expect'. Those two issues >> were probably unnoticed because the script only printed errors, but did >> not crash. This fixes these issues. > > Oh, this just crossed with my mail. :) > > Definitely fixes the issue, though I wonder: > >> - echo $expect >expected >> - test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" ' >> + echo $expect >expect >> + test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" ' >> git for-each-ref --format="%(contents:size)" "$ref" >actual && >> test_cmp expect actual >> ' > > Should we instead switch the test_cmp to look at "expected" to be > consistent with the rest of the tests in this file? > > -Peff > OK, I'm fixing that. Alban
Jeff King <peff@peff.net> writes: > On Fri, Jul 31, 2020 at 07:45:09PM +0200, Alban Gruin wrote: > >> b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16) >> added a new format for ref-filter, and added a function to generate >> tests for this new feature in t6300. Unfortunately, it tries to run >> `test_expect_sucess' instead of `test_expect_success', and writes >> $expect to `expected', but tries to read `expect'. Those two issues >> were probably unnoticed because the script only printed errors, but did >> not crash. This fixes these issues. > > Oh, this just crossed with my mail. :) > > Definitely fixes the issue, though I wonder: > >> - echo $expect >expected >> - test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" ' >> + echo $expect >expect >> + test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" ' >> git for-each-ref --format="%(contents:size)" "$ref" >actual && >> test_cmp expect actual >> ' > > Should we instead switch the test_cmp to look at "expected" to be > consistent with the rest of the tests in this file? If I recall correctly, "expect vs actual" were more common when I counted across all the tests last time. Matching local convention is fine, though.
On Fri, Jul 31, 2020 at 01:04:10PM -0700, Junio C Hamano wrote: > > Definitely fixes the issue, though I wonder: > > > >> - echo $expect >expected > >> - test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" ' > >> + echo $expect >expect > >> + test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" ' > >> git for-each-ref --format="%(contents:size)" "$ref" >actual && > >> test_cmp expect actual > >> ' > > > > Should we instead switch the test_cmp to look at "expected" to be > > consistent with the rest of the tests in this file? > > If I recall correctly, "expect vs actual" were more common when I > counted across all the tests last time. Matching local convention > is fine, though. Yes, I agree that "expect" is where we should be heading overall. I think matching local convention is best here to avoid introducing new mistakes like this one, but I wouldn't be opposed to somebody switching out s/expected/expect/ in the whole file. -Peff
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index ea9bb6dade..bbec555977 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -65,8 +65,8 @@ test_atom() { expect=$(printf '%s' "$3" | wc -c) ;; esac # Leave $expect unquoted to lose possible leading whitespaces - echo $expect >expected - test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" ' + echo $expect >expect + test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" ' git for-each-ref --format="%(contents:size)" "$ref" >actual && test_cmp expect actual '
b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16) added a new format for ref-filter, and added a function to generate tests for this new feature in t6300. Unfortunately, it tries to run `test_expect_sucess' instead of `test_expect_success', and writes $expect to `expected', but tries to read `expect'. Those two issues were probably unnoticed because the script only printed errors, but did not crash. This fixes these issues. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- t/t6300-for-each-ref.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)