use node-gyp correctly in the tests

[aminya]

Description of the change

This uses npm's built-in node-gyp which is used in the tests. The tests assume that node-gyp should exist in node_modules, but this is an assumption about the test environment which may not be true. npm does not hoist node-gyp which is a dependency of the npm to the top-level node-modules in the newer versions. It adds node-gyp to devDependencies just for install-spec test.

apm/spec/install-spec.coffee

Lines 536 to 541 in 475d35f

	nodeModules=fs.realpathSync(path.join(__dirname, '..', 'node_modules'))
	beforeEach->
	# Normally npm_config_node_gyp would be ignored, but it works here because we're calling apm
	# directly and not through the scripts in bin/
	fs.copySyncpath.join(nodeModules, 'node-gyp'), path.join(nodeModules, 'with a space')

Verification

The tests now pass correctly. This is tested on Atom too: atom-community/atom#103

Drawbacks

none

Release Notes

N/A

Closes#10

[aminya]

For what it's worth, the require.resolve('node-gyp') workaround is all that should be needed here, IMO.

That's not correct. Using a newer npm (#900) changes the location of node-gyp which results in the test failure.

[aminya]

also passes CI. 6226314

This is with the old npm.

I have already tested this: atom-community#10 (comment)

We need to install node-gyp for testing apm on the newer npm versions. There is no way around this, and we should not rely on the hoisting algorithm.

[DeeDeeG]

I see you are right. Sorry for the misunderstanding.

[aminya]

I don't understand what you are trying to achieve. I just added a devDependency which is only used in the tests. This does not affect Atom or anything. This machine-generated package-lock.json does not change by running npm install.

[aminya]

The point of this PR is to not rely on npm's hoisting algorithm and package-lock.json which are susceptible to change. Updating package-lock.json with a different command is not sustainable and is a very fragile approach.

If a certain version of node-gyp needs to be a devDependency it should be declared in package.json. I added version 5 just in case Atom's build scripts rely on this fragile behavior.

[DeeDeeG]

That works. Thanks for addressing it. This look good to me now.

[darangi]

Thanks 🙇🏾‍♂️ @aminya

[aminya]

You're welcome!