# [PATCH] t6026: ensure that long-running script really is 9 messages from 2016-11-09 to 2016-11-11. Participants: Johannes Schindelin, Junio C Hamano, Johannes Sixt, Jeff King. Thread: https://gitlist.dev/t/44446 ## Johannes Schindelin, 2016-11-09 13:51 Subject: [PATCH] t6026: ensure that long-running script really is Message-ID: <16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin%40gmx.de ``` When making sure that background tasks are cleaned up in 5babb5b (t6026-merge-attr: clean up background process at end of test case, 2016-09-07), we considered to let the background task sleep longer, just to be certain that it will still be running when we want to kill it after the test. Sadly, the assumption appears not to hold true that the test case passes quickly enough to kill the background task within a second. Simply increase it to an hour. No system can be possibly slow enough to make above-mentioned assumption incorrect. Reported by Andreas Schwab. Signed-off-by: Johannes Schindelin --- Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1 Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1 t/t6026-merge-attr.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh index 7a6e33e..348d78b 100755 --- a/t/t6026-merge-attr.sh +++ b/t/t6026-merge-attr.sh @@ -183,16 +183,16 @@ test_expect_success 'up-to-date merge without common ancestor' ' test_expect_success 'custom merge does not lock index' ' git reset --hard anchor && - write_script sleep-one-second.sh <<-\EOF && - sleep 1 & + write_script sleep-an-hour.sh <<-\EOF && + sleep 3600 & echo $! >sleep.pid EOF test_when_finished "kill \$(cat sleep.pid)" && test_write_lines >.gitattributes \ - "* merge=ours" "text merge=sleep-one-second" && + "* merge=ours" "text merge=sleep-an-hour" && test_config merge.ours.driver true && - test_config merge.sleep-one-second.driver ./sleep-one-second.sh && + test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh && git merge master ' base-commit: be5a750939c212bc0781ffa04fabcfd2b2bd744e -- 2.10.1.583.g721a9e0 ``` ## Junio C Hamano, 2016-11-10 23:53 Subject: Re: [PATCH] t6026: ensure that long-running script really is Message-ID: URL: https://gitlist.dev/e/xmqqfumy51tk.fsf%40gitster.mtv.corp.google.com In-Reply-To: <16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de> ``` Johannes Schindelin writes: > When making sure that background tasks are cleaned up in 5babb5b > (t6026-merge-attr: clean up background process at end of test case, > 2016-09-07), we considered to let the background task sleep longer, just > to be certain that it will still be running when we want to kill it > after the test. > > Sadly, the assumption appears not to hold true that the test case passes > quickly enough to kill the background task within a second. > > Simply increase it to an hour. No system can be possibly slow enough to > make above-mentioned assumption incorrect. > > Reported by Andreas Schwab. > > Signed-off-by: Johannes Schindelin > --- > Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1 > Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1 OK, I think this is a much better option. Assuming 3600 is long enough for everybody (and if not, we have a bigger problem ;-), it will ensure that the stray process will be around when we run the 'git merge' test, and by not adding "|| :" after the kill, we check that the stray process is still there, i.e. we tested what we wanted to test. Will revert the two patches that were queued previously and then queue this one. Thanks. > > t/t6026-merge-attr.sh | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh > index 7a6e33e..348d78b 100755 > --- a/t/t6026-merge-attr.sh > +++ b/t/t6026-merge-attr.sh > @@ -183,16 +183,16 @@ test_expect_success 'up-to-date merge without common ancestor' ' > > test_expect_success 'custom merge does not lock index' ' > git reset --hard anchor && > - write_script sleep-one-second.sh <<-\EOF && > - sleep 1 & > + write_script sleep-an-hour.sh <<-\EOF && > + sleep 3600 & > echo $! >sleep.pid > EOF > test_when_finished "kill \$(cat sleep.pid)" && > > test_write_lines >.gitattributes \ > - "* merge=ours" "text merge=sleep-one-second" && > + "* merge=ours" "text merge=sleep-an-hour" && > test_config merge.ours.driver true && > - test_config merge.sleep-one-second.driver ./sleep-one-second.sh && > + test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh && > git merge master > ' > > > base-commit: be5a750939c212bc0781ffa04fabcfd2b2bd744e ``` ## Johannes Sixt, 2016-11-11 06:50 Subject: Re: [PATCH] t6026: ensure that long-running script really is Message-ID: URL: https://gitlist.dev/e/fbf517ad-7341-eb6d-ab38-4fe91410e57c%40kdbg.org In-Reply-To: ``` Am 11.11.2016 um 00:53 schrieb Junio C Hamano: > Johannes Schindelin writes: > >> When making sure that background tasks are cleaned up in 5babb5b >> (t6026-merge-attr: clean up background process at end of test case, >> 2016-09-07), we considered to let the background task sleep longer, just >> to be certain that it will still be running when we want to kill it >> after the test. >> >> Sadly, the assumption appears not to hold true that the test case passes >> quickly enough to kill the background task within a second. >> >> Simply increase it to an hour. No system can be possibly slow enough to >> make above-mentioned assumption incorrect. >> >> Reported by Andreas Schwab. >> >> Signed-off-by: Johannes Schindelin >> --- >> Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1 >> Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1 > > OK, I think this is a much better option. Assuming 3600 is long > enough for everybody (and if not, we have a bigger problem ;-), > it will ensure that the stray process will be around when we run the > 'git merge' test, and by not adding "|| :" after the kill, we check > that the stray process is still there, i.e. we tested what we wanted > to test. > > Will revert the two patches that were queued previously and then > queue this one. Shouldn't we then move the 'kill' out of test_when_finished and make it a proper condition of the test? Like this? diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh index 348d78b205..6ad8bd098a 100755 --- a/t/t6026-merge-attr.sh +++ b/t/t6026-merge-attr.sh @@ -187,13 +187,19 @@ test_expect_success 'custom merge does not lock index' ' sleep 3600 & echo $! >sleep.pid EOF - test_when_finished "kill \$(cat sleep.pid)" && test_write_lines >.gitattributes \ "* merge=ours" "text merge=sleep-an-hour" && test_config merge.ours.driver true && test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh && - git merge master + git merge master && + + # We are testing that the custom merge driver does not block + # index.lock on Windows due to an inherited file handle. + # To ensure that this test checks this condition, the process + # must still be running at this point (and must have started + # in the first place), hence, this kill must not fail: + kill "$(cat sleep.pid)" ' test_done ``` ## Jeff King, 2016-11-11 08:41 Subject: Re: [PATCH] t6026: ensure that long-running script really is Message-ID: <20161111084148.tgtsijn74z2pdylq@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161111084148.tgtsijn74z2pdylq%40sigill.intra.peff.net In-Reply-To: ``` On Fri, Nov 11, 2016 at 07:50:14AM +0100, Johannes Sixt wrote: > Shouldn't we then move the 'kill' out of test_when_finished and make > it a proper condition of the test? Like this? > > diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh > index 348d78b205..6ad8bd098a 100755 > --- a/t/t6026-merge-attr.sh > +++ b/t/t6026-merge-attr.sh > @@ -187,13 +187,19 @@ test_expect_success 'custom merge does not lock index' ' > sleep 3600 & > echo $! >sleep.pid > EOF > - test_when_finished "kill \$(cat sleep.pid)" && > > test_write_lines >.gitattributes \ > "* merge=ours" "text merge=sleep-an-hour" && > test_config merge.ours.driver true && > test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh && > - git merge master > + git merge master && > + > + # We are testing that the custom merge driver does not block > + # index.lock on Windows due to an inherited file handle. > + # To ensure that this test checks this condition, the process > + # must still be running at this point (and must have started > + # in the first place), hence, this kill must not fail: > + kill "$(cat sleep.pid)" > ' That makes it more obvious that the return value of "kill" is important, which is good. But the other thing the "kill" is doing is make sure we clean up after ourselves, even if another part of the test fails. What happens if "merge" unexpectedly fails after starting the sleep process? I think the best compromise is a comment like the one you have here, but around the test_when_finished call (and possibly bumping the call down to right before "git merge master", which is where we expect the process to start). -Peff ``` ## Johannes Sixt, 2016-11-11 20:24 Subject: [PATCH v2] t6026: ensure that long-running script really is Message-ID: <6a421222-a138-5647-4965-8ede24d904b2@kdbg.org> URL: https://gitlist.dev/e/6a421222-a138-5647-4965-8ede24d904b2%40kdbg.org In-Reply-To: <20161111084148.tgtsijn74z2pdylq@sigill.intra.peff.net> ``` When making sure that background tasks are cleaned up in 5babb5b (t6026-merge-attr: clean up background process at end of test case, 2016-09-07), we considered to let the background task sleep longer, just to be certain that it will still be running when we want to kill it after the test. Sadly, the assumption appears not to hold true that the test case passes quickly enough to kill the background task within a second. Simply increase it to an hour. No system can be possibly slow enough to make above-mentioned assumption incorrect. Reported by Andreas Schwab. In-code comments by J6t. Signed-off-by: Johannes Schindelin Signed-off-by: Johannes Sixt --- Am 11.11.2016 um 09:41 schrieb Jeff King: > But the other thing the "kill" is doing is make sure we clean up after > ourselves, even if another part of the test fails. Good point. Here is an updated version. If the pid file is not created, the kill command receives no arguments. Here on Linux, it reports failure in this case. This is good. I could have said test_when_finished "kill \"\$(cat sleep.pid)\"" but that is a bit too much quoting for my taste when it is not strictly necessary. t/t6026-merge-attr.sh | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh index 7a6e33e673..8f9b48a493 100755 --- a/t/t6026-merge-attr.sh +++ b/t/t6026-merge-attr.sh @@ -183,16 +183,24 @@ test_expect_success 'up-to-date merge without common ancestor' ' test_expect_success 'custom merge does not lock index' ' git reset --hard anchor && - write_script sleep-one-second.sh <<-\EOF && - sleep 1 & + write_script sleep-an-hour.sh <<-\EOF && + sleep 3600 & echo $! >sleep.pid EOF - test_when_finished "kill \$(cat sleep.pid)" && test_write_lines >.gitattributes \ - "* merge=ours" "text merge=sleep-one-second" && + "* merge=ours" "text merge=sleep-an-hour" && test_config merge.ours.driver true && - test_config merge.sleep-one-second.driver ./sleep-one-second.sh && + test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh && + + # We are testing that the custom merge driver does not block + # index.lock on Windows due to an inherited file handle. + # To ensure that the backgrounded process ran sufficiently + # long (and has been started in the first place), we do not + # ignore the result of the kill command. + # By packaging the command in test_when_finished, we get both + # the correctness check and the clean-up. + test_when_finished "kill \$(cat sleep.pid)" && git merge master ' -- 2.11.0.rc0.55.gd967357 ``` ## Junio C Hamano, 2016-11-11 20:48 Subject: Re: [PATCH v2] t6026: ensure that long-running script really is Message-ID: URL: https://gitlist.dev/e/xmqq60nt3fqc.fsf%40gitster.mtv.corp.google.com In-Reply-To: <6a421222-a138-5647-4965-8ede24d904b2@kdbg.org> ``` Johannes Sixt writes: > Good point. Here is an updated version. Unfortunately, I already took the version before this one and started my integration cycle today. I'll wiggle this in; it essentially is about adding a big comment to explain what is going on and then moving the when-finished down below it, right? Thanks. > If the pid file is not created, the kill command receives no > arguments. Here on Linux, it reports failure in this case. > This is good. I could have said > > test_when_finished "kill \"\$(cat sleep.pid)\"" > > but that is a bit too much quoting for my taste when it is > not strictly necessary. > > t/t6026-merge-attr.sh | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh > index 7a6e33e673..8f9b48a493 100755 > --- a/t/t6026-merge-attr.sh > +++ b/t/t6026-merge-attr.sh > @@ -183,16 +183,24 @@ test_expect_success 'up-to-date merge without common ancestor' ' > > test_expect_success 'custom merge does not lock index' ' > git reset --hard anchor && > - write_script sleep-one-second.sh <<-\EOF && > - sleep 1 & > + write_script sleep-an-hour.sh <<-\EOF && > + sleep 3600 & > echo $! >sleep.pid > EOF > - test_when_finished "kill \$(cat sleep.pid)" && > > test_write_lines >.gitattributes \ > - "* merge=ours" "text merge=sleep-one-second" && > + "* merge=ours" "text merge=sleep-an-hour" && > test_config merge.ours.driver true && > - test_config merge.sleep-one-second.driver ./sleep-one-second.sh && > + test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh && > + > + # We are testing that the custom merge driver does not block > + # index.lock on Windows due to an inherited file handle. > + # To ensure that the backgrounded process ran sufficiently > + # long (and has been started in the first place), we do not > + # ignore the result of the kill command. > + # By packaging the command in test_when_finished, we get both > + # the correctness check and the clean-up. > + test_when_finished "kill \$(cat sleep.pid)" && > git merge master > ' ``` ## Johannes Sixt, 2016-11-11 20:54 Subject: Re: [PATCH v2] t6026: ensure that long-running script really is Message-ID: URL: https://gitlist.dev/e/ba2b3f08-87b1-c954-2020-e5a4bece7f83%40kdbg.org In-Reply-To: ``` Am 11.11.2016 um 21:48 schrieb Junio C Hamano: > Johannes Sixt writes: > >> Good point. Here is an updated version. > > Unfortunately, I already took the version before this one and > started my integration cycle today. I'll wiggle this in; it > essentially is about adding a big comment to explain what is going > on and then moving the when-finished down below it, right? Correct. Thank you very much! -- Hannes ``` ## Junio C Hamano, 2016-11-11 21:09 Subject: Re: [PATCH v2] t6026: ensure that long-running script really is Message-ID: URL: https://gitlist.dev/e/xmqqmvh5207k.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Sixt writes: > Am 11.11.2016 um 21:48 schrieb Junio C Hamano: >> Johannes Sixt writes: >> >>> Good point. Here is an updated version. >> >> Unfortunately, I already took the version before this one and >> started my integration cycle today. I'll wiggle this in; it >> essentially is about adding a big comment to explain what is going >> on and then moving the when-finished down below it, right? > > Correct. Thanks. It has become like so: -- >8 -- From: Johannes Sixt Date: Fri, 11 Nov 2016 21:24:44 +0100 Subject: [PATCH] t6026: clarify the point of "kill $(cat sleep.pid)" We lengthened the time the leftover process sleeps in the previous commit to make sure it will be there while 'git merge' runs and finishes. It therefore needs to be killed before leaving the test. And it needs to be killed even when 'git merge' fails, so it has to be triggered via test_when_finished mechanism. Explain all that in a large comment, and move the use site of test_when_finished to immediately before 'git merge' invocation, where the process is spawned. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- t/t6026-merge-attr.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh index 348d78b205..8f9b48a493 100755 --- a/t/t6026-merge-attr.sh +++ b/t/t6026-merge-attr.sh @@ -187,12 +187,20 @@ test_expect_success 'custom merge does not lock index' ' sleep 3600 & echo $! >sleep.pid EOF - test_when_finished "kill \$(cat sleep.pid)" && test_write_lines >.gitattributes \ "* merge=ours" "text merge=sleep-an-hour" && test_config merge.ours.driver true && test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh && + + # We are testing that the custom merge driver does not block + # index.lock on Windows due to an inherited file handle. + # To ensure that the backgrounded process ran sufficiently + # long (and has been started in the first place), we do not + # ignore the result of the kill command. + # By packaging the command in test_when_finished, we get both + # the correctness check and the clean-up. + test_when_finished "kill \$(cat sleep.pid)" && git merge master ' -- 2.11.0-rc0-149-gea84a1ce98 ``` ## Johannes Sixt, 2016-11-11 21:15 Subject: Re: [PATCH v2] t6026: ensure that long-running script really is Message-ID: URL: https://gitlist.dev/e/c4cdd21e-0cef-8255-75f2-90c84f271e02%40kdbg.org In-Reply-To: ``` Am 11.11.2016 um 22:09 schrieb Junio C Hamano: > Johannes Sixt writes: > >> Am 11.11.2016 um 21:48 schrieb Junio C Hamano: >>> Johannes Sixt writes: >>> >>>> Good point. Here is an updated version. >>> >>> Unfortunately, I already took the version before this one and >>> started my integration cycle today. I'll wiggle this in; it >>> essentially is about adding a big comment to explain what is going >>> on and then moving the when-finished down below it, right? >> >> Correct. > > Thanks. It has become like so: > > -- >8 -- > From: Johannes Sixt > Date: Fri, 11 Nov 2016 21:24:44 +0100 > Subject: [PATCH] t6026: clarify the point of "kill $(cat sleep.pid)" > > We lengthened the time the leftover process sleeps in the previous > commit to make sure it will be there while 'git merge' runs and > finishes. It therefore needs to be killed before leaving the test. > And it needs to be killed even when 'git merge' fails, so it has to > be triggered via test_when_finished mechanism. > > Explain all that in a large comment, and move the use site of > test_when_finished to immediately before 'git merge' invocation, > where the process is spawned. > > Signed-off-by: Johannes Sixt > Signed-off-by: Junio C Hamano Perfect, thank you very much. > --- > t/t6026-merge-attr.sh | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh > index 348d78b205..8f9b48a493 100755 > --- a/t/t6026-merge-attr.sh > +++ b/t/t6026-merge-attr.sh > @@ -187,12 +187,20 @@ test_expect_success 'custom merge does not lock index' ' > sleep 3600 & > echo $! >sleep.pid > EOF > - test_when_finished "kill \$(cat sleep.pid)" && > > test_write_lines >.gitattributes \ > "* merge=ours" "text merge=sleep-an-hour" && > test_config merge.ours.driver true && > test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh && > + > + # We are testing that the custom merge driver does not block > + # index.lock on Windows due to an inherited file handle. > + # To ensure that the backgrounded process ran sufficiently > + # long (and has been started in the first place), we do not > + # ignore the result of the kill command. > + # By packaging the command in test_when_finished, we get both > + # the correctness check and the clean-up. > + test_when_finished "kill \$(cat sleep.pid)" && > git merge master > ' > > ```