| Message ID | 20220402192415.19023-2-khalid.masum.92@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t3501: remove redundant file checking and stop ignoring git <cmd> exit code | expand |
Labnann <khalid.masum.92@gmail.com> writes: > In the test 'cherry-pick after renaming branch', stop checking for > the presence of a file (opos) because we are going to "grep" in it in > the same test and the lack of it will be noticed as a failure anyway. OK. > In the test 'revert after renaming branch', instead of allowing any > random contents as long as a known phrase is not there in it, we can > expect the exact outcome---after the successful revert of > "added", the contents of file "spoo" should become identical to > what was in file "oops" in the "initial" commit. Makes sense. The code removes "test -f spoo", which is not explained in the above, and I do not think we want to. We'd notice a breakage where revert leaves the right result in HEAD but fails to match the working tree files if we leave it there. > In both tests, we would not notice when "git rev-parse" starts > segfaulting without emitting any output. The 'test' command would > end up being just "test =", which yields success. Therefore we could > use test_cmp_rev The two sentences are right. The conclusion is a bit iffy; it is not "we could", which implies that the current one is OK but it is OK to also rewrite it to use test_cmp_rev. Use the 'test_cmp_rev' helper to make sure we will notice such a breakage. or something like that, perhaps? > Signed-off-by: Labnann <khalid.masum.92@gmail.com> Is Labnann your name? I do not know where you are from so forgive me if it is. We prefer people to sign with their real names (cf. Documentation/SubmittingPatches[[real-name]]) on this line (and because the name/e-mail on this line must match the author's ident, the same preference applies there, too). > --- > t/t3501-revert-cherry-pick.sh | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh > index 8617efaaf1..ad8f0cae5a 100755 > --- a/t/t3501-revert-cherry-pick.sh > +++ b/t/t3501-revert-cherry-pick.sh > @@ -66,8 +66,7 @@ test_expect_success 'cherry-pick after renaming branch' ' > > git checkout rename2 && > git cherry-pick added && > - test $(git rev-parse HEAD^) = $(git rev-parse rename2) && > - test -f opos && > + test_cmp_rev rename2 HEAD^ && > grep "Add extra line at the end" opos && > git reflog -1 | grep cherry-pick > > @@ -77,9 +76,8 @@ test_expect_success 'revert after renaming branch' ' > > git checkout rename1 && > git revert added && > - test $(git rev-parse HEAD^) = $(git rev-parse rename1) && > - test -f spoo && > - ! grep "Add extra line at the end" spoo && > + test_cmp_rev rename1 HEAD^ && > + test_cmp_rev initial:oops HEAD:spoo && > git reflog -1 | grep revert > > ' The diff looks almost perfect here (modulo that "test -f spoo" is better kept, I would think). Thanks.
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 8617efaaf1..ad8f0cae5a 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -66,8 +66,7 @@ test_expect_success 'cherry-pick after renaming branch' ' git checkout rename2 && git cherry-pick added && - test $(git rev-parse HEAD^) = $(git rev-parse rename2) && - test -f opos && + test_cmp_rev rename2 HEAD^ && grep "Add extra line at the end" opos && git reflog -1 | grep cherry-pick @@ -77,9 +76,8 @@ test_expect_success 'revert after renaming branch' ' git checkout rename1 && git revert added && - test $(git rev-parse HEAD^) = $(git rev-parse rename1) && - test -f spoo && - ! grep "Add extra line at the end" spoo && + test_cmp_rev rename1 HEAD^ && + test_cmp_rev initial:oops HEAD:spoo && git reflog -1 | grep revert '
In the test 'cherry-pick after renaming branch', stop checking for the presence of a file (opos) because we are going to "grep" in it in the same test and the lack of it will be noticed as a failure anyway. In the test 'revert after renaming branch', instead of allowing any random contents as long as a known phrase is not there in it, we can expect the exact outcome---after the successful revert of "added", the contents of file "spoo" should become identical to what was in file "oops" in the "initial" commit. In both tests, we would not notice when "git rev-parse" starts segfaulting without emitting any output. The 'test' command would end up being just "test =", which yields success. Therefore we could use test_cmp_rev Signed-off-by: Labnann <khalid.masum.92@gmail.com> --- t/t3501-revert-cherry-pick.sh | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)