Cross-shell tests mistakenly run everything with sh

[mklement0]

The cross-shell tests specified in .travis.yml / runnable with make test do not work as intended, because it is only urchin itself that gets invoked with the various shells, not the test scripts:
urchin invokes test scripts directly, so that each script's shebang line is respected, and they're all set to #!/bin/sh.

In other words: irrespective of what shell urchin was invoked with, the test scripts are always run with sh.

Seems to me that you'd have to change urchin itself in order to support invocation of test scripts with a specifiable shell.

Note that simply removing the shebang lines from the test scripts is NOT an option, because different shells have different default behavior (some process shebang-less files as themselves, others default to sh).

[ljharb]

cc @koenpunt

[mklement0]

@ljharb: If the urchin PR gets merged and published, then the only change required on the nvm side would be to add option -s to the urchin invocations in the makefile (see updated urchin read-me at https://github.com/scraperwiki/urchin/pull/8/files#diff-2)

[ljharb]

Awesome, thanks!

[drj11]

Just remove the #!/bin/sh from the tests that are shell scripts and leave them executable. They will be invoked by the shell that is running urchin. See this section of POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01 paragraph 2.

[mklement0]

@drj11: Unfortunately, as I noted above, not all POSIX-like shells work this way:

• bash, ksh: execute a shebang-less script - when invoked stand-alone (e.g., ./script) - as themselves.
• zsh, dash: default to executing the script with sh (which, in the case of dash, may be - but isn't necessarily - dash itself).

[mklement0]

@ljharb Unfortunately, my approach to solving this problem by adding a new feature to urchin was misguided.

However, the good news is that there is a solution that works with urchin's current version. It is outlined at scraperwiki/urchin#9

I will take a closer look soon, in conjunction with further makefile changes.

[ljharb]

So all our test files need to have a shell line at the top?? That seems pretty onerous.

[mklement0]

Indeed, it's not pretty, but it's the only solution I see at the moment.
It's not complicated, but requires a lot of discipline - however, we could simply implement a meta test that ensures that the line is present in all other tests.

My original approach - the -s option discussed above - would have worked, but not for the general case, only for tests that source the shell code to test (which happens to be sufficient for nvm).

I'm not sure the maintainers of urchin (@drj11) would go for adding a feature with such limited scope, especially given that it can mislead people into thinking (as it did me originally) that it also works for directly invoking scripts to test.

That said, @tlevine has said at https://github.com/scraperwiki/urchin/pull/9#issuecomment-59350954 that he's thinking about a better solution - I'll post more thoughts there.

[tlevine]

I like the -s option for its simplicity, and I think it's fine to
have a feature that is kind of weird as a flag, especially if we
document that it is intended for things that get sourced.

My current thinking for Urchin is to make a separate executable that
you can call inside of your tests to run a script in several languages.

See the ./all-shells file in the "multiple-shells" branch of
git@github.com:tlevine/urchin

I think this would work for the situation where shell scripts are run
rather than sourced; I don't think this would help with sourced things
like nvm, but I say this here in case it sparks any ideas.

On 16 Oct 06:35, Michael Klement wrote:

Indeed, it's not pretty, but it's the only solution I see at the moment.
It's not complicated, but requires a lot of discipline - however, we could simply implement a meta test that ensures that the line is present in all other tests.

My original approach - the -s option discussed above - would have worked, but not for the general case, only for tests that source the shell code to test.

I'm not sure the maintainers of urchin (@drj11) would go for adding a feature with such limited scope, especially given that it can mislead people into thinking (as it did me originally) that it also works for directly invoking scripts to test.

That said, @tlevine has said at https://github.com/scraperwiki/urchin/pull/9#issuecomment-59350954 that he's thinking about a better solution - I'll post more thoughts there.

---

Reply to this email directly or view it on GitHub:
#551 (comment)

[mklement0]

@tlevine Let's continue the discussion at scraperwiki/urchin#11.

@ljharb Just to get a sense: Should the urchin maintainers decide not to implement support for cross-shell tests, we're left with 2 options:

• Use urchin as is, placing [ -n "$TEST_SHELL" ] && TEST_SHELL= exec "$TEST_SHELL" -- "$0" "$@" at the top of every test script and invoking urchin with TEST_SHELL=<shell> urchin ...
• Use a forked version of urchin that implements cross-shell-test support, possibly even one directly embedded in the nvm repo itself.

If we had to make this choice, what would be your preference?

[ljharb]

If we had to fork urchin, I'd absolutely want it to be a separate published module on npm.

If we require that preamble, we'd need another test that would fail if any files inside the test dir were missing it.

I'm really not happy with either one. I'd use the former, if there's a likelihood we'd switch off of urchin at some point (#429), but otherwise I'd probably go with the latter.

[mklement0]

@ljharb
Understood.

The outlook is promising, fortunately: After rethinking the original approach, I've submitted a new PR (scraperwiki/urchin#12), which not only would help the nvm tests but is a worthwhile addition to urchin itself, I think; the feedback is encouraging so far.

If implemented, the only change required on the nvm side would be to add option -s <shell> to the urchin invocations in the makefile. That is, you'd no longer invoke urchin itself with a different shell, but would instruct it to invoke test scripts with the specified shell.

Re replacing urchin: I quite like its simplicity - based on conventions, no DSL to learn; the only notable missing feature is TAP (machine-parseable) output.
I see no compelling reason to switch.
Have there been problems in the past?

[mklement0]

@ljharb: Good news: @tlevine is taking over maintenance of urchin, and he's planning to merge the PR and publish an update to npm next week.

[ljharb]

Awesome, thanks so much for your persistence on this, and for discovering the problem! This is one of the blockers for v1.0 :-)

[mklement0]

@ljharb: My pleasure; let's hope it proceeds as planned.

[ljharb]

This is now fixed; thanks to @mklement0 for recognizing and unilaterally fixing this problem!