| Message ID | 20210209214159.22815-8-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | test-lib: misc improvements | expand |
On Tue, Feb 09, 2021 at 10:41:54PM +0100, Ævar Arnfjörð Bjarmason wrote: > Move a function added to test-lib-functions.sh in ea047a8eb4f (t5310: > factor out bitmap traversal comparison, 2020-02-14) into a new > lib-bitmap.sh. > > The test-lib-functions.sh file should be for functions that are widely > used across the test suite, if something's only used by a few tests it > makes more sense to have it in a lib-*.sh file. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/lib-bitmap.sh | 26 ++++++++++++++++++++++++++ > t/t5310-pack-bitmaps.sh | 2 ++ > t/t6113-rev-list-bitmap-filters.sh | 1 + > t/test-lib-functions.sh | 27 --------------------------- > 4 files changed, 29 insertions(+), 27 deletions(-) > create mode 100644 t/lib-bitmap.sh > > diff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh > new file mode 100644 > index 00000000000..fe3f98be24f > --- /dev/null > +++ b/t/lib-bitmap.sh > @@ -0,0 +1,26 @@ > +# Compare a file containing rev-list bitmap traversal output to its non-bitmap > +# counterpart. You can't just use test_cmp for this, because the two produce > +# subtly different output: > +# > +# - regular output is in traversal order, whereas bitmap is split by type, > +# with non-packed objects at the end > +# > +# - regular output has a space and the pathname appended to non-commit > +# objects; bitmap output omits this > +# > +# This function normalizes and compares the two. The second file should > +# always be the bitmap output. > +test_bitmap_traversal () { > + if test "$1" = "--no-confirm-bitmaps" > + then > + shift > + elif cmp "$1" "$2" > + then > + echo >&2 "identical raw outputs; are you sure bitmaps were used?" > + return 1 Since you converted two 'error's to BUG earlier in this series... This helper function was added in ea047a8eb4 (t5310: factor out bitmap traversal comparison, 2020-02-14), and so I Cc: its author and quote part of its commit message: While we're at it, let's also try to confirm that the bitmap output did indeed use bitmaps. Since the code internally falls back to the non-bitmap path in some cases, the tests are at risk of becoming trivial noops. This is a bit fragile, as not all outputs will differ (e.g., looking at only the commits from a fully-bitmapped pack will end up exactly the same as the normal traversal order, since it also matches the pack order). So we'll provide an escape hatch by which tests can disable this check (which should only be used after manually confirming that bitmaps kicked in). Shouldn't this be a BUG as well? I think this should be a BUG. > + fi && > + cut -d' ' -f1 "$1" | sort >"$1.normalized" && > + sort "$2" >"$2.normalized" && > + test_cmp "$1.normalized" "$2.normalized" && > + rm -f "$1.normalized" "$2.normalized" > +} > diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh > index 5ba76031090..40b9f632441 100755 > --- a/t/t5310-pack-bitmaps.sh > +++ b/t/t5310-pack-bitmaps.sh > @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-bundle.sh > +. "$TEST_DIRECTORY"/lib-bitmap.sh > > objpath () { > echo ".git/objects/$(echo "$1" | sed -e 's|\(..\)|\1/|')" > diff --git a/t/t6113-rev-list-bitmap-filters.sh b/t/t6113-rev-list-bitmap-filters.sh > index 2b551e6fd0c..3f889949ca1 100755 > --- a/t/t6113-rev-list-bitmap-filters.sh > +++ b/t/t6113-rev-list-bitmap-filters.sh > @@ -2,6 +2,7 @@ > > test_description='rev-list combining bitmaps and filters' > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-bitmap.sh > > test_expect_success 'set up bitmapped repo' ' > # one commit will have bitmaps, the other will not > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 55d58b4a6ac..f228647c2b8 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -1602,33 +1602,6 @@ test_set_port () { > eval $var=$port > } > > -# Compare a file containing rev-list bitmap traversal output to its non-bitmap > -# counterpart. You can't just use test_cmp for this, because the two produce > -# subtly different output: > -# > -# - regular output is in traversal order, whereas bitmap is split by type, > -# with non-packed objects at the end > -# > -# - regular output has a space and the pathname appended to non-commit > -# objects; bitmap output omits this > -# > -# This function normalizes and compares the two. The second file should > -# always be the bitmap output. > -test_bitmap_traversal () { > - if test "$1" = "--no-confirm-bitmaps" > - then > - shift > - elif cmp "$1" "$2" > - then > - echo >&2 "identical raw outputs; are you sure bitmaps were used?" > - return 1 > - fi && > - cut -d' ' -f1 "$1" | sort >"$1.normalized" && > - sort "$2" >"$2.normalized" && > - test_cmp "$1.normalized" "$2.normalized" && > - rm -f "$1.normalized" "$2.normalized" > -} > - > # Tests for the hidden file attribute on Windows > test_path_is_hidden () { > test_have_prereq MINGW || > -- > 2.30.0.284.gd98b1dd5eaa7 >
On Wed, Feb 10, 2021 at 09:56:45PM +0100, SZEDER Gábor wrote: > > +test_bitmap_traversal () { > > + if test "$1" = "--no-confirm-bitmaps" > > + then > > + shift > > + elif cmp "$1" "$2" > > + then > > + echo >&2 "identical raw outputs; are you sure bitmaps were used?" > > + return 1 > > Since you converted two 'error's to BUG earlier in this series... > > This helper function was added in ea047a8eb4 (t5310: factor out bitmap > traversal comparison, 2020-02-14), and so I Cc: its author and quote > part of its commit message: > > While we're at it, let's also try to confirm that the bitmap output did > indeed use bitmaps. Since the code internally falls back to the > non-bitmap path in some cases, the tests are at risk of becoming trivial > noops. > > This is a bit fragile, as not all outputs will differ (e.g., looking at > only the commits from a fully-bitmapped pack will end up exactly the > same as the normal traversal order, since it also matches the pack > order). So we'll provide an escape hatch by which tests can disable this > check (which should only be used after manually confirming that bitmaps > kicked in). > > Shouldn't this be a BUG as well? I think this should be a BUG. I dunno. I guess you are thinking of the case where somebody adds a new caller but fails to invoke Git correctly. Which would be a bug in the test script. But it could also easily be due to Git changing how it behaves in a certain circumstance. And maybe the solution is changing the test to adapt, or maybe we just found a bug in Git. TBH, I do not care that much either way. Either way, I think somebody would notice the problem. -Peff
On Wed, Feb 10, 2021 at 04:10:58PM -0500, Jeff King wrote: > On Wed, Feb 10, 2021 at 09:56:45PM +0100, SZEDER Gábor wrote: > > > > +test_bitmap_traversal () { > > > + if test "$1" = "--no-confirm-bitmaps" > > > + then > > > + shift > > > + elif cmp "$1" "$2" > > > + then > > > + echo >&2 "identical raw outputs; are you sure bitmaps were used?" > > > + return 1 > > > > Since you converted two 'error's to BUG earlier in this series... > > > > This helper function was added in ea047a8eb4 (t5310: factor out bitmap > > traversal comparison, 2020-02-14), and so I Cc: its author and quote > > part of its commit message: > > > > While we're at it, let's also try to confirm that the bitmap output did > > indeed use bitmaps. Since the code internally falls back to the > > non-bitmap path in some cases, the tests are at risk of becoming trivial > > noops. > > > > This is a bit fragile, as not all outputs will differ (e.g., looking at > > only the commits from a fully-bitmapped pack will end up exactly the > > same as the normal traversal order, since it also matches the pack > > order). So we'll provide an escape hatch by which tests can disable this > > check (which should only be used after manually confirming that bitmaps > > kicked in). > > > > Shouldn't this be a BUG as well? I think this should be a BUG. > > I dunno. I guess you are thinking of the case where somebody adds a new > caller but fails to invoke Git correctly. Which would be a bug in the > test script. > > But it could also easily be due to Git changing how it behaves in a > certain circumstance. And maybe the solution is changing the test to > adapt, or maybe we just found a bug in Git. Well, I though that if the raw output is the same with and without pack bitmaps, then the missing '--no-confirm-bitmaps' option is the bug in the test script. > TBH, I do not care that much either way. Either way, I think somebody > would notice the problem. Me neither, I just don't like test helper functions that simply return in the middle, becauase that makes suppressing their trace output even more awkward than it already is :)
diff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh new file mode 100644 index 00000000000..fe3f98be24f --- /dev/null +++ b/t/lib-bitmap.sh @@ -0,0 +1,26 @@ +# Compare a file containing rev-list bitmap traversal output to its non-bitmap +# counterpart. You can't just use test_cmp for this, because the two produce +# subtly different output: +# +# - regular output is in traversal order, whereas bitmap is split by type, +# with non-packed objects at the end +# +# - regular output has a space and the pathname appended to non-commit +# objects; bitmap output omits this +# +# This function normalizes and compares the two. The second file should +# always be the bitmap output. +test_bitmap_traversal () { + if test "$1" = "--no-confirm-bitmaps" + then + shift + elif cmp "$1" "$2" + then + echo >&2 "identical raw outputs; are you sure bitmaps were used?" + return 1 + fi && + cut -d' ' -f1 "$1" | sort >"$1.normalized" && + sort "$2" >"$2.normalized" && + test_cmp "$1.normalized" "$2.normalized" && + rm -f "$1.normalized" "$2.normalized" +} diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index 5ba76031090..40b9f632441 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-bundle.sh +. "$TEST_DIRECTORY"/lib-bitmap.sh objpath () { echo ".git/objects/$(echo "$1" | sed -e 's|\(..\)|\1/|')" diff --git a/t/t6113-rev-list-bitmap-filters.sh b/t/t6113-rev-list-bitmap-filters.sh index 2b551e6fd0c..3f889949ca1 100755 --- a/t/t6113-rev-list-bitmap-filters.sh +++ b/t/t6113-rev-list-bitmap-filters.sh @@ -2,6 +2,7 @@ test_description='rev-list combining bitmaps and filters' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-bitmap.sh test_expect_success 'set up bitmapped repo' ' # one commit will have bitmaps, the other will not diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 55d58b4a6ac..f228647c2b8 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1602,33 +1602,6 @@ test_set_port () { eval $var=$port } -# Compare a file containing rev-list bitmap traversal output to its non-bitmap -# counterpart. You can't just use test_cmp for this, because the two produce -# subtly different output: -# -# - regular output is in traversal order, whereas bitmap is split by type, -# with non-packed objects at the end -# -# - regular output has a space and the pathname appended to non-commit -# objects; bitmap output omits this -# -# This function normalizes and compares the two. The second file should -# always be the bitmap output. -test_bitmap_traversal () { - if test "$1" = "--no-confirm-bitmaps" - then - shift - elif cmp "$1" "$2" - then - echo >&2 "identical raw outputs; are you sure bitmaps were used?" - return 1 - fi && - cut -d' ' -f1 "$1" | sort >"$1.normalized" && - sort "$2" >"$2.normalized" && - test_cmp "$1.normalized" "$2.normalized" && - rm -f "$1.normalized" "$2.normalized" -} - # Tests for the hidden file attribute on Windows test_path_is_hidden () { test_have_prereq MINGW ||
Move a function added to test-lib-functions.sh in ea047a8eb4f (t5310: factor out bitmap traversal comparison, 2020-02-14) into a new lib-bitmap.sh. The test-lib-functions.sh file should be for functions that are widely used across the test suite, if something's only used by a few tests it makes more sense to have it in a lib-*.sh file. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/lib-bitmap.sh | 26 ++++++++++++++++++++++++++ t/t5310-pack-bitmaps.sh | 2 ++ t/t6113-rev-list-bitmap-filters.sh | 1 + t/test-lib-functions.sh | 27 --------------------------- 4 files changed, 29 insertions(+), 27 deletions(-) create mode 100644 t/lib-bitmap.sh