aureliojargas / clitest

Command Line Tester
MIT License
143 stars 12 forks source link

Leftover temporary directories #44

Closed vmmello closed 4 years ago

vmmello commented 4 years ago

I've noticed that there are several leftover temporary directories /tmp/clitest.XXXXXX/ created by clitest that are not being removed after it ends. Running test.md is leaving 3 leftover directories on each run.

When running with options like --help and --version it creates temporary dirs that never get removed. I'm creating a pull request to fix these cases.

Though there's one case that is test 42 of test.md (consider the text below as the contents of a test file, not a manual run of the command):

$ ./clitest --debug --color always test/option-debug-color.sh | head -n 3 | tr -d '\033'
[36m--------------------------------------------------------------------------------[m
[36m-- INPUT_LINE[$ echo "tab   " #=> tab   ][m
[36m--   LINE_CMD[[m$ echo "tab[32m<tab>[m" [31m#=> [mtab[32m<tab>[m[36m][m
$

Somehow the test above is leaving a temp directory. Weirdly when the command is run manually in the shell it works as expected. But not when running as a test (when read as a test file from clitest, i.e nested run of clitest).

I've tried to debug it, but I wasn't able to figure it out.

NOTE: I tested that using trap tt_clean_up EXIT solves the problem, but I guess it's not portable (...probably the reason why tt_clean_up is referenced many times).

aureliojargas commented 4 years ago

AFAIK, using trap is portable, but relying on the EXIT signal not (only Bash implements it completely).

I agree that using trap would be a better solution. Honestly, I don't remember why I haven't used it. Probably just forgot about it :/

I've merged your PR, thank you for the contribution! Feel free to add a trap solution. If it passes all tests, it should be portable enough.

vmmello commented 4 years ago

For what I understand, "EXIT" is not an OS signal, but actually a convention to run commands just before the shell exits (even though I see shells use in the documentation the word "signal" informally). I did a quick search and the first mention to it I found on IEEE Std 1003.1, 2004 Edition.

I tested with dash 0.5 from 2008 (Ubuntu 8.04) and it implements trap EXIT (even though the documentation only mentions trap 0, the "EXIT" word also works). The same with ksh 93 (version from 1993 with some patches up to 2007) and it also worked. With this last shell, tens of other tests fail. So I believe this change is not introducing any backwards incompatibility up to what it's being tested so far.

So I've implemented it with trap EXIT. The branch of the original pull request is updated.

aureliojargas commented 4 years ago

@vmmello awesome, could you please open a new pull request for this change?

aureliojargas commented 4 years ago

@vmmello thanks again for your time and contribution. Your changes from #46 were merged. 👍