From 53a18f1016e222680e46ea84f8c53363c808e057 Mon Sep 17 00:00:00 2001 From: Gar Date: Thu, 24 Feb 2022 10:29:16 -0800 Subject: [PATCH] fix(doctor): allow for missing local bin and node_modules --- lib/commands/doctor.js | 42 +++--- .../test/lib/commands/doctor.js.test.cjs | 58 ++++++++ test/lib/commands/doctor.js | 131 ++++++++++++------ 3 files changed, 169 insertions(+), 62 deletions(-) diff --git a/lib/commands/doctor.js b/lib/commands/doctor.js index 552fe5d517341..630150c0886fe 100644 --- a/lib/commands/doctor.js +++ b/lib/commands/doctor.js @@ -10,7 +10,6 @@ const semver = require('semver') const { promisify } = require('util') const log = require('../utils/log-shim.js') const ansiTrim = require('../utils/ansi-trim.js') -const isWindows = require('../utils/is-windows.js') const ping = require('../utils/ping.js') const { registry: { default: defaultRegistry }, @@ -55,32 +54,36 @@ class Doctor extends BaseCommand { ['node -v', 'getLatestNodejsVersion', []], ['npm config get registry', 'checkNpmRegistry', []], ['which git', 'getGitPath', []], - ...(isWindows + ...(process.platform === 'win32' ? [] : [ - ['Perms check on cached files', 'checkFilesPermission', [this.npm.cache, true, R_OK]], [ + 'Perms check on cached files', + 'checkFilesPermission', + [this.npm.cache, true, R_OK], + ], [ 'Perms check on local node_modules', 'checkFilesPermission', - [this.npm.localDir, true], - ], - [ + [this.npm.localDir, true, R_OK | W_OK, true], + ], [ 'Perms check on global node_modules', 'checkFilesPermission', - [this.npm.globalDir, false], - ], - [ + [this.npm.globalDir, false, R_OK], + ], [ 'Perms check on local bin folder', 'checkFilesPermission', - [this.npm.localBin, false, R_OK | W_OK | X_OK], - ], - [ + [this.npm.localBin, false, R_OK | W_OK | X_OK, true], + ], [ 'Perms check on global bin folder', 'checkFilesPermission', [this.npm.globalBin, false, X_OK], ], ]), - ['Verify cache contents', 'verifyCachedFiles', [this.npm.flatOptions.cache]], + [ + 'Verify cache contents', + 'verifyCachedFiles', + [this.npm.flatOptions.cache], + ], // TODO: // - ensure arborist.loadActual() runs without errors and no invalid edges // - ensure package-lock.json matches loadActual() @@ -202,11 +205,7 @@ class Doctor extends BaseCommand { } } - async checkFilesPermission (root, shouldOwn, mask = null) { - if (mask === null) { - mask = shouldOwn ? R_OK | W_OK : R_OK - } - + async checkFilesPermission (root, shouldOwn, mask, missingOk) { let ok = true const tracker = log.newItem(root, 1) @@ -218,8 +217,11 @@ class Doctor extends BaseCommand { for (const f of files) { tracker.silly('checkFilesPermission', f.substr(root.length + 1)) const st = await lstat(f).catch(er => { - ok = false - tracker.warn('checkFilesPermission', 'error getting info for ' + f) + // if it can't be missing, or if it can and the error wasn't that it was missing + if (!missingOk || er.code !== 'ENOENT') { + ok = false + tracker.warn('checkFilesPermission', 'error getting info for ' + f) + } }) tracker.completeWork(1) diff --git a/tap-snapshots/test/lib/commands/doctor.js.test.cjs b/tap-snapshots/test/lib/commands/doctor.js.test.cjs index 057b6f2e46c2b..a28654e5d9b29 100644 --- a/tap-snapshots/test/lib/commands/doctor.js.test.cjs +++ b/tap-snapshots/test/lib/commands/doctor.js.test.cjs @@ -826,6 +826,64 @@ Perms check on global bin folder not ok Check the permissions of files in {C Verify cache contents ok verified 0 tarballs ` +exports[`test/lib/commands/doctor.js TAP missing local node_modules > logs 1`] = ` +Object { + "error": Array [], + "info": Array [ + Array [ + "Running checkup", + ], + Array [ + "checkPing", + "Pinging registry", + ], + Array [ + "getLatestNpmVersion", + "Getting npm package information", + ], + Array [ + "getLatestNodejsVersion", + "Getting Node.js release information", + ], + Array [ + "getGitPath", + "Finding git in your PATH", + ], + Array [ + "verifyCachedFiles", + "Verifying the npm cache", + ], + Array [ + "verifyCachedFiles", + String( + Verification complete. Stats: { + "badContentCount": 0, + "reclaimedCount": 0, + "missingContent": 0, + "verifiedContent": 0 + } + ), + ], + ], + "warn": Array [], +} +` + +exports[`test/lib/commands/doctor.js TAP missing local node_modules > missing local node_modules 1`] = ` +Check Value Recommendation/Notes +npm ping ok +npm -v ok current: v1.0.0, latest: v1.0.0 +node -v ok current: v1.0.0, recommended: v1.0.0 +npm config get registry ok using default registry (https://registry.npmjs.org/) +which git ok /path/to/git +Perms check on cached files ok +Perms check on local node_modules ok +Perms check on global node_modules ok +Perms check on local bin folder ok +Perms check on global bin folder ok +Verify cache contents ok verified 0 tarballs +` + exports[`test/lib/commands/doctor.js TAP node out of date - current > logs 1`] = ` Object { "error": Array [], diff --git a/test/lib/commands/doctor.js b/test/lib/commands/doctor.js index 9445db8a6fa05..5badab99a1d56 100644 --- a/test/lib/commands/doctor.js +++ b/test/lib/commands/doctor.js @@ -1,8 +1,9 @@ const t = require('tap') -const { load: loadMockNpm } = require('../../fixtures/mock-npm') -const tnock = require('../../fixtures/tnock.js') const fs = require('fs') +const { load: loadMockNpm } = require('../../fixtures/mock-npm') +const tnock = require('../../fixtures/tnock.js') +const mockGlobals = require('../../fixtures/mock-globals') const { cleanCwd, cleanDate } = require('../../fixtures/clean-snapshot.js') const cleanCacheSha = (str) => @@ -10,38 +11,6 @@ const cleanCacheSha = (str) => t.cleanSnapshot = p => cleanCacheSha(cleanDate(cleanCwd(p))) -// TODO mockglobals! -const isWindows = require('../../../lib/utils/is-windows.js') - -const processVersion = process.version -// TODO mockglobals! -t.beforeEach(() => { - Object.defineProperty(process, 'version', { value: 'v1.0.0' }) -}) - -const consoleErrorFn = console.error -let consoleError = false -console.error = () => { - consoleError = true -} -t.teardown(() => { - Object.defineProperty(process, 'version', { value: processVersion }) - console.error = consoleErrorFn -}) - -t.afterEach(() => { - consoleError = false -}) - -// getuid and getgid do not exist in windows, so we shim them -// to return 0, as that is the value that lstat will assign the -// gid and uid properties for fs.Stats objects -// TODO mockglobals! -if (isWindows) { - process.getuid = () => 0 - process.getgid = () => 0 -} - const npmManifest = (version) => { return { name: 'npm', @@ -83,9 +52,37 @@ const dirs = { }, } +let consoleError = false +t.afterEach(() => { + consoleError = false +}) + +const globals = { + console: { + error: () => { + consoleError = true + }, + }, + process: { + platform: 'test-not-windows', + version: 'v1.0.0', + }, +} + +// getuid and getgid do not exist in windows, so we shim them +// to return 0, as that is the value that lstat will assign the +// gid and uid properties for fs.Stats objects +if (process.platform === 'win32') { + mockGlobals(t, { + process: { + getuid: () => 0, + getgid: () => 0, + }, + }) +} + const mocks = { '../../package.json': { version: '1.0.0' }, - '../../lib/utils/is-windows.js': false, which: async () => '/path/to/git', cacache: { verify: () => { @@ -97,6 +94,7 @@ const mocks = { t.test('all clear', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -113,6 +111,7 @@ t.test('all clear', async t => { t.test('all clear in color', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -130,6 +129,7 @@ t.test('all clear in color', async t => { t.test('silent', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, config: { loglevel: 'silent', }, @@ -145,9 +145,11 @@ t.test('silent', async t => { t.notOk(consoleError, 'console.error not called') t.matchSnapshot({ info: logs.info, warn: logs.warn, error: logs.error }, 'logs') }) + t.test('ping 404', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -164,6 +166,7 @@ t.test('ping 404', async t => { t.test('ping 404 in color', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -180,6 +183,7 @@ t.test('ping 404 in color', async t => { t.test('ping exception with code', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -195,6 +199,7 @@ t.test('ping exception with code', async t => { t.test('ping exception without code', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -210,6 +215,7 @@ t.test('ping exception without code', async t => { t.test('npm out of date', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -223,9 +229,15 @@ t.test('npm out of date', async t => { }) t.test('node out of date - lts', async t => { - Object.defineProperty(process, 'version', { value: 'v0.0.1' }) const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals: { + ...globals, + process: { + platform: 'test-not-windows', + version: 'v0.0.1', + }, + }, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -239,9 +251,15 @@ t.test('node out of date - lts', async t => { }) t.test('node out of date - current', async t => { - Object.defineProperty(process, 'version', { value: 'v2.0.0' }) const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals: { + ...globals, + process: { + ...globals.process, + version: 'v2.0.0', + }, + }, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -257,6 +275,7 @@ t.test('node out of date - current', async t => { t.test('non-default registry', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, config: { registry: 'http://some-other-url.npmjs.org' }, ...dirs, }) @@ -278,6 +297,7 @@ t.test('missing git', async t => { throw new Error('test error') }, }, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -292,9 +312,13 @@ t.test('missing git', async t => { t.test('windows skips permissions checks', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { - mocks: { - ...mocks, - '../../lib/utils/is-windows.js': true, + mocks, + globals: { + ...globals, + process: { + ...globals.process, + platform: 'win32', + }, }, prefixDir: {}, globalPrefixDir: {}, @@ -312,6 +336,7 @@ t.test('windows skips permissions checks', async t => { t.test('missing global directories', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, prefixDir: dirs.prefixDir, }) tnock(t, npm.config.get('registry')) @@ -324,6 +349,22 @@ t.test('missing global directories', async t => { t.matchSnapshot({ info: logs.info, warn: logs.warn, error: logs.error }, 'logs') }) +t.test('missing local node_modules', async t => { + const { joinedOutput, logs, npm } = await loadMockNpm(t, { + mocks, + globals, + globalPrefixDir: dirs.globalPrefixDir, + }) + tnock(t, npm.config.get('registry')) + .get('/-/ping?write=true').reply(200, '{}') + .get('/npm').reply(200, npmManifest(npm.version)) + tnock(t, 'https://nodejs.org') + .get('/dist/index.json').reply(200, nodeVersions) + await npm.exec('doctor', []) + t.matchSnapshot(joinedOutput(), 'missing local node_modules') + t.matchSnapshot({ info: logs.info, warn: logs.warn, error: logs.error }, 'logs') +}) + t.test('incorrect owner', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks: { @@ -338,6 +379,7 @@ t.test('incorrect owner', async t => { }, }, }, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -361,6 +403,7 @@ t.test('incorrect permissions', async t => { }, }, }, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -384,6 +427,7 @@ t.test('error reading directory', async t => { }, }, }, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -406,6 +450,7 @@ t.test('cacache badContent', async t => { }, }, }, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -428,6 +473,7 @@ t.test('cacache reclaimedCount', async t => { }, }, }, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -450,6 +496,7 @@ t.test('cacache missingContent', async t => { }, }, }, + globals, ...dirs, }) tnock(t, npm.config.get('registry')) @@ -465,8 +512,9 @@ t.test('cacache missingContent', async t => { t.test('bad proxy', async t => { const { joinedOutput, logs, npm } = await loadMockNpm(t, { mocks, + globals, config: { - proxy: 'ssh://npmjs.org' + proxy: 'ssh://npmjs.org', }, ...dirs, }) @@ -474,4 +522,3 @@ t.test('bad proxy', async t => { t.matchSnapshot(joinedOutput(), 'output') t.matchSnapshot({ info: logs.info, warn: logs.warn, error: logs.error }, 'logs') }) -