Add failing regression test for npm ls --depth=0 #4757

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Member

robertkowalski commented Feb 22, 2014

This test should turn green if read-installed is fixed and should
avoid regressions in the future.

This tests the cli, as it turned out that the programatical version
of npm has a bug where depth=0 is not working

#4733

test/tap/ls-depth-cli.js
+ , node = process.execPath
+ , npm = path.resolve(__dirname, '../../cli.js')
+
+function run (command, t, test) {
@domenic

domenic Feb 22, 2014

Member

I wonder if we can put this in common-tap by now?

test/tap/ls-depth-cli.js
+ t.has(c, /test-package-with-one-dep@0\.0\.0/
+ , "output contains test-package-with-one-dep@0.0.0")
+ t.doesNotHave(c, /test-package@0\.0\.0/
+ , "output not contains test-package@0.0.0")
@domenic

domenic Feb 22, 2014

Member

"does not contain" is slightly better English, FWIW :)

Member

domenic commented Feb 22, 2014

LGTM although at some point we should figure out a way to deduplicate code in the tap tests generally. Maybe factoring out the spawn helper would be good in that regard.

robertkowalski added some commits Feb 22, 2014

Add failing regression test for npm ls --depth=0
This test should turn green if read-installed is fixed and should
avoid regressions in the future.

This tests the cli, as it turned out that the programatical version
of npm has a bug where depth=0 is not working
Factor test-helper out of tests
Bonus:
  - Fix test in `startstop.js`
Member

robertkowalski commented Feb 22, 2014

Yep, that's a good point. Introduced the helper and found a bug in the startstop.js test -- the helper returned the same results:

return {actual: output, expected: output}

// later

t.equal(c.actual, c.expected)
var port = exports.port = 1337
exports.registry = "http://localhost:" + port
+
+exports.run = run
+function run (cmd, t, opts, cb) {
@isaacs

isaacs Apr 15, 2014

Owner

Why not common.npm(['help', 'args', 'etc'], opts, cb)? Most of the time we do this, it's all npm cli stuff anyway.

Owner

isaacs commented Apr 15, 2014

I like adding a common thing to test npm cli stuff. That's long overdue.

The other thing is also good, but I dont' want to just land a failing test without the code to fix it.

Member

domenic commented Apr 18, 2014

This test no longer fails, merging.

@domenic domenic closed this Apr 18, 2014

@othiym23 othiym23 added the bug label Sep 24, 2014

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