fixes Unsupported URL Type: git+git+ssh bug

[twhid]

More details here: #6422 and here: #7294.

[othiym23]

Thank you! Is it too much to ask for a test?

[twhid]

Should have a test, yes. I'd like to knock something up.

However, when I $ tap ./lib/cache/add-remote-git.js

      Error: npm.load() required
          at Object.npm.config.get (/Users/timwhidden/projects/npm/lib/npm.js:31:11)
          at Object.<anonymous> (/Users/timwhidden/projects/npm/lib/utils/git.js:18:18)
          at Module._compile (module.js:456:26)
          at Object.Module._extensions..js (module.js:474:10)
          at Module.load (module.js:356:32)
          at Function.Module._load (module.js:312:12)
          at Module.require (module.js:364:17)
          at require (module.js:380:17)
          at Object.<anonymous> (/Users/timwhidden/projects/npm/lib/cache/add-remote-git.js:3:11)
          at Module._compile (module.js:456:26)
    command: "/usr/local/Cellar/node/0.10.35_2/bin/node add-remote-git.js"

My inclination would be to refactor the code that generates resolved into it's own module and test that. I.e. everything from https://github.com/npm/npm/blob/master/lib/cache/add-remote-git.js#L188 to https://github.com/npm/npm/blob/master/lib/cache/add-remote-git.js#L201

Too invasive for this small change?

[othiym23]

Too invasive for this small change?

I think so. Tests should go into test/tap anyway. You can probably piggy-back on add-remote-git (or copy it into a new test), which already does the npm.load() dance for you. Sometime within the next few months, @iarna and I are going to start rewriting npm's test suite for consistency and simplicity, but until then, these tests are most useful as functional or integration tests, rather than unit tests.

Thanks for your patience!

[twhid]

That's embarrassing. I ran the test on the wrong file. That's what I get for working on this after... 🍷

OK, I'll see about adding something to /test/tap/add-remote-git.

[twhid]

Hiya,

The current test, test/tap/add-remote-git.js, tests that a mock package can install a mock dep against a real git server. To test a dependency that uses the git+ssh protocol would require adding a lot of setup to this test. Specifically, generating private and public keys for the current user and configuring the system properly (both the git server and the current user).

Not saying I'm not going to do it, but seems like a lot of rigmarole for a change that a simple unit test would cover. It would be a nice integration test to have, but tying it to this change seems somewhat arbitrary to me.

Best,

[othiym23]

Since the underlying bug also affects every other kind of git dependency including git+http:, perhaps the test can do that instead of trying to do complicated mocking or trying to get an ssh server running in a test?

[twhid]

Ah. Good point. I can try that.

On Fri, Feb 20, 2015 at 11:37 AM, Forrest L Norvell <
notifications@github.com> wrote:

Since the underlying bug also affects every other kind of git dependency
including git+http:, perhaps the test can do that instead of trying to do
complicated mocking or trying to get an ssh server running in a test?

—
Reply to this email directly or view it on GitHub
#7296 (comment).

TIM WHID__DEN
Director, Front-End Engineering
1stdibs
*p *347.410.6760
*e *tim@1stdibs.com name@1stdibs.com

1stdibs http://www.1stdibs.com/ - T__he Most Beautiful Things On
Earth.
Follow us on Facebook https://www.facebook.com/1stdibs, Pinterest
http://pinterest.com/1stdibs/ and Twitter https://twitter.com/1stdibs.

[othiym23]

I went ahead and wrote a test and landed it as 5334207. While doing so, I had to do some digging, and realized that there actually was a problem in the GitHub-specific caching code, which I fixed in 0f87f5e. Finally, I tweaked the wording and rebased your patch and landed it as a2e04bd.

Thanks for putting this together, and for your patience in working with me to get it merged!

[twhid]

Thanks! Sorry I didn't find the time to write the test.

[imsaar]

I was having the same issue of having tmp path in my "from" and I realized I was using npm 2.7.0.
I did a npm install npm -g and now I got 2.11.0 and now npm shrinkwrap is generating the "from" correctly.