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

test-npm-install.js now requires Internet connection #36895

Closed
joyeecheung opened this issue Jan 12, 2021 · 4 comments · Fixed by #36933
Closed

test-npm-install.js now requires Internet connection #36895

joyeecheung opened this issue Jan 12, 2021 · 4 comments · Fixed by #36933
Labels
test Issues and PRs related to the tests.

Comments

@joyeecheung
Copy link
Member

It seems recently test-npm-install.js started requiring Internet connection (which I think means now running npm install on a package.json with no module to install requires Internet connection somehow). We should either try to make it runnable offline or move it to test/internet. Anyone familiar with the installer know how to make the former possible?

P.S. this causes a TIMEOUT when I am behind the GFW

cc @nodejs/testing

@joyeecheung joyeecheung added the test Issues and PRs related to the tests. label Jan 12, 2021
@richardlau
Copy link
Member

Cc @nodejs/npm

@ruyadorno
Copy link
Member

I'm taking a look and trying to reproduce but running node test/parallel/test-npm-install.js in my machine exits with code 0 and yields no error at all even without any internet connection. I would appreciate if you can point me a way to reliably reproduce it 😊 thanks!

@Trott
Copy link
Member

Trott commented Jan 13, 2021

running node test/parallel/test-npm-install.js in my machine exits with code 0 and yields no error at all even without any internet connection. I would appreciate if you can point me a way to reliably reproduce it 😊 thanks!

The GFW almost certainly silently discards packets so turning off your network is not the same thing.

On my macOS laptop, adding this to /etc/hosts seems to do the trick:

127.0.0.2	registry.npmjs.org

I'm not sure if silently dropping packets destined for 127.0.0.2 is expected default behavior everywhere, but you can try that. If that doesn't work, another option is to set up a proxy that silently discards packets.

ruyadorno added a commit to ruyadorno/node that referenced this issue Jan 14, 2021
Disabling any internet-required features (namely audit and
update-notifer) in order for the test to work without an internet
connection.

- Fixes: nodejs#36895
@ruyadorno
Copy link
Member

Thanks @Trott that did it 😊

It turns out there were two requests happening in that test, one reaching out to the registry to retrieve info for the update notifier feature the npm cli provides (GET 200 https://registry.npmjs.org/npm) and a second one for the audit feature (POST 200 https://registry.npmjs.org/-/npm/v1/security/advisories/bulk).

It does seem like the internal mechanisms in which these features work might have slightly changed from v6 to v7 (which we'll follow up in the npm cli) - given that none of these features seem essential to the purpose of this test I just sent a PR that disables them in #36933

@aduh95 aduh95 closed this as completed in 887f199 Jan 18, 2021
ruyadorno added a commit that referenced this issue Jan 22, 2021
Disabling any internet-required features (namely audit and
update-notifer) in order for the test to work without an internet
connection.

- Fixes: #36895

PR-URL: #36933
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue May 1, 2021
Disabling any internet-required features (namely audit and
update-notifer) in order for the test to work without an internet
connection.

- Fixes: #36895

PR-URL: #36933
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants