Message ID | patch-v2-1.1-13c26e546f6-20220614T153746Z-avarab@gmail.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 7ccbea564e8589a911d521a78b0eaefacc16badf |
Headers | show |
Series | [v2] add -i tests: mark "TODO" depending on GIT_TEST_ADD_I_USE_BUILTIN | expand |
Ævar Arnfjörð Bjarmason wrote: > Fix an issue that existed before 0527ccb1b55 (add -i: default to the > built-in implementation, 2021-11-30), but which became the default > with that change, we should not be marking tests that are known to > pass as "TODO" tests. [...] > --- > Just converting it to "test_expect_success" will break CI and other > setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false. > > The below fixes it, however. Nice catch. FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0 and without. > diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh > index bc3f69b4b1d..a5822e41af2 100755 > --- a/t/t2016-checkout-patch.sh > +++ b/t/t2016-checkout-patch.sh > @@ -4,7 +4,7 @@ test_description='git checkout --patch' > > . ./lib-patch-mode.sh > > -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL > +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL > then > skip_all='skipping interactive add tests, PERL not set' It's not the fault of this patch, but it makes it obvious that the `skip_all` message is no longer accurate. Perhaps somethine like this? skip_all='skipping interactive add tests, missing ADD_I_USE_BUILTIN or PERL' Maybe a separate `ADD_I` prereq would be better? Though without looking closer, I don't know if that would end up being clearer to anyone running the tests without either PERL or the add -i builtin enabled. Thanks for the keen eye and attention to detail, Ævar,
On Tue, Jun 14 2022, Todd Zullinger wrote: > Ævar Arnfjörð Bjarmason wrote: >> Fix an issue that existed before 0527ccb1b55 (add -i: default to the >> built-in implementation, 2021-11-30), but which became the default >> with that change, we should not be marking tests that are known to >> pass as "TODO" tests. > [...] >> --- >> Just converting it to "test_expect_success" will break CI and other >> setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false. >> >> The below fixes it, however. > > Nice catch. FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0 > and without. My patch landed on "master" as 7ccbea564e8 (add -i tests: mark "TODO" depending on GIT_TEST_ADD_I_USE_BUILTIN, 2022-06-14) so this is water under the bridge. But just to tie this loose knot I think something went wrong in your testing. If I: git checkout v2.37.0-rc0 # Apply your patch from <20220614185218.1091413-1-tmz@pobox.com> I'll consistently get a failure from: GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh Since we do fail that test with the Perl implementation, and now it's no longer a TODO test. Perhaps you used it as a parameter to "make"? I.e.: make GIT_TEST_ADD_I_USE_BUILTIN=false make test Which isn't how it works, just speculating... >> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh >> index bc3f69b4b1d..a5822e41af2 100755 >> --- a/t/t2016-checkout-patch.sh >> +++ b/t/t2016-checkout-patch.sh >> @@ -4,7 +4,7 @@ test_description='git checkout --patch' >> >> . ./lib-patch-mode.sh >> >> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL >> +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL >> then >> skip_all='skipping interactive add tests, PERL not set' > > It's not the fault of this patch, but it makes it obvious > that the `skip_all` message is no longer accurate. Perhaps > somethine like this? > > skip_all='skipping interactive add tests, missing ADD_I_USE_BUILTIN or PERL' > > Maybe a separate `ADD_I` prereq would be better? Though > without looking closer, I don't know if that would end up > being clearer to anyone running the tests without either > PERL or the add -i builtin enabled. Yeah seems like a good idea for a follow-up, but since it's landed I'll probably forget :) > Thanks for the keen eye and attention to detail, Ævar, Happy to have it fixed!
Ævar Arnfjörð Bjarmason wrote: > > On Tue, Jun 14 2022, Todd Zullinger wrote: >> Nice catch. FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0 >> and without. > > My patch landed on "master" as 7ccbea564e8 (add -i tests: mark "TODO" > depending on GIT_TEST_ADD_I_USE_BUILTIN, 2022-06-14) so this is water > under the bridge. > > But just to tie this loose knot I think something went wrong in your > testing. > > If I: > > git checkout v2.37.0-rc0 > # Apply your patch from <20220614185218.1091413-1-tmz@pobox.com> > > I'll consistently get a failure from: > > GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh > Sorry for being unclear. I meant that I tested your patch. The patch I sent didn't handle that case. :)
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh index bc3f69b4b1d..a5822e41af2 100755 --- a/t/t2016-checkout-patch.sh +++ b/t/t2016-checkout-patch.sh @@ -4,7 +4,7 @@ test_description='git checkout --patch' . ./lib-patch-mode.sh -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL then skip_all='skipping interactive add tests, PERL not set' test_done diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 94537a6b40a..fc26cb8bae8 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -538,7 +538,15 @@ test_expect_success 'split hunk "add -p (edit)"' ' ! grep "^+15" actual ' -test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' +test_expect_success 'setup ADD_I_USE_BUILTIN check' ' + result=success && + if ! test_have_prereq ADD_I_USE_BUILTIN + then + result=failure + fi +' + +test_expect_$result 'split hunk "add -p (no, yes, edit)"' ' test_write_lines 5 10 20 21 30 31 40 50 60 >test && git reset && # test sequence is s(plit), n(o), y(es), e(dit) @@ -562,7 +570,7 @@ test_expect_success 'split hunk with incomplete line at end' ' test_must_fail git grep --cached before ' -test_expect_failure 'edit, adding lines to the first hunk' ' +test_expect_$result 'edit, adding lines to the first hunk' ' test_write_lines 10 11 20 30 40 50 51 60 >test && git reset && tr _ " " >patch <<-EOF && diff --git a/t/test-lib.sh b/t/test-lib.sh index 736c6447ecf..f5291ef56ef 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1759,6 +1759,10 @@ test_lazy_prereq SHA1 ' esac ' +test_lazy_prereq ADD_I_USE_BUILTIN ' + test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true +' + # Ensure that no test accidentally triggers a Git command # that runs the actual maintenance scheduler, affecting a user's # system permanently.