Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes error code to when test is undefined #91

Closed
wants to merge 4 commits into from
Closed

Changes error code to when test is undefined #91

wants to merge 4 commits into from

Conversation

deiga
Copy link
Contributor

@deiga deiga commented Nov 4, 2018

Changes the CLI to be more in line with how Unix operates.
This is in line with how npm foo and npm run foo behave and npm init generates a tests script which has exit 1 defined.

Moved from npm/npm#21026


This change is Reviewable

@deiga deiga requested a review from a team as a code owner November 4, 2018 20:39
@zkat zkat added semver:major backwards-incompatible breaking changes needs-discussion labels Nov 13, 2018
@zkat
Copy link
Contributor

zkat commented Nov 13, 2018

This is a pretty impactful change, and not to be taken lightly. Why do you think it's worth the potential negative impact on thousands of users?

@deiga
Copy link
Contributor Author

deiga commented Nov 27, 2018

@zkat I am well aware that this will have a rather large impact.

In my oppinion there is a fairly large discrepancy in how this script works on a UX level. Most UNIX commands return 1 if something is not defined, just like npm does with everything else. In addition UNIX commands also return 1 when the intended purpose was not fulfilled. Take for example grep which returns 1 if no matches were found.
As a developer or an operations engineer I expect npm test to fail if no one bothered to define it, since it's not doing what you think it is.

Let's think of building a (rather silly) CI pipeline example, where I know that all projects will always be npm based, but then I have to either force the developers to create a test script with exit 1 as default or use some semi-fancy parsing of the pipeline results to identify if any tests have even been run, as I can't count on the output of npm test.

I think it makes a lot of sense to uniform the Interface of how npm test works.

Someone in the npm community already agrees with this as the generated test script includes exit 1

test/tap/run-script.js Outdated Show resolved Hide resolved
@isaacs
Copy link
Contributor

isaacs commented Dec 11, 2018

My $0.02 on this:

I think it's a great idea, and long overdue. I don't love the implementation, though. I think you can just delete the default echo command, and let it just be like npm run arglebargle or any other undefined script.

Like,

$ npm test
npm ERR! missing script: test

I seem to recall that I made it do that once upon a time, probably about 7 years ago or so, and some folks objected because they had setups that ran npm test on every install, and a LOT of packages had no test defined, since there were only a few dozen dozen test runners back then.

As a compromise, I made npm init drop that obnoxious "complain and exit in error" test script by default, and it was just enough of a guardrail to get everyone to at least consider writing a test. Now, most packages have tests, and those that don't, have that exit in error "test", so why not just clean it up?

TL;DR

My suggestion:

  • Get rid of the special casing around test in lib/run-script.js
  • Get rid of the "exit in error" default test in npm init
  • As @zkat wisely points out: This is a semver-major change!

@deiga
Copy link
Contributor Author

deiga commented Dec 22, 2018

@isaacs I really like your suggestion of just deleting the special handling, it makes the most sense here.

@isaacs
Copy link
Contributor

isaacs commented Jul 10, 2019

This will be included in npm v7. It is a breaking change, or I'd get it done sooner, but it looks good to me. Thanks for your continued patience.

@deiga
Copy link
Contributor Author

deiga commented Jul 16, 2019

@isaacs I fixed the merge conflicts. Can I do something else to prepare this?

@isaacs
Copy link
Contributor

isaacs commented Jul 16, 2019

This is looking good. Thanks!

It is a breaking change, but I am sure it can get into npm 7.

@darcyclarke darcyclarke added the Release 7.x work is associated with a specific npm 7 release label Nov 19, 2019
isaacs pushed a commit that referenced this pull request Jul 7, 2020
PR-URL: #91
Credit: @deiga
Close: #91
Reviewed-by: @isaacs

NOTE: The functional part of this was actually done in ac25561, with
the refactoring of lib/run-script.js, but this PR also adds the updates
to the tests that go along with that change.
@darcyclarke
Copy link
Contributor

@deiga Hey! Sorry for the delay, we've essentially refactored this code in v7 so I'm going to close this for now. Appreciate the thought/contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants