Fix `npm ls` with --depth=x option #4179

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@yyx990803
Contributor

yyx990803 commented Nov 23, 2013

There are currently two issues when using npm ls with --depth=x option:

  1. Unmet dependency with depth that equals maxDepth is missing the "UNMET DEPENDENCY" label, and the error output has message "max depth reached" instead of "missing".
  2. Met dependencies with depth that equals maxDepth also output error, with message "max depth reached". (just try npm ls -g --depth=0, also see #2843)

This commit should fix both.

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Nov 23, 2013

Contributor

Assuming we have an unmet dependency at depth=0 and another at depth=1,

Before:
before

After:
after

Contributor

yyx990803 commented Nov 23, 2013

Assuming we have an unmet dependency at depth=0 and another at depth=1,

Before:
before

After:
after

@@ -206,7 +206,14 @@ function readInstalled_ (folder, parent, name, reqver, depth, maxDepth, cb) {
return readJson(jsonFile, function (er, depData) {
// already out of our depth, ignore errors
if (er || !depData || !depData.version) return cb(null, obj)
- obj.dependencies[pkg] = depData.version

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Nov 23, 2013

Member

Unfortunately this change needs to be made to the read-installed package, found https://github.com/isaacs/read-installed, and not in npm's copy of it.

@domenic

domenic Nov 23, 2013

Member

Unfortunately this change needs to be made to the read-installed package, found https://github.com/isaacs/read-installed, and not in npm's copy of it.

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Nov 23, 2013

Contributor

Cool I'll move this to a pull request to that repo.
The changes really need to be pulled in together to work properly though.

@yyx990803

yyx990803 Nov 23, 2013

Contributor

Cool I'll move this to a pull request to that repo.
The changes really need to be pulled in together to work properly though.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Nov 23, 2013

Member

Is it possible you could add a tap test for this behavior?

Member

domenic commented Nov 23, 2013

Is it possible you could add a tap test for this behavior?

yyx990803 added a commit to yyx990803/read-installed that referenced this pull request Nov 23, 2013

npm --depth option edge case fix
When a dependency has depth === maxDepth, it should be set with its
package info instead of a plain version string (treated as a problem
by npm). It should also have its own dependencies/peerDependencies
set to an empty object to prevent them being output in `npm ls`.

Also see npm/npm#4179

@yyx990803 yyx990803 referenced this pull request in npm/read-installed Nov 23, 2013

Merged

npm --depth option edge case fix #10

Fix `npm ls` with --depth=x option
1. correctly mark unmet dependency when depth === maxDepth
2. no longer output ERR if depth === maxDepth and dependency is met
3. included Tap test case for this behavior

NOTE: this fix requires this PR for the `read-installed` package
to be merged: npm/read-installed#10
@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Nov 23, 2013

Contributor

I've separated the changes to read-installed and added a tap test case. Please note the test case needs the read-installed changes to be merged to pass.

Contributor

yyx990803 commented Nov 23, 2013

I've separated the changes to read-installed and added a tap test case. Please note the test case needs the read-installed changes to be merged to pass.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Nov 27, 2013

Member

When I upgrade read-installed and rebase this change on top of master, the tests fail. You can check my work at the pr/4179 branch. In particular:

not ok test/tap/ls-with-depth.js ........................ 5/7
    Command: "node" "ls-with-depth.js"
    TAP version 13
    ok 1 should find 3 packages
    ok 2 should find 1 unmet dependencies
    not ok 3 should print 2 ERR messages
      ---
        file:   events.js
        line:   101
        column: 17
        stack:
          - getCaller (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:418:17)
          - assert (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:21:16)
          - Function.equal (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:162:10)
          - Test._testAssert [as equal] (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-test.js:87:16)
          - ChildProcess.<anonymous> (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\test\tap\ls-with-depth.js:24:7)
          - ChildProcess.EventEmitter.emit (events.js:101:17)
          - Process.ChildProcess._handle.onexit (child_process.js:827:12)
        found:  1
        wanted: 2
      ...
    ok 4 should find 4 packages
    ok 5 should find 2 unmet dependencies
    not ok 6 should print 3 ERR messages
      ---
        file:   events.js
        line:   101
        column: 17
        stack:
          - getCaller (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:418:17)
          - assert (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:21:16)
          - Function.equal (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:162:10)
          - Test._testAssert [as equal] (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-test.js:87:16)
          - ChildProcess.<anonymous> (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\test\tap\ls-with-depth.js:42:7)
          - ChildProcess.EventEmitter.emit (events.js:101:17)
          - Process.ChildProcess._handle.onexit (child_process.js:827:12)
        found:  2
        wanted: 3
      ...
    ok 7 test/tap/ls-with-depth.js

    1..7
    # tests 7
    # pass  5
    # fail  2
Member

domenic commented Nov 27, 2013

When I upgrade read-installed and rebase this change on top of master, the tests fail. You can check my work at the pr/4179 branch. In particular:

not ok test/tap/ls-with-depth.js ........................ 5/7
    Command: "node" "ls-with-depth.js"
    TAP version 13
    ok 1 should find 3 packages
    ok 2 should find 1 unmet dependencies
    not ok 3 should print 2 ERR messages
      ---
        file:   events.js
        line:   101
        column: 17
        stack:
          - getCaller (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:418:17)
          - assert (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:21:16)
          - Function.equal (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:162:10)
          - Test._testAssert [as equal] (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-test.js:87:16)
          - ChildProcess.<anonymous> (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\test\tap\ls-with-depth.js:24:7)
          - ChildProcess.EventEmitter.emit (events.js:101:17)
          - Process.ChildProcess._handle.onexit (child_process.js:827:12)
        found:  1
        wanted: 2
      ...
    ok 4 should find 4 packages
    ok 5 should find 2 unmet dependencies
    not ok 6 should print 3 ERR messages
      ---
        file:   events.js
        line:   101
        column: 17
        stack:
          - getCaller (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:418:17)
          - assert (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:21:16)
          - Function.equal (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-assert.js:162:10)
          - Test._testAssert [as equal] (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\node_modules\tap\lib\tap-test.js:87:16)
          - ChildProcess.<anonymous> (C:\Users\Domenic\Dropbox\Programming\GitHub\npm\test\tap\ls-with-depth.js:42:7)
          - ChildProcess.EventEmitter.emit (events.js:101:17)
          - Process.ChildProcess._handle.onexit (child_process.js:827:12)
        found:  2
        wanted: 3
      ...
    ok 7 test/tap/ls-with-depth.js

    1..7
    # tests 7
    # pass  5
    # fail  2
@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Nov 27, 2013

Member

Investigating further this could very well be an issue with Node.js 0.11 on Windows, so, if another committer can get the tests to pass, then we don't need to wait for me.

Member

domenic commented Nov 27, 2013

Investigating further this could very well be an issue with Node.js 0.11 on Windows, so, if another committer can get the tests to pass, then we don't need to wait for me.

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Nov 27, 2013

Contributor

Ok - so I tested for both Mac/Ubuntu with both 0.10.22 and 0.11.9, works fine and passes the test in all four cases - I have actually never used Node on windows so I hope someone else can look into that...

Contributor

yyx990803 commented Nov 27, 2013

Ok - so I tested for both Mac/Ubuntu with both 0.10.22 and 0.11.9, works fine and passes the test in all four cases - I have actually never used Node on windows so I hope someone else can look into that...

@luk-

This comment has been minimized.

Show comment Hide comment
@luk-

luk- Dec 3, 2013

Contributor

I updated npm to use read-installed@0.2.5, running through the tests w/ this patch now.

Contributor

luk- commented Dec 3, 2013

I updated npm to use read-installed@0.2.5, running through the tests w/ this patch now.

@luk-

This comment has been minimized.

Show comment Hide comment
@luk-

luk- Dec 3, 2013

Contributor

Tests are failing for me, checking to see if it's this or the new read-installed.

Contributor

luk- commented Dec 3, 2013

Tests are failing for me, checking to see if it's this or the new read-installed.

@luk-

This comment has been minimized.

Show comment Hide comment
@luk-

luk- Dec 3, 2013

Contributor

Something is happening with read-installed when the entire test suite is ran, but not when the outdated tests are individually ran. I need to look into this further tonight/tomorrow when I get more time.

Contributor

luk- commented Dec 3, 2013

Something is happening with read-installed when the entire test suite is ran, but not when the outdated tests are individually ran. I need to look into this further tonight/tomorrow when I get more time.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Dec 3, 2013

Member

pinging @robertkowalski, he's seen and I believe fixed similar cross-test corruption before.

Member

domenic commented Dec 3, 2013

pinging @robertkowalski, he's seen and I believe fixed similar cross-test corruption before.

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Dec 3, 2013

Contributor

Just ran the whole suite too, here's what I found: running the old tests (node ./test/run.js) would somehow delete the node_modules folders in the two test fixture folders I created. Here's the git diff:

#   deleted:    test/tap/ls-with-depth/node_modules/ok/package.json
#   deleted:    test/tap/ls-with-depth/node_modules/test/package.json
#   deleted:    test/tap/outdated-with-depth/node_modules/request/node_modules/underscore/package.json
#   deleted:    test/tap/outdated-with-depth/node_modules/request/package.json

These would cause the tap tests for ls-with-depth and outdated-with-depth to both fail. If you hard reset then run tap tests alone it would all pass.

Contributor

yyx990803 commented Dec 3, 2013

Just ran the whole suite too, here's what I found: running the old tests (node ./test/run.js) would somehow delete the node_modules folders in the two test fixture folders I created. Here's the git diff:

#   deleted:    test/tap/ls-with-depth/node_modules/ok/package.json
#   deleted:    test/tap/ls-with-depth/node_modules/test/package.json
#   deleted:    test/tap/outdated-with-depth/node_modules/request/node_modules/underscore/package.json
#   deleted:    test/tap/outdated-with-depth/node_modules/request/package.json

These would cause the tap tests for ls-with-depth and outdated-with-depth to both fail. If you hard reset then run tap tests alone it would all pass.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Dec 3, 2013

Member

What the ...!?

Member

domenic commented Dec 3, 2013

What the ...!?

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Dec 4, 2013

Contributor

I think I figured out the problem. npm's package.json has this "prepublish" script: node bin/npm-cli.js prune --prefix=. --no-global && rm -rf test/*/*/node_modules && make -j4 doc. This line gets executed during npm install in npm's own directory during the tests, and the command rm -rf test/*/*/node_modules deleted my test fixtures.

Removed that command and the whole npm test suite passes, but obviously it's there for a reason. I think a solution would be changing my tests to create the fixtures on the fly. Thoughts?

Contributor

yyx990803 commented Dec 4, 2013

I think I figured out the problem. npm's package.json has this "prepublish" script: node bin/npm-cli.js prune --prefix=. --no-global && rm -rf test/*/*/node_modules && make -j4 doc. This line gets executed during npm install in npm's own directory during the tests, and the command rm -rf test/*/*/node_modules deleted my test fixtures.

Removed that command and the whole npm test suite passes, but obviously it's there for a reason. I think a solution would be changing my tests to create the fixtures on the fly. Thoughts?

@luk-

This comment has been minimized.

Show comment Hide comment
@luk-

luk- Dec 5, 2013

Contributor

Same error I got. Just match your tests to the same patterns that exist. If
you don't see one, whatever works and is understandable for other
contributors is fine.

This is my fault for merging it in.

On Wednesday, December 4, 2013, Evan You wrote:

I think I figured out the problem. npm's package.json has this
"prepublish" script: node bin/npm-cli.js prune --prefix=. --no-global &&
rm -rf test///node_modules && make -j4 doc. The command rm -rf
test///node_modules deleted my test fixtures.

Removed that command and the whole npm test suite passes, but obviously
it's there for a reason. I think a solution would be changing my tests to
create the fixtures on the fly. Thoughts?


Reply to this email directly or view it on GitHubhttps://github.com/isaacs/npm/pull/4179#issuecomment-29856718
.

Contributor

luk- commented Dec 5, 2013

Same error I got. Just match your tests to the same patterns that exist. If
you don't see one, whatever works and is understandable for other
contributors is fine.

This is my fault for merging it in.

On Wednesday, December 4, 2013, Evan You wrote:

I think I figured out the problem. npm's package.json has this
"prepublish" script: node bin/npm-cli.js prune --prefix=. --no-global &&
rm -rf test///node_modules && make -j4 doc. The command rm -rf
test///node_modules deleted my test fixtures.

Removed that command and the whole npm test suite passes, but obviously
it's there for a reason. I think a solution would be changing my tests to
create the fixtures on the fly. Thoughts?


Reply to this email directly or view it on GitHubhttps://github.com/isaacs/npm/pull/4179#issuecomment-29856718
.

@ljharb

This comment has been minimized.

Show comment Hide comment
@ljharb

ljharb Dec 5, 2013

Contributor

ps, naming this branch "pr/4179" conflicts with fetch = +refs/pull/*/head:refs/remotes/origin/pr/* which many people use to fetch all pull requests as remote refs. Can this branch be renamed perhaps?

Contributor

ljharb commented Dec 5, 2013

ps, naming this branch "pr/4179" conflicts with fetch = +refs/pull/*/head:refs/remotes/origin/pr/* which many people use to fetch all pull requests as remote refs. Can this branch be renamed perhaps?

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Dec 5, 2013

Contributor

@luk should I open a new PR or overwrite this one?

Contributor

yyx990803 commented Dec 5, 2013

@luk should I open a new PR or overwrite this one?

@luk-

This comment has been minimized.

Show comment Hide comment
@luk-

luk- Dec 5, 2013

Contributor

new one please

Contributor

luk- commented Dec 5, 2013

new one please

@luk-

This comment has been minimized.

Show comment Hide comment
@luk-

luk- Dec 5, 2013

Contributor

+1 on @ljharb's thing

Contributor

luk- commented Dec 5, 2013

+1 on @ljharb's thing

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Dec 18, 2013

+1 for this

Would prefer to see the "missing" message go away for anything outside the depth. All they do is clutter the output when you would otherwise just get a simple list of your immediate deps at depth 0

+1 for this

Would prefer to see the "missing" message go away for anything outside the depth. All they do is clutter the output when you would otherwise just get a simple list of your immediate deps at depth 0

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Dec 18, 2013

Contributor

@defunctzombie not sure what your point is. This patch does ignore anything out of depth, including missing deps.

Contributor

yyx990803 commented Dec 18, 2013

@defunctzombie not sure what your point is. This patch does ignore anything out of depth, including missing deps.

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Dec 18, 2013

@yyx990803 maybe I misread the "after" picture cause I saw that one missing dep (missing-b) for depth=1 and would not have expected that

@yyx990803 maybe I misread the "after" picture cause I saw that one missing dep (missing-b) for depth=1 and would not have expected that

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Dec 18, 2013

Contributor

@defunctzombie --depth=0 would print root dependencies, --depth=1 would include one more level... that should be the expected behavior right?

Contributor

yyx990803 commented Dec 18, 2013

@defunctzombie --depth=0 would print root dependencies, --depth=1 would include one more level... that should be the expected behavior right?

@defunctzombie

This comment has been minimized.

Show comment Hide comment
@defunctzombie

defunctzombie Dec 18, 2013

@yyx990803 yep, I was just misreading the images :)

@yyx990803 yep, I was just misreading the images :)

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Dec 18, 2013

Contributor

FYI the second issue is now fixed in 1.3.17 with the updated read-installed package.
First issue is still happening though.

Contributor

yyx990803 commented Dec 18, 2013

FYI the second issue is now fixed in 1.3.17 with the updated read-installed package.
First issue is still happening though.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Jan 8, 2014

Member

Any chance on updating this to create the fixtures on the fly (e.g. move them there from another folder)?

Member

domenic commented Jan 8, 2014

Any chance on updating this to create the fixtures on the fly (e.g. move them there from another folder)?

@robertkowalski

This comment has been minimized.

Show comment Hide comment
@robertkowalski

robertkowalski Feb 7, 2014

Member

What is the status here? What is merged, what not? Maybe I can close the issue?

Member

robertkowalski commented Feb 7, 2014

What is the status here? What is merged, what not? Maybe I can close the issue?

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Feb 7, 2014

Member

@robertkowalski we were trying to merge but the problem at #4179 (comment) makes it need some test-rewriting.

Member

domenic commented Feb 7, 2014

@robertkowalski we were trying to merge but the problem at #4179 (comment) makes it need some test-rewriting.

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 Feb 7, 2014

Contributor

@robertkowalski @domenic see this: #4259
I was waiting for the mock registry to have some more content to work with per luk's suggestion. Will take a look this weekend.

Contributor

yyx990803 commented Feb 7, 2014

@robertkowalski @domenic see this: #4259
I was waiting for the mock registry to have some more content to work with per luk's suggestion. Will take a look this weekend.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Apr 18, 2014

Member

Bump @yyx990803

Member

domenic commented Apr 18, 2014

Bump @yyx990803

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic May 14, 2014

Member

If someone ( @robertkowalski?) would be willing to take this over, it's kind of badly needed.

Member

domenic commented May 14, 2014

If someone ( @robertkowalski?) would be willing to take this over, it's kind of badly needed.

@robertkowalski

This comment has been minimized.

Show comment Hide comment
@robertkowalski

robertkowalski May 14, 2014

Member

I can take care this weekend

Member

robertkowalski commented May 14, 2014

I can take care this weekend

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 May 14, 2014

Contributor

I just opened a new PR with tests using the mock registry: #5276

In addition to unmet dependencies, invalid and extraneous deps are also not labeled correctly at max depth at the moment. The issue roots from the read-installed package which is not handling this edge case properly, and I've submitted PR for that as well.

Contributor

yyx990803 commented May 14, 2014

I just opened a new PR with tests using the mock registry: #5276

In addition to unmet dependencies, invalid and extraneous deps are also not labeled correctly at max depth at the moment. The issue roots from the read-installed package which is not handling this edge case properly, and I've submitted PR for that as well.

@yyx990803

This comment has been minimized.

Show comment Hide comment
@yyx990803

yyx990803 May 28, 2014

Contributor

Closing this in favor of #5276 .
@domenic thoughts?

Contributor

yyx990803 commented May 28, 2014

Closing this in favor of #5276 .
@domenic thoughts?

@yyx990803 yyx990803 closed this May 28, 2014

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