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

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

[Johannes Schindelin]

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(-)

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

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

[Junio C Hamano]

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.

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

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

[Johannes Sixt]

Am 11.11.2016 um 00:53 schrieb Junio C Hamano:
> 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?

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

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

[Jeff King]

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

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

[Johannes Sixt]

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(-)

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

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

[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?

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
>  '

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

[Johannes Sixt]

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.

Thank you very much!
-- Hannes

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

[Junio C Hamano]

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>
---
 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

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

[Johannes Sixt]

Am 11.11.2016 um 22:09 schrieb Junio C Hamano:
> 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.

> ---
>  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
>  '
>
>