t6026-merge-attr: don't fail if sleep exits early
PATCH 1/1 t6026-merge-attr: don't fail if sleep exits early
Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end of test case") added a kill command to clean up after the test, but this can fail if the sleep command exits before the cleanup is executed. Ignore the error from the kill command.
---
The failure can be simulated by adding a sleep after the last command to delay the cleanup.
--- t/t6026-merge-attr.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/t6026-merge-attr.sh +1 -1
@@ -187,7 +187,7 @@ test_expect_success 'custom merge does not lock index' ' sleep 1 & echo $! >sleep.pid EOF - test_when_finished "kill \$(cat sleep.pid)" && + test_when_finished "kill \$(cat sleep.pid) || :" && test_write_lines >.gitattributes \ "* merge=ours" "text merge=sleep-one-second" &&
Signature · 7 lines
2.10.2 -- Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Quoted 9 lines · Andreas Schwab
Commit
5babb5bdb3("t6026-merge-attr: clean up background process at end of test case") added a kill command to clean up after the test, but this can fail if the sleep command exits before the cleanup is executed. Ignore the error from the kill command.Signed-off-by Andreas Schwab <[email protected]>---
The failure can be simulated by adding a sleep after the last command to delay the cleanup.
Thanks for the reproduction hint. I sometimes run the test suite through a "stress" script that sees if a test script racily fails under load, but I wasn't able to trigger it here (I guess a full second is just too long even under high load). But the extra sleep makes it obvious.
Looks like the original is in v2.10.1, so this should probably go to maint, as well as the upcoming v2.11-rc1.
-Peff
Hi
Quoted 12 lines · Jeff King
Quoted 9 lines · Andreas Schwab
Commit
5babb5bdb3("t6026-merge-attr: clean up background process at end of test case") added a kill command to clean up after the test, but this can fail if the sleep command exits before the cleanup is executed. Ignore the error from the kill command.Signed-off-by Andreas Schwab <[email protected]>---
The failure can be simulated by adding a sleep after the last command to delay the cleanup.
Quoted 7 lines
Thanks for the reproduction hint. I sometimes run the test suite through a "stress" script that sees if a test script racily fails under load, but I wasn't able to trigger it here (I guess a full second is just too long even under high load). But the extra sleep makes it obvious.
Looks like the original is in v2.10.1, so this should probably go to maint, as well as the upcoming v2.11-rc1.
The reason why we do not ignore kill errors is that we want to make sure that the script *actually ran*. Otherwise, the thing we need to test here does not necessarily get tested.
So I would rather go with the patch Hannes hinted at when he said that he did not want to change the file name (as it would have made the diff less obvious): increase the number of seconds.
Will send out a superseding patch in a minute,
Dscho
Quoted 3 lines · Johannes Schindelin
The reason why we do not ignore kill errors is that we want to make sure that the script *actually ran*. Otherwise, the thing we need to test here does not necessarily get tested.
That can be tested by looking for the pid file.
Andreas.
Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Quoted 7 lines · Andreas Schwab
Quoted 3 lines · Johannes Schindelin
The reason why we do not ignore kill errors is that we want to make sure that the script *actually ran*. Otherwise, the thing we need to test here does not necessarily get tested.
That can be tested by looking for the pid file.
I agree that makes the intent a lot more obvious. Having a necessary condition of the test stuffed into a test_when_finished block seems counter-intuitive.
-Peff
PATCH v2 1/1 t6026-merge-attr: don't fail if sleep exits early
Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end of test case") added a kill command to clean up after the test, but this can fail if the sleep command exits before the cleanup is executed. Ignore the error from the kill command.
Explicitly check for the existence of the pid file to test that the merge driver was actually called.
--- t/t6026-merge-attr.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
t/t6026-merge-attr.sh +3 -2
@@ -187,13 +187,14 @@ test_expect_success 'custom merge does not lock index' ' sleep 1 & echo $! >sleep.pid EOF - test_when_finished "kill \$(cat sleep.pid)" && + test_when_finished "kill \$(cat sleep.pid) || :" && test_write_lines >.gitattributes \ "* merge=ours" "text merge=sleep-one-second" && test_config merge.ours.driver true && test_config merge.sleep-one-second.driver ./sleep-one-second.sh && - git merge master + git merge master && + test -f sleep.pid ' test_done
Signature · 7 lines
2.10.2 -- Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Quoted 10 lines · Andreas Schwab
Commit
5babb5bdb3("t6026-merge-attr: clean up background process at end of test case") added a kill command to clean up after the test, but this can fail if the sleep command exits before the cleanup is executed. Ignore the error from the kill command.Explicitly check for the existence of the pid file to test that the merge driver was actually called.
Signed-off-by Andreas Schwab <[email protected]>---
OK. sleep.pid is a reasonable easy-to-access side effect we can observe to make sure that the sleep-one-second merge driver was indeed invoked, which was missing from the earlier round.
Thanks, will apply.
Quoted 26 lines
t/t6026-merge-attr.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)t/t6026-merge-attr.sh +3
-2@@ -187,13 +187,14 @@ test_expect_success 'custom merge does not lock index' ' sleep 1 & echo $! >sleep.pid EOF - test_when_finished "kill \$(cat sleep.pid)" && + test_when_finished "kill \$(cat sleep.pid) || :" && test_write_lines >.gitattributes \ "* merge=ours" "text merge=sleep-one-second" && test_config merge.ours.driver true && test_config merge.sleep-one-second.driver ./sleep-one-second.sh && - git merge master + git merge master && + test -f sleep.pid ' test_doneSignature · 1 line
2.10.2
Hi all,
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Andreas Schwab
Commit
5babb5bdb3("t6026-merge-attr: clean up background process at end of test case") added a kill command to clean up after the test, but this can fail if the sleep command exits before the cleanup is executed. Ignore the error from the kill command.Explicitly check for the existence of the pid file to test that the merge driver was actually called.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Andreas Schwab <[email protected]>---
OK. sleep.pid is a reasonable easy-to-access side effect we can observe to make sure that the sleep-one-second merge driver was indeed invoked, which was missing from the earlier round.
No, this is incorrect. The condition that we need to know applies is that the script is still running, and blocking if the bug reappears.
It is not enough to test that the script *started* running. If it exits too early, the problematic condition is never tested, and the test is useless.
Ciao,
Johannes
Quoted 6 lines · Johannes Schindelin
Quoted 3 lines
OK. sleep.pid is a reasonable easy-to-access side effect we can observe to make sure that the sleep-one-second merge driver was indeed invoked, which was missing from the earlier round.
No, this is incorrect. The condition that we need to know applies is that the script is still running, and blocking if the bug reappears.
OK, I see what you are saying, and I see a few things wrong in here:
* First, the test is titled in a misleading way. In the context of a patch that was titled ad65f7e3b7 ("t6026-merge-attr: child processes must not inherit index.lock handles", 2016-08-18), it might have been clear enough to say "does not lock index", but the sleeping is to make sure that we would notice if the fd to the index.lock leaked to the child process by mistake, and the way to do so is that the child arranges the leaked fd to be kept open after it exits (by spawning "sleep"). The test was never about "does not lock index" (the driver does not take any lock by itself in the first place).
* There are three possible outcome from this test:
- 'git merge' fails.
This is expected to happen only on Windows and if the code gets broken and starts leaking the fd.
- 'git merge' finishes correctly, the sleep is still running when test_when_finished goes to cull it.
In this case, we KNOW there wasn't any fd leak IF we are on Windows where a leaked FD would not allow 'git merge' to succeed. But on other platforms, fd leak that may cause trouble for Windows friends will not be caught.
- 'git merge' finishes correctly, the sleep is no longer running because the machine was heavily loaded; a workaround is to tolerate failure of culling it.
In this case, we cannot tell anything from the test. Even if the fd was leaked, 'git merge' may have succeeded even on Windows.
As everybody knows there is no appropriate timeout value that is good for everybody. I wonder if we can replace the sleep 1 with something like
( while sleep 3600; do :; done ) &
so that leaked fd will be kept even in any heavily loaded environment instead?
Quoted 8 lines · Junio C Hamano
As everybody knows there is no appropriate timeout value that is good for everybody. I wonder if we can replace the sleep 1 with something like
( while sleep 3600; do :; done ) &so that leaked fd will be kept even in any heavily loaded environment instead?
I think you may have missed:
which does roughly that. It does not loop, but I suspect 3600 is plenty in practice.
I do think the test would be a lot more obvious if it confirmed at the end of the test that the process was still running, as opposed to relying on test_when_finished to check it.
-Peff
Quoted 3 lines · Jeff King
I do think the test would be a lot more obvious if it confirmed at the end of the test that the process was still running, as opposed to relying on
test_when_finishedto check it.
I agree that "check that the process is still running" is a wrong thing to do in the first place. What would it mean if the process is no longer running? It is a timing-dependent bug in the test; after all we failed to produce the condition that could trigger a bug that we are guarding against. And "let's do '|| :'" is sweeping the bug (not in the code we are testing, but in the test that will fail to notice a bug we are preparing against) under the rug. So I agree with Dscho that we should do that first.
If we ensure that the process is still running, then such a check is a good belt-and-suspenders way to catch a breakage in the mechanism we choose to ensure it. So probably we can require that the kill in the "when finished" part to actually send a signal to a process that is still running.
Is there an equivalent to pause(2) available to shell scripts? I really hate a single "sleep 3600" or anything with a magic number.
Quoted 8 lines · Junio C Hamano
If we ensure that the process is still running, then such a check is a good belt-and-suspenders way to catch a breakage in the mechanism we choose to ensure it. So probably we can require that the kill in the "when finished" part to actually send a signal to a process that is still running.
Is there an equivalent to pause(2) available to shell scripts? I really hate a single "sleep 3600" or anything with a magic number.
I think it is usually spelled "read <some-fifo", but we can't use FIFOs here because Windows doesn't have them. You could probably do something with "read <&9" and set up descriptor 9 in the test code. But frankly, that gets complex pretty quickly, as you have to background things.
This minor issue isn't worth it. Just bumping the sleep to 3600 makes the raciness problem go away, and everything works in practice. That's probably good enough for our purposes.
-Peff