| Message ID | 20210425080508.154159-1-bagasdotme@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] t6030: add test for git bisect skip started with --term* arguments | expand |
On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > Trygve Aaberge reported [1] git bisect breakage when the bisection > is started with --term* arguments (--term-new and --term-old). In that > case, skipping with `git bisect skip` should cause HEAD to be changed, > but actually the HEAD stayed same after skipping. > > Let's add the test to catch the breakage. Because there isn't any fixes > yet, mark the test as test_expect_failure. > > [1]: https://lore.kernel.org/git/20210418151459.GC10839@aaberge.net/ We prefer when people can give a proper explanation of what happens in the commit message, instead of providing a link to such an explanation. Here it's not so important if this patch (which only adds tests) is merged along with (or if it's squashed into) the fix which hopefully contains an explanation. Maybe if both patches are sent in the same patch series the commit message might want to only say something like "A previous commit fixed a `git bisect skip` breakage when the bisection is started with --term* arguments, let's add a test for that breakage." A commit message trailer might be the right way to credit Trygve Aaberge (also please don't forget to put the reporter in Cc like I am doing), for example like: Reported-by: Trygve Aaberge <trygveaa@gmail.com> > Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com> > --- > Changes from v1 [1]: > > * Move the test to the the end of test script (test 74). > v1 placed the test as test 26, and when I run the script, there > are 9 more failed tests rather than just one (because such tests > depended on previous ones). Now the test is placed together with > similar tests (git bisect with --terms*). > * Begin the test with git bisect reset, as with other nearby tests. Nice! > [1]: https://lore.kernel.org/git/20210423070308.85275-1-bagasdotme@gmail.com/ > > t/t6030-bisect-porcelain.sh | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > index 32bb66e1ed..b1b847ebbc 100755 > --- a/t/t6030-bisect-porcelain.sh > +++ b/t/t6030-bisect-porcelain.sh > @@ -922,6 +922,19 @@ test_expect_success 'bisect start takes options and revs in any order' ' > test_cmp expected actual > ' > > +# Bisect is started with --term-new and --term-old arguments, > +# then skip. The HEAD should be changed. > +# FIXME: Mark this test as test_expect_failure. Remove the FIXME and > +# mark as test_expect_success when this test successes (fixed bug). > +test_expect_failure '"bisect skip: bisection is started with --term*"' ' I am not sure why there are double quotes inside simple quotes around the name of the test. Aren't simple quotes enough? Also what about a simpler name like: test_expect_failure 'bisect skip works with --term*' ' ? > + git bisect reset && > + git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 && > + HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) && > + git bisect skip && > + HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) && > + test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO It might be a bit safer and more consistent with the rest of this test script to use double quotes around $HASH_SKIPPED_FROM and $HASH_SKIPPED_TO, like: test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO" Thanks, Christian.
On Wed, Apr 28, 2021 at 4:12 AM Christian Couder <christian.couder@gmail.com> wrote: > On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > + git bisect reset && > > + git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 && > > + HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) && > > + git bisect skip && > > + HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) && > > + test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO > > It might be a bit safer and more consistent with the rest of this test > script to use double quotes around $HASH_SKIPPED_FROM and > $HASH_SKIPPED_TO, like: > > test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO" Also, is there a reason for upcasing these variable names (HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be globals even though they are used only in this test? More appropriate and less misleading names might be `skipped_from` and `skipped_to`.
On Wed, Apr 28, 2021 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Wed, Apr 28, 2021 at 4:12 AM Christian Couder > <christian.couder@gmail.com> wrote: > > On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > > + git bisect reset && > > > + git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 && > > > + HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) && > > > + git bisect skip && > > > + HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) && > > > + test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO > > > > It might be a bit safer and more consistent with the rest of this test > > script to use double quotes around $HASH_SKIPPED_FROM and > > $HASH_SKIPPED_TO, like: > > > > test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO" > > Also, is there a reason for upcasing these variable names > (HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be > globals even though they are used only in this test? More appropriate > and less misleading names might be `skipped_from` and `skipped_to`. In this test script many variables are called HASH1, HASH2, ... or PARA_HASH1, PARA_HASH2, ... So in this regard HASH_SKIPPED_FROM and HASH_SKIPPED_TO look ok to me.
On Wed, Apr 28, 2021 at 1:17 PM Christian Couder <christian.couder@gmail.com> wrote: > On Wed, Apr 28, 2021 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Wed, Apr 28, 2021 at 4:12 AM Christian Couder > > <christian.couder@gmail.com> wrote: > > > On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote: > > > > + HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) && > > > > + HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) && > > > > + test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO > > > > > > test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO" > > > > Also, is there a reason for upcasing these variable names > > (HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be > > globals even though they are used only in this test? More appropriate > > and less misleading names might be `skipped_from` and `skipped_to`. > > In this test script many variables are called HASH1, HASH2, ... or > PARA_HASH1, PARA_HASH2, ... So in this regard HASH_SKIPPED_FROM and > HASH_SKIPPED_TO look ok to me. Many of the values of the existing uppercase variables -- such as HASH1, HASH2 -- in this script are explicitly and intentionally shared among tests, so they are necessarily global, thus the conventional uppercase names make sense. However, the variables in this test are local to it (and there is no indication that their values will ever be shared with other tests, so using uppercase for the names is misleading and confusing for readers[1]. [1]: It's true that some of the uppercase names in existing tests are local to those tests, as well, so they ought not use uppercase names, and therefore share the same problem of misleading and confusing readers. It would be nice for the naming to be cleaned up, but that's outside the scope of this patch. Admittedly, considering the existing naming inconsistency, questioning the uppercase names in this new test is a relatively minor point, but as a reader, they confuse me and make me spend extra cycles wondering why they need to be global (even though they are not, but their uppercase names mislead me into thinking they are).
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 32bb66e1ed..b1b847ebbc 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -922,6 +922,19 @@ test_expect_success 'bisect start takes options and revs in any order' ' test_cmp expected actual ' +# Bisect is started with --term-new and --term-old arguments, +# then skip. The HEAD should be changed. +# FIXME: Mark this test as test_expect_failure. Remove the FIXME and +# mark as test_expect_success when this test successes (fixed bug). +test_expect_failure '"bisect skip: bisection is started with --term*"' ' + git bisect reset && + git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 && + HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) && + git bisect skip && + HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) && + test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO +' + test_expect_success 'git bisect reset cleans bisection state properly' ' git bisect reset && git bisect start &&
Trygve Aaberge reported [1] git bisect breakage when the bisection is started with --term* arguments (--term-new and --term-old). In that case, skipping with `git bisect skip` should cause HEAD to be changed, but actually the HEAD stayed same after skipping. Let's add the test to catch the breakage. Because there isn't any fixes yet, mark the test as test_expect_failure. [1]: https://lore.kernel.org/git/20210418151459.GC10839@aaberge.net/ Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com> --- Changes from v1 [1]: * Move the test to the the end of test script (test 74). v1 placed the test as test 26, and when I run the script, there are 9 more failed tests rather than just one (because such tests depended on previous ones). Now the test is placed together with similar tests (git bisect with --terms*). * Begin the test with git bisect reset, as with other nearby tests. [1]: https://lore.kernel.org/git/20210423070308.85275-1-bagasdotme@gmail.com/ t/t6030-bisect-porcelain.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a