From 81c95c7de71b40831ad46356d75ed56b20c66372 Mon Sep 17 00:00:00 2001 From: Gar Date: Wed, 17 Jan 2024 07:19:59 -0800 Subject: [PATCH] fix: don't reset update notifier duration on every check (#7063) Instead only reset if we actually checked the registry for a new version. --- lib/utils/update-notifier.js | 17 ++++---- test/lib/utils/update-notifier.js | 67 +++++++++++++++++++++++-------- 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/lib/utils/update-notifier.js b/lib/utils/update-notifier.js index 1b3e21d878b94..7c9611e4773f9 100644 --- a/lib/utils/update-notifier.js +++ b/lib/utils/update-notifier.js @@ -98,11 +98,16 @@ const updateNotifier = async (npm, spec = 'latest') => { return null } + // intentional. do not await this. it's a best-effort update. if this + // fails, it's ok. might be using /dev/null as the cache or something weird + // like that. + writeFile(lastCheckedFile(npm), '').catch(() => {}) + return updateCheck(npm, spec, version, current) } // only update the notification timeout if we actually finished checking -module.exports = async npm => { +module.exports = npm => { if ( // opted out !npm.config.get('update-notifier') @@ -113,14 +118,8 @@ module.exports = async npm => { // CI || ciInfo.isCI ) { - return null + return Promise.resolve(null) } - const notification = await updateNotifier(npm) - - // intentional. do not await this. it's a best-effort update. if this - // fails, it's ok. might be using /dev/null as the cache or something weird - // like that. - writeFile(lastCheckedFile(npm), '').catch(() => {}) - return notification + return updateNotifier(npm) } diff --git a/test/lib/utils/update-notifier.js b/test/lib/utils/update-notifier.js index cc5348a440e0a..052367c60dadb 100644 --- a/test/lib/utils/update-notifier.js +++ b/test/lib/utils/update-notifier.js @@ -23,6 +23,7 @@ const runUpdateNotifier = async (t, { prefixDir, version = CURRENT_VERSION, argv = [], + wroteFile = false, ...config } = {}) => { const mockFs = { @@ -37,6 +38,7 @@ const runUpdateNotifier = async (t, { return { mtime: new Date(STAT_MTIME) } }, writeFile: async (path, content) => { + wroteFile = true if (content !== '') { t.fail('no write file content allowed') } @@ -86,88 +88,108 @@ const runUpdateNotifier = async (t, { const result = await updateNotifier(mock.npm) return { + wroteFile, result, MANIFEST_REQUEST, } } -t.test('does not notify by default', async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t) +t.test('duration has elapsed, no updates', async t => { + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t) + t.equal(wroteFile, true) t.not(result) t.equal(MANIFEST_REQUEST.length, 1) }) t.test('situations in which we do not notify', t => { t.test('nothing to do if notifier disabled', async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { 'update-notifier': false, }) + t.equal(wroteFile, false) t.equal(result, null) t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests') }) t.test('do not suggest update if already updating', async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { command: 'install', prefixDir: { 'package.json': `{"name":"${t.testName}"}` }, argv: ['npm'], global: true, }) + t.equal(wroteFile, false) t.equal(result, null) t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests') }) t.test('do not suggest update if already updating with spec', async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { command: 'install', prefixDir: { 'package.json': `{"name":"${t.testName}"}` }, argv: ['npm@latest'], global: true, }) + t.equal(wroteFile, false) t.equal(result, null) t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests') }) t.test('do not update if same as latest', async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t) + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t) + t.equal(wroteFile, true) t.equal(result, null) t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version') }) t.test('check if stat errors (here for coverage)', async t => { const STAT_ERROR = new Error('blorg') - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_ERROR }) + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_ERROR }) + t.equal(wroteFile, true) t.equal(result, null) t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version') }) t.test('ok if write errors (here for coverage)', async t => { const WRITE_ERROR = new Error('grolb') - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { WRITE_ERROR }) + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { WRITE_ERROR }) + t.equal(wroteFile, true) t.equal(result, null) t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version') }) t.test('ignore pacote failures (here for coverage)', async t => { const PACOTE_ERROR = new Error('pah-KO-tchay') - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { PACOTE_ERROR }) + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { PACOTE_ERROR }) t.equal(result, null) + t.equal(wroteFile, true) t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version') }) t.test('do not update if newer than latest, but same as next', async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { version: NEXT_VERSION }) + const { + wroteFile, + result, + MANIFEST_REQUEST, + } = await runUpdateNotifier(t, { version: NEXT_VERSION }) t.equal(result, null) + t.equal(wroteFile, true) const reqs = ['npm@latest', `npm@^${NEXT_VERSION}`] t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions') }) t.test('do not update if on the latest beta', async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { version: CURRENT_BETA }) + const { + wroteFile, + result, + MANIFEST_REQUEST, + } = await runUpdateNotifier(t, { version: CURRENT_BETA }) t.equal(result, null) + t.equal(wroteFile, true) const reqs = [`npm@^${CURRENT_BETA}`] t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions') }) t.test('do not update in CI', async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { mocks: { + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { mocks: { 'ci-info': { isCI: true, name: 'something' }, } }) + t.equal(wroteFile, false) t.equal(result, null) t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests') }) @@ -175,7 +197,8 @@ t.test('situations in which we do not notify', t => { t.test('only check weekly for GA releases', async t => { // One week (plus five minutes to account for test environment fuzziness) const STAT_MTIME = Date.now() - 1000 * 60 * 60 * 24 * 7 + 1000 * 60 * 5 - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_MTIME }) + const { wroteFile, result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { STAT_MTIME }) + t.equal(wroteFile, false, 'duration was not reset') t.equal(result, null) t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests') }) @@ -183,9 +206,14 @@ t.test('situations in which we do not notify', t => { t.test('only check daily for betas', async t => { // One day (plus five minutes to account for test environment fuzziness) const STAT_MTIME = Date.now() - 1000 * 60 * 60 * 24 + 1000 * 60 * 5 - const res = await runUpdateNotifier(t, { STAT_MTIME, version: HAVE_BETA }) - t.equal(res.result, null) - t.strictSame(res.MANIFEST_REQUEST, [], 'no requests for manifests') + const { + wroteFile, + result, + MANIFEST_REQUEST, + } = await runUpdateNotifier(t, { STAT_MTIME, version: HAVE_BETA }) + t.equal(wroteFile, false, 'duration was not reset') + t.equal(result, null) + t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests') }) t.end() @@ -204,8 +232,13 @@ t.test('notification situations', async t => { for (const [version, reqs] of Object.entries(cases)) { for (const color of [false, 'always']) { await t.test(`${version} - color=${color}`, async t => { - const { result, MANIFEST_REQUEST } = await runUpdateNotifier(t, { version, color }) + const { + wroteFile, + result, + MANIFEST_REQUEST, + } = await runUpdateNotifier(t, { version, color }) t.matchSnapshot(result) + t.equal(wroteFile, true) t.strictSame(MANIFEST_REQUEST, reqs.map(r => `npm@${r.replace('{V}', version)}`)) }) }