Cleanup actions are only run if no tests failed

[jluebbe]

The cleanup helper documentation says:

Public: Schedule cleanup commands to be run unconditionally when all tests
have run.

This can be used to clean up things like test databases. It is not needed to
clean up temporary files, as test_done already does that.

The test_done() function actually only runs the cleanup actions if the $test_failure variable is 0 (no failed tests). As we use this to stop a local dbus-daemon, we have leftover processes as soon as a single test case fails. It works fine if we add test_eval_ "$final_cleanup" to the *) case as well.

Is this behavior intended? If so, I was confused by the documentation.

[felipec]

I think this behavior is intended. At least the git test suite does the same thing.

[chriscool]

Yeah, this should probably be explained better in the doc.

[felipec]

@jluebbe I think what you want is implemented in Git upstream with test_atexit. That's called truly unconditionally when the tests are done. Even if the test crashes.

[LeSpocky]

Stumbled over the same thing. Documentation mismatches behavior! What fix would you prefer in a PR?

[felipec]

@LeSpocky What explained in my previous comment: port test_atexit from Junio's git so that can be used to specify actions that should be done at the end regardless if tests fail or not.

[LeSpocky]

You mean this one? https://github.com/git/git/blob/master/t/test-lib-functions.sh#L1364

Thing is: final_cleanup is not present in upstream Git test suite, but according to sharness docs it serves the same purpose as test_atexit as far as I understand both docs. Or let me ask the other way round: what would be the purpose of final_cleanup after adding test_atexit in sharness? How would both differ and would it really be necessary to have two of them? The problem at hand for sharness here is: documentation mismatches behaviour. Either one has to be fixed.

[felipec]

You mean this one? https://github.com/git/git/blob/master/t/test-lib-functions.sh#L1364

No, this one: https://github.com/git/git/blob/master/t/test-lib-functions.sh#L1397

Thing is: final_cleanup is not present in upstream Git test suite, but according to sharness docs it serves the same purpose as test_atexit as far as I understand both docs.

I don't think so. One says it runs at unconditionally at the end of the test script, and the other unconditionally after all the tasks have been run. It's not the same.

Or let me ask the other way round: what would be the purpose of final_cleanup after adding test_atexit in sharness?

I don't know.

How would both differ and would it really be necessary to have two of them?

It's probably not necessary to have both of them. But I believe test_atexit can be added without removing final_cleanup.

Eventually we probably should get rid of final_cleanup as most people would want to use test_atexit instead. But we could live with both for a while.

[felipec]

After reading the original commit bfa91e4 it seems clear the intention was to serve as git's test_atexit, but the wording was not the best.

So I've updated the code to make cleanup work as test_atexit, now it's run on failure too, and if the script ends in the middle, or when running in interactive mode, the final-cleanup stuff is executed.

There are some subtle differences with test_atexit probably not important for the vast majority of people.