threads / patch / 44446

patcht6026: ensure that long-running script really is

Subject: [PATCH] t6026: ensure that long-running script really is

## tl;dr

9 messages between Nov 9, 2016 and Nov 11, 2016. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Johannes Schindelin· Nov 9, 2016, 13:51 UTC · lore

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 <johannes.schindelin@gmx.de>
---
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(-)
Show changes to t/t6026-merge-attr.sh +4 −4
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· Nov 10, 2016, 23:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH] t6026: ensure that long-running script really is

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 18 quoted lines
> 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 <johannes.schindelin@gmx.de>
> ---
> 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.
Show 31 quoted lines
>
>  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· Nov 11, 2016, 06:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] t6026: ensure that long-running script really is

Am 11.11.2016 um 00:53 schrieb Junio C Hamano:
Show 30 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> 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 <johannes.schindelin@gmx.de>
>> ---
>> 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?

Show changes to t/t6026-merge-attr.sh +8 −2
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· Nov 11, 2016, 08:41 UTC · re: Johannes Sixt · lore

Re: [PATCH] t6026: ensure that long-running script really is

On Fri, Nov 11, 2016 at 07:50:14AM +0100, Johannes Sixt wrote:
Show 27 quoted lines
> 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· Nov 11, 2016, 20:24 UTC · re: Jeff King · lore

[PATCH v2] t6026: ensure that long-running script really is

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 <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
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(-)
Show changes to t/t6026-merge-attr.sh +13 −5
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· Nov 11, 2016, 20:48 UTC · re: Johannes Sixt · lore

Re: [PATCH v2] t6026: ensure that long-running script really is

Johannes Sixt <j6t@kdbg.org> 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.
Show 45 quoted lines
> 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· Nov 11, 2016, 20:54 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] t6026: ensure that long-running script really is

Am 11.11.2016 um 21:48 schrieb Junio C Hamano:
Show 8 quoted lines
> Johannes Sixt <j6t@kdbg.org> 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· Nov 11, 2016, 21:09 UTC · re: Johannes Sixt · lore

Re: [PATCH v2] t6026: ensure that long-running script really is

Johannes Sixt <j6t@kdbg.org> writes:
Show 11 quoted lines
> Am 11.11.2016 um 21:48 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> 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 <j6t@kdbg.org>
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 <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6026-merge-attr.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
Show changes to t/t6026-merge-attr.sh +9 −1
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· Nov 11, 2016, 21:15 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] t6026: ensure that long-running script really is

Am 11.11.2016 um 22:09 schrieb Junio C Hamano:
Show 33 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 11.11.2016 um 21:48 schrieb Junio C Hamano:
>>> Johannes Sixt <j6t@kdbg.org> 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 <j6t@kdbg.org>
> 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 <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Perfect, thank you very much.
Show 31 quoted lines
> ---
>  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
>  '
>
>

← back to recent threads