Makefile enhanced to error out with instructions if invoked without a target.

[mklement0]

Makefile enhanced to install required test utility urchin on demand locally, via npm install. The shell command that locates the urchin executable was amended to also find a locally installed version, not just a global one as before.

Note that invoking just make (without a goal) currently (still) only runs the sh test (at least on my OS X 10.9.5 machine). Is that the intent?

[ljharb]

The first step after cloning any repo with package.json is to npm install. Then, to run tests, npm test - which puts the local node_modules folder in the PATH. Our make tasks for tests are not intended to be run by themselves.

(As for the default make task, I don't really have an opinion on what that runs)

[mklement0]

Oops! Sounds like I made some rookie mistakes (and didn't read the read-me re testing). Thanks for clarifying.

That said, there are issues worth resolving:

• The scripts defined in package.json actually bypass Makefile and only run the tests with the default shell, sh (by virtue of how npm invokes scripts).
• Thus, you currently must use make directly to test with all (supported) shells - assuming you've put urchin in the $PATH: make test
• Similarly, you currently must use make directly to make a new release: TAG=<newVersion> make release
• Also, make release utilizes npm package replace, which is not currently defined as a dev dependency in package.json.

---

A fundamental question is whether to continue to (incompletely) expose scripts through both npm test / npm run <script> and make (via Makefile):

• One option is to migrate all Makefile functionality into package.json and make do with only accessing test/release functionality via npm test / npm run <script>.
• Otherwise, if the existing duality should be retained, there are two possible improvements:
  • Implement a simple check that errors out when someone tries to use make directly.
  • Preferably, amend Makefile to transparently act the same when invoked directly as when invoked via npm test / npm run <script> - that happened to be the direction of the initial version of this PR.

---

I'll happily amend this PR, if you tell me how you want to proceed.

[ljharb]

If you note .travis.yml, tests run in all supported shells - altho npm test only runs in sh.

I'd prefer to use package.json rather than a Makefile if there's a choice. make is great, but npm run-script is my preference.

I'd be more than happy to accept something that makes make error out with proper instructions.

[mklement0]

OK, I've made the following changes:

• Invoking just make (without a target (goal)) now errors out saying you either need an explicit target or you should run npm test for the default tests and npm run to see a list of all tests.
• make list now shows a list of targets.
• make - assuming a target was specified - now automatically also finds executables in locally installed npm packages (urchin, replace) and errors out if they're not found, providing a hint that npm install must be run after first cloning the repo.
• npm package replace was added to the dev. dependencies in package.json, so as to make the make release command work.

---

Regarding the multi-shell tests: they don't seem to do anything, because it is only urchin itself that gets invoked with the various shells, not the test scripts, because
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.

[mklement0]

This simply expanded variable (due to := rather than =) - evaluated once, right away - receives the full path to the urchin executable, while first looking for it among the local npm packages.

[ljharb]

whoa - wrt the "In other words: irrespective of what shell urchin was invoked with, the test scripts are always run with sh." line, can you please file a separate issue for that?

[mklement0]

I filed #551 for the cross-shell test bug.

[mklement0]

The updated PR addresses your two requests (replace npm-package dependency semver spec. changed to ~, make list now lists one target per line).

[ljharb]

Isn't replace a posix command? I'm not sure why we need the one from npm at all.

[mklement0]

I don't think replace is a POSIX command.

In practice, it does not come preinstalled on recent versions of any of the following platforms: Ubuntu/Debian, Fedora, OSX.

http://man.cx/replace suggests that it's an Oracle utility.

[ljharb]

Would it be better, then, to use sed, rather than pulling in an extra dependency?

[mklement0]

replace makes the substitution much easier, though, and since there already is a dependency - urchin - I don't see the harm in introducing another.

It could be done in sed, but it would require much more work.

As an aside: I just made what was already there work. Who started using replace, and what are his/her thoughts?

[ljharb]

I believe @koenpunt was the original author of the Makefile. I wasn't aware that it wasn't part of posix at the time.

That's a fair point that it's just a dev dependency, which we already have, and that it does make the substitution easier.

[mklement0]

Summary of the latest commit:

• The shell-specific test targets are now named test-sh, test-bash, ...
• The test target now runs not just the slow tests, but all test suites for all shells, as on Travis.
• The scripts defined in the package.json file now invoke the Makefile (rather than urchin directly), so that all test tasks can be maintained in a single location.
• The release target is now substantially more robust; it now offers version incrementing (e.g., make TAG=patch release to increment the patch level of the current version number, thanks to new dev. dependency semver), checks whether the workspace is clean, and prompts for confirmation.

I hope I didn't change anything I shouldn't have.

[mklement0]

Once #551 is resolved, a small tweak to Makefile will habe to be made: $$shell $(URCHIN) -f test/$$suite must be replaced with $(URCHIN) -s $$shell -f test/$$suite.

[ljharb]

How hard would it be to make it so npm test runs the tests in the current shell, whatever it is?

[mklement0]

@ljharb: I've added it to the latest PR.

[ljharb]

Looks like the tests are failing - perhaps the cwd isn't being set properly?

[mklement0]

Oops! The problem is that I accidentally reset the NVM_DIR environment variable in an effort to unset NVM_* variables from an nvm instance that happens to be loaded into the shell calling the makefile.

I've just changed it to only unset if NVM_BIN is found set, but it still breaks on Travis (not locally); I'm looking into it, but any help is appreciated.

[ljharb]

nvm unload will remove nvm and unset the proper environment variables, fwiw.

[mklement0]

nvm unload will remove nvm

Thanks, but this would have to be done in the calling shell rather than in the makefile - the best that can be done inside the makefile is to undefine NVM_* env. variables.

[mklement0]

@ljharb: The tests pass now.

[ljharb]

This will error out when there are untracked files. Please change this to git status --porcelain --untracked-files=no

[mklement0]

Added to the latest update.

However, wouldn't it be safer to also check for untracked files? If you habitually end up with such files, perhaps they should be added to .gitignore.

Also, shouldn't the tag be an annotated tag (git tag -a "v$(TAG)"), so as to have metadata associated with it (who tagged, what time, ...)?

[ljharb]

Not necessarily - I often have untracked files like npm-debug.log, or files that are for in-progress changes. The only concern here is "what will get committed", and untracked files won't.

I've never used tag with "-a", but I'm fine adding it - as long as the tag name points to the right SHA, it should work fine.

[mklement0]

I see your point re in-progess changes (npm-debug.log is a candidate for .gitignore).

I've added -a to create annotated tags - it's what man git-tag says should be used for releases. They're lightweight tags + metadata, notably who created the tag and when. A message must be recorded as well, so I'm simply defaulting that to the version number.

[ljharb]

I think you'll need another rebase (sorry about that)

[mklement0]

I think you'll need another rebase (sorry about that)

No worries - done.

[ljharb]

i'd prefer not to add spaces for alignment :-) not a big deal tho, i can fix after merging too

[ljharb]

Hmm - $(basename -- $(ps -o comm= $(ps -o ppid= -p $PPID)) | sed 's/^-//') on my terminal echoes "Terminal". What's this supposed to output? Should it just be using $SHELL?

[mklement0]

This determines the grandparent process and therefore only returns the shell when run via npm test or npm run (parent process is npm itself, and the grandparent is the shell that invoked npm).
(When run directly from an interactive shell on OSX, the grandparent process is, as you've experienced, your terminal (parent is login).)

Does it not work when you run npm run test/fast, for instance?

I chose this approach because it truly targets the same shell that the npm command was invoked from, as opposed to using $SHELL, which always reports the user's default shell - the two may or may not be the same.

[ljharb]

gotcha, that makes sense. I'll play around with it some more.

[ljharb]

This will need another rebase :-/ sorry that things are churning. I'd love to get this in but it's large and I need to test it thoroughly - if there's any smaller, less scary chunks that could go out in a separate PR (to shrink this one) that would be cool too.

[mklement0]

I've rebased it again. I hope that'll do. The changes are interrelated, so I don't think chopping this up is the way to go.

[mklement0]

I've consolidated the PR into a single commit and made one last change: the makefile now determines the list of all test suites automatically by obtaining the names of all subfolders of ./test.

That way, adding new tests in the future will automatically include them in npm test or when running make test[-<shell>] without explicitly specifying a suite.

Finally, I've added npm-debug.log to .gitignore.

Let me know if that works.

[ljharb]

Tested this locally and seems to work great. Thanks so much for your persistence on this!

[mklement0]

@ljharb: My pleasure - I learned quite a bit.

Let's hope that @tlevine soon finds the time to merge the urchin PR and publish the update to the npm registry, so that we can implement true cross-shell tests.