Add more handling for undefined vars

[yeeplusplus]

Also ensure tests are run with 'nounset' flag.

Resolves #17

[yeeplusplus]

I'm not sure why, but the set -o nounset in run_tests isn't being enforced

[codehearts]

Oh, It might be because run_tests kicks off a new shell for each test! I wonder if adding a setup to each test or modifying run_tests to set nounset in the shell would work. I also realized these options might different slightly between shells, it'd be good to look at different set options and ensure shpy runs under the most restrictive options

I'll check out the subshell angle and approve this if that doesn't pan out or update with that change

[yeeplusplus]

It appears that it (nounset) is supported:

• dash: https://www.mail-archive.com/dash@vger.kernel.org/msg00789.html
• zsh: https://linux.die.net/man/1/zshoptions

I also verified this by adding

set -u
echo ${no_such_var}

to the top of run_tests, and then running each of make test_bash, make test_dash, make test_sh, make test_zsh. Each one failed with the same error

[yeeplusplus]

Also, each of the four shells supports a -u option when invoking it, so we can just remove the set -o nounset from the top of individual tests and add -u to the shell invocation. I'll add that to this PR

[yeeplusplus]

There's a wrinkle. Invoking a script via bash -u SCRIPT_NAME sets the -u flag, but bash -u -c "SCRIPT_NAME" does not. Investigating

[codehearts]

I wonder if we could add a test to verify the testing environment, something that references an undefined variable and expects itself to fail

[yeeplusplus]

We could change

  "$SHELL" -c "SHUNIT_PARENT=$test_file $test_runner $test_file" || exit $?

to

  SHUNIT_PARENT=$test_file "$SHELL" -u $test_file || exit $?

and to ensure kcov is used, we could put the original version in an if statement.

But that's getting too complicated, and I think it would be far simpler to just stick a set -u at the beginning of each test

[codehearts]

It's also possible to use a here-doc to feed multi-line input to the shell's stdin, which might make it clean and easy to call set and retain $test_runner and $SHUNIT_PARENT

[yeeplusplus]

That's another approach that will work but I think it might make the runner (run_tests) too complicated and harder to maintain.

[codehearts]

So I made some interesting discoveries poking at an unrelated hunch of mine:

1. The tests aren't all running under different shells, the shebang in each test is set to sh and the test runner is invoking them in such a way that the shebang is used
2. There are more places where nounset causes errors, which only became apparent once each test shell was running with it set

I've gotten a test in place which verifies nounset is set in the test environment, and which also fixes the shells used to run the test. I'd like to keep your undefined variable fixes and the cleanup tests, but I think the rest of your PR is covered by what I'll be putting up soon 🙏

[yeeplusplus]

OK sounds good! Just to clarify, should I kill this PR?

[codehearts]

I'd say leave it open and I'll add to it. I don't want you to lose your contributions!

[codehearts]

This looks good! I have some changes mostly around testing and removing a few things, which hopefully shouldn't be too bad to make 🙏

[codehearts]

This should be removed, I set it globally for all tests in my upcoming PR

[codehearts]

Private variables (those starting with an underscore, which also isn't documented anywhere…) shouldn't be used directly in testing. Instead, you could try:

• Stubbing the mktmp call to return a known directory and verifying that directory is removed
• Do a pattern match for a directory named shpy.[0-9]+, keeping in mind that zsh doesn't glob like other shells
• Stub all the i/o functions to verify a call for a tmp directory was made and a call to rm it was made

It's likely that $_shpy_spies_dir reflects the directory on disk, but it's best to check for the directory itself because that's what the end user would see 🔍

[codehearts]

This test is good, but you should remove the nounset and private variable references. You shouldn't have to remove the dir tree every time, if you add a teardown function you can more-or-less ensure the test slate is clean

tearDown() {
    cleanupSpies
}

[codehearts]

This can be removed, my upcoming PR runs all tests with nounset to ensure shpy runs under those stricter conditions

[yeeplusplus]

I've had some other priorities come up recently but I'll be returning to this next weekend

[codehearts]

Sounds good! I'll try getting my fixes up, it looks like there were some rather large mistakes I made with testing under different shells and getting coverage reports…