`npm run <not-a-valid-script>` fails silently #2959

Closed
andrewrk opened this Issue Nov 24, 2012 · 15 comments

Comments

Projects
None yet
8 participants

this should display an error message and return a nonzero exit code. would a pull request be accepted?

Owner

isaacs commented Nov 25, 2012

Yes, patch welcome.

andrewrk added a commit to andrewrk/npm that referenced this issue Nov 25, 2012

Contributor

gabrielf commented Dec 12, 2012

This would be nice!

Member

edef1c commented Dec 13, 2012

Looks good to me, waiting for @isaacs to approve or merge this.

Owner

isaacs commented Dec 13, 2012

The problem with this is that now npm test will fail if there's no test. I don't think that's ideal.

Perhaps it could just print a warning, rather than exit in error?

@isaacs is npm test the only one that this breaks?

If so, that seems like correct behavior to me. If you try to test, and there are no tests, then running the test command should fail.

Contributor

gabrielf commented Dec 13, 2012

I agree that it should fail if there are no tests. In our case we have had scripts run with "npm run-script " and then the entry has been renamed causing no script to run without our CI-server failing. I think running a named entry that doesn't exist or trying to run test/stop/start etc that hasn't been set should all fail.

Or is there a default implementation for the test/start/stop that I'm not aware of? If that's the case then I agree that those should not fail (unless the default implementation fail).

Member

mfncooper commented Dec 15, 2012

I agree with @isaacs that we shouldn't fail if there's no test script. If someone doesn't provide tests, we shouldn't be stopping the presses, so to speak; nothing failed. Besides, this would be a breaking change in npm functionality, which could make a lot of people very unhappy.

I wonder if we should distinguish between script names that npm knows about / defines, and script names that the package owner presumably defines or the user mistypes. That is, we could quietly ignore the ones npm defines when they aren't present (e.g. test, preinstall), but complain about ones that npm doesn't know about if someone tries to invoke them (e.g. someone uses npm foo when there is no foo script, or mistypes test as testy). This would require that we maintain a list of known script types in code (the same list that's in doc/cli/scripts.md), but that doesn't seem too onerous.

you're saying that this command:

npm test

should succeed if there are no tests?

I do not understand how anyone can think this.

Member

mfncooper commented Dec 16, 2012

I suspect you're thinking about this from the perspective of a mandate for developers to write tests, with npm test being the only acceptable means of running those tests. That's a fine development methodology, and I happen to agree that every package should have unit tests and that npm test can often be a convenient way to run them.

However, I don't believe that npm should be in the business of enforcing any particular methodology. As such, I don't believe that it should be considered an error if someone doesn't provide tests, or if their tests are not runnable via npm. A no-op - perhaps with a warning - seems to me to be an appropriate response in those circumstances.

Well we've each said our piece; now it's up to @isaacs to call the shot.

Member

edef1c commented Dec 16, 2012

Maybe we should have npm test --strict or something like that?

Contributor

gabrielf commented Dec 16, 2012

I agree that no certain methodology should be forced upon anyone by npm, but I still think it seems appropriate to fail running tests that doesn't exist. Unless there are times when the equivalence of "npm test" is invoked automatically on install or some other npm command. Is there?

If tests are run automatically during some other task then I can certainly understand why missing tests should not be an error but I cannot come up with other reasons. Can you?

Changing the behaviour of npm test would only be a breaking change if people currently rely on npm test succeeding on missing tests. I'm trying to understand in what cases people do this.

I vote for:

  1. Fail npm run-script for non-existent scripts
  2. Fail npm test if not set unless npm test is in fact run as a sub-command of some other command and it actually makes sense for it to just be a no-op
Member

domenic commented Mar 25, 2013

So, @isaacs, what's the verdict? I agree with @superjoe30 FWIW.

flipside added a commit to flipside/npm that referenced this issue Jan 24, 2014

fix: npm run fails silently. closes #2959
if the missing command is "test", logs a text error, otherwise creates an actual error.
Contributor

flipside commented Jan 24, 2014

lost a few hours because scripts were silently failing, only to find a second scripts property overwriting the first in package.json...

anyways, similar to @andrewrk 's patch but treat "npm run test" as a special case.

thomblake added a commit to thomblake/npm that referenced this issue Jul 28, 2014

fix: npm run fails silently. closes #2959
if the missing command is "test", logs a text error, otherwise creates an actual error.

smikes added a commit to smikes/npm that referenced this issue Nov 4, 2014

fix: npm run fails silently. closes #2959
if the missing command is "test", logs a text error, otherwise creates an actual error.
fixed so test always "passes" and doesn't exit
Fix ignore-scripts test
Add unit tests

@othiym23 othiym23 closed this in 0aefae9 Nov 7, 2014

andrewrk commented Nov 7, 2014

\o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment