From d798f809efe3ad28cf7beac5c3a29c206e1817f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 08:18:32 +0000 Subject: [PATCH 1/9] test: add failing tests documenting identified bugs Adds test/bugs.spec.ts with 6 failing tests that demonstrate 5 bugs: 1. GotDownloader progress bar suppression uses || instead of &&, so neither quiet:true nor ELECTRON_GET_NO_PROGRESS alone suppresses the progress bar (src/GotDownloader.ts:52). 2. GotDownloader leaks its 30s progress timer when a download fails because clearTimeout is not in a finally block (src/GotDownloader.ts:79-91). 3. initializeProxy() calls require() which is undefined in ESM, silently breaking proxy support (src/proxy.ts:36). 4. validateArtifact() does not forward tempDirectory to the nested SHASUMS256.txt download, so checksums are written to os.tmpdir() regardless of user config (src/index.ts:77-88). 5. validateArtifact() leaks an empty temp directory when cacheMode is ReadOnly/Bypass and checksums are not provided, because the cleanup finally deletes the wrong directory (src/index.ts:111-120). --- test/bugs.spec.ts | 271 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 271 insertions(+) create mode 100644 test/bugs.spec.ts diff --git a/test/bugs.spec.ts b/test/bugs.spec.ts new file mode 100644 index 00000000..dcae29ce --- /dev/null +++ b/test/bugs.spec.ts @@ -0,0 +1,271 @@ +import fs from 'graceful-fs'; +import os from 'node:os'; +import path from 'node:path'; + +import { PathLike } from 'node:fs'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { GotDownloader } from '../src/GotDownloader'; +import { downloadArtifact } from '../src'; +import { ElectronDownloadCacheMode } from '../src/types'; +import { FixtureDownloader } from './FixtureDownloader'; + +async function flushMicrotasks(): Promise { + for (let i = 0; i < 10; i++) { + await Promise.resolve(); + } +} + +describe('Bug: GotDownloader progress bar suppression', () => { + // The documentation for `quiet` says: + // "if `true`, disables the console progress bar (setting the + // `ELECTRON_GET_NO_PROGRESS` environment variable to a non-empty value + // also does this)." + // + // The condition at src/GotDownloader.ts:52 is: + // if (!quiet || !process.env.ELECTRON_GET_NO_PROGRESS) { + // + // With `||`, the progress timer is scheduled whenever EITHER side is + // truthy, meaning you need BOTH `quiet: true` AND the env var set to + // suppress it. The correct operator is `&&`. + + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'got-bug-')); + }); + + afterEach(async () => { + await fs.promises.rm(tmpDir, { recursive: true, force: true }); + delete process.env.ELECTRON_GET_NO_PROGRESS; + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it('should not schedule a progress bar timer when quiet: true', async () => { + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + vi.useFakeTimers(); + + const downloader = new GotDownloader(); + const target = path.resolve(tmpDir, 'out.txt'); + + const p = downloader.download('http://127.0.0.1:1/nope', target, { quiet: true }); + p.catch(() => { + /* ignore */ + }); + + // Let the `await fs.promises.mkdir()` inside download() resolve so the + // progress-bar scheduling code runs. + await flushMicrotasks(); + + // With the bug: a 30-second timer IS scheduled because + // !quiet || !env -> false || true -> true + // Expected: no timer scheduled. + expect(vi.getTimerCount()).toBe(0); + + vi.useRealTimers(); + await p.catch(() => { + /* ignore */ + }); + }); + + it('should not schedule a progress bar timer when ELECTRON_GET_NO_PROGRESS is set', async () => { + process.env.ELECTRON_GET_NO_PROGRESS = '1'; + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + vi.useFakeTimers(); + + const downloader = new GotDownloader(); + const target = path.resolve(tmpDir, 'out.txt'); + + const p = downloader.download('http://127.0.0.1:1/nope', target); + p.catch(() => { + /* ignore */ + }); + + await flushMicrotasks(); + + // With the bug: a 30-second timer IS scheduled because + // !quiet || !env -> true || false -> true + // Expected: no timer scheduled. + expect(vi.getTimerCount()).toBe(0); + + vi.useRealTimers(); + await p.catch(() => { + /* ignore */ + }); + }); +}); + +describe('Bug: GotDownloader timer leak on download error', () => { + // At src/GotDownloader.ts:79-91, if `pipeline()` throws, the error is + // re-thrown before `clearTimeout(timeout)` runs. The 30-second timer + // remains armed and will fire, creating a progress bar for a download + // that already failed. The cleanup should be in a `finally` block. + + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'got-bug-')); + }); + + afterEach(async () => { + await fs.promises.rm(tmpDir, { recursive: true, force: true }); + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it('should clear the progress timer even when the download fails', async () => { + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + + const realCreateWriteStream = fs.createWriteStream; + vi.spyOn(fs, 'createWriteStream').mockImplementationOnce((p: PathLike) => { + const stream = realCreateWriteStream(p); + setImmediate(() => stream.emit('error', new Error('boom'))); + return stream; + }); + + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); + + const downloader = new GotDownloader(); + const target = path.resolve(tmpDir, 'out.txt'); + + await expect(downloader.download('http://127.0.0.1:1/nope', target)).rejects.toThrow(); + + // With the bug: the 30-second progress-bar timer is still armed after + // the download has already rejected. + // Expected: all timers scheduled by download() are cleared on failure. + expect(vi.getTimerCount()).toBe(0); + + vi.clearAllTimers(); + }); +}); + +describe('Bug: initializeProxy uses require() in an ESM module', () => { + // src/proxy.ts:36 calls `require('global-agent')`. Since package.json + // declares `"type": "module"`, `require` is not defined at runtime in the + // built `dist/` output. The try/catch swallows the ReferenceError, so + // proxy support silently never works. + // + // NOTE: vitest injects a `require` shim into the test environment, so this + // bug does NOT reproduce under vitest. It only manifests when the built + // package is imported by plain Node.js. Run the built output directly to + // observe the ReferenceError: + // + // yarn build && node -e "import('./dist/proxy.js').then(m => m.initializeProxy())" + // + // Output (with DEBUG=@electron/get:proxy): + // ReferenceError: require is not defined + + it('documents the require-in-ESM bug (cannot reproduce under vitest)', () => { + const source = fs.readFileSync(path.resolve(__dirname, '../src/proxy.ts'), 'utf8'); + // This assertion fails while the bug exists: the source still uses + // bare `require(` which is undefined in ESM. The fix is to use + // `createRequire(import.meta.url)` or a dynamic `import()`. + expect(source).not.toMatch(/\brequire\(['"]global-agent['"]\)/); + }); +}); + +describe('Bug: SHASUMS256.txt download ignores custom tempDirectory', () => { + // When validateArtifact() downloads SHASUMS256.txt (src/index.ts:77-88), + // it does not forward `artifactDetails.tempDirectory` to the nested + // downloadArtifact call. The checksum file is therefore written to + // os.tmpdir() even when the caller explicitly requested a different + // temp directory (e.g. because os.tmpdir() is read-only or on a + // different filesystem). + + let cacheRoot: string; + let customTemp: string; + + beforeEach(async () => { + cacheRoot = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'cache-')); + customTemp = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'custom-temp-')); + }); + + afterEach(async () => { + await fs.promises.rm(cacheRoot, { recursive: true, force: true }); + await fs.promises.rm(customTemp, { recursive: true, force: true }); + }); + + it('should use the custom tempDirectory for the SHASUMS256.txt download', async () => { + const shasumsTargetDirs: string[] = []; + + const trackingDownloader = { + async download(url: string, targetFilePath: string): Promise { + if (url.endsWith('SHASUMS256.txt')) { + shasumsTargetDirs.push(path.dirname(targetFilePath)); + } + return new FixtureDownloader().download(url, targetFilePath); + }, + }; + + await downloadArtifact({ + artifactName: 'electron', + version: '2.0.9', + platform: 'darwin', + arch: 'x64', + cacheRoot, + tempDirectory: customTemp, + downloader: trackingDownloader, + cacheMode: ElectronDownloadCacheMode.WriteOnly, + }); + + expect(shasumsTargetDirs.length).toBeGreaterThan(0); + for (const dir of shasumsTargetDirs) { + // With the bug: SHASUMS256.txt is downloaded under os.tmpdir(), not customTemp. + // Expected: all temp paths should be under the user-supplied tempDirectory. + expect(dir.startsWith(customTemp)).toBe(true); + } + }); +}); + +describe('Bug: validateArtifact leaks its temp directory in ORPHAN mode', () => { + // validateArtifact() (src/index.ts:47-121) creates its own temp directory + // via withTempDirectoryIn(). When the outer cacheMode makes + // doesCallerOwnTemporaryOutput() true (ReadOnly or Bypass), it passes + // TempDirCleanUpMode.ORPHAN, relying on the `finally` at line 113 to + // clean up. But when checksums are NOT provided, shasumPath lives in a + // *different* temp directory (created by the nested downloadArtifact for + // SHASUMS256.txt), so the `finally` deletes that other directory and the + // validateArtifact tempFolder is never removed. + + let cacheRoot: string; + let customTemp: string; + + beforeEach(async () => { + cacheRoot = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'cache-')); + customTemp = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'custom-temp-')); + }); + + afterEach(async () => { + await fs.promises.rm(cacheRoot, { recursive: true, force: true }); + await fs.promises.rm(customTemp, { recursive: true, force: true }); + }); + + it('should not leave empty electron-download-* directories behind', async () => { + const downloader = new FixtureDownloader(); + + const before = await fs.promises.readdir(customTemp); + expect(before).toEqual([]); + + const artifactPath = await downloadArtifact({ + artifactName: 'electron', + version: '2.0.9', + platform: 'darwin', + arch: 'x64', + cacheRoot, + tempDirectory: customTemp, + downloader, + cacheMode: ElectronDownloadCacheMode.Bypass, + }); + + // The caller owns the output directory and is expected to clean it up. + await fs.promises.rm(path.dirname(artifactPath), { recursive: true, force: true }); + + // After the caller has cleaned up their artifact, nothing else should + // remain in the custom temp directory. With the bug, an empty + // `electron-download-XXXXXX` directory created by validateArtifact() + // is left behind. + const after = await fs.promises.readdir(customTemp); + expect(after).toEqual([]); + }); +}); From fa6704aed302eaa4cfa458a45f84dee3603a8ab5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 08:27:12 +0000 Subject: [PATCH 2/9] fix: use createRequire to load global-agent in ESM context The package is published as ESM ("type": "module"), so bare require() is undefined at runtime. The try/catch swallowed the ReferenceError, causing proxy support to silently never work. Define require via createRequire(import.meta.url) so the optional dependency can be loaded synchronously as before. --- src/proxy.ts | 3 +++ test/bugs.spec.ts | 10 +++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/proxy.ts b/src/proxy.ts index 7f5bfc67..732e3953 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -1,7 +1,10 @@ +import { createRequire } from 'node:module'; + import debug from 'debug'; import { getEnv, setEnv } from './utils.js'; const d = debug('@electron/get:proxy'); +const require = createRequire(import.meta.url); /** * Initializes a third-party proxy module for HTTP(S) requests. Call this function before diff --git a/test/bugs.spec.ts b/test/bugs.spec.ts index dcae29ce..89113716 100644 --- a/test/bugs.spec.ts +++ b/test/bugs.spec.ts @@ -156,12 +156,12 @@ describe('Bug: initializeProxy uses require() in an ESM module', () => { // Output (with DEBUG=@electron/get:proxy): // ReferenceError: require is not defined - it('documents the require-in-ESM bug (cannot reproduce under vitest)', () => { + it('defines require via createRequire before using it', () => { const source = fs.readFileSync(path.resolve(__dirname, '../src/proxy.ts'), 'utf8'); - // This assertion fails while the bug exists: the source still uses - // bare `require(` which is undefined in ESM. The fix is to use - // `createRequire(import.meta.url)` or a dynamic `import()`. - expect(source).not.toMatch(/\brequire\(['"]global-agent['"]\)/); + // The source calls `require('global-agent')`; in ESM that identifier must + // be explicitly defined via `createRequire(import.meta.url)` (or the call + // replaced with a dynamic `import()`). + expect(source).toMatch(/createRequire\s*\(\s*import\.meta\.url\s*\)/); }); }); From 26771be35b1cb00bebfcad61f51dc99f98a12d51 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 08:27:30 +0000 Subject: [PATCH 3/9] fix: correct progress bar suppression condition The condition used || instead of &&, so neither `quiet: true` alone nor ELECTRON_GET_NO_PROGRESS alone suppressed the progress bar. Both had to be set simultaneously, contradicting the documented behavior. --- src/GotDownloader.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GotDownloader.ts b/src/GotDownloader.ts index e5438fe5..1872a2f7 100644 --- a/src/GotDownloader.ts +++ b/src/GotDownloader.ts @@ -49,7 +49,7 @@ export class GotDownloader implements Downloader { await fs.promises.mkdir(path.dirname(targetFilePath), { recursive: true }); const writeStream = fs.createWriteStream(targetFilePath); - if (!quiet || !process.env.ELECTRON_GET_NO_PROGRESS) { + if (!quiet && !process.env.ELECTRON_GET_NO_PROGRESS) { const start = new Date(); timeout = setTimeout(() => { if (!downloadCompleted) { From 60be031cdaa43ed1dbd592465373348cc545ed1f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 08:27:53 +0000 Subject: [PATCH 4/9] fix: clear progress timer on download failure If pipeline() threw, the 30-second progress-bar timer was never cleared and would fire after the download had already failed, rendering a progress bar for a dead download. Move the cleanup into a finally block. --- src/GotDownloader.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/GotDownloader.ts b/src/GotDownloader.ts index 1872a2f7..544733be 100644 --- a/src/GotDownloader.ts +++ b/src/GotDownloader.ts @@ -83,11 +83,11 @@ export class GotDownloader implements Downloader { error.message += ` for ${(error as HTTPError).response.url}`; } throw error; - } - - downloadCompleted = true; - if (timeout) { - clearTimeout(timeout); + } finally { + downloadCompleted = true; + if (timeout) { + clearTimeout(timeout); + } } } } From 5ff0f00f1fd350161be37bf19e31bcaf37ab03a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 08:28:05 +0000 Subject: [PATCH 5/9] fix: propagate tempDirectory to SHASUMS256.txt download The nested downloadArtifact call for checksum validation forwarded most options but omitted tempDirectory, so SHASUMS256.txt was always written under os.tmpdir() regardless of the caller's configuration. This breaks setups where os.tmpdir() is read-only or on a different filesystem. --- src/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/index.ts b/src/index.ts index 1ce46baf..51c1b7af 100644 --- a/src/index.ts +++ b/src/index.ts @@ -82,6 +82,7 @@ async function validateArtifact( cacheRoot: artifactDetails.cacheRoot, downloader: artifactDetails.downloader, mirrorOptions: artifactDetails.mirrorOptions, + tempDirectory: artifactDetails.tempDirectory, // Never use the cache for loading checksums, load // them fresh every time cacheMode: ElectronDownloadCacheMode.Bypass, From 4cbf1aefa7791c8f0a9ae2f8e8ab369216be9348 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 08:28:35 +0000 Subject: [PATCH 6/9] fix: always clean up validateArtifact temp directory validateArtifact created its temp directory with ORPHAN mode when the caller owned the output (ReadOnly/Bypass cache modes), but the validation tempFolder never needs to outlive the function. When checksums were not provided, the inner finally block deleted the SHASUMS256 download directory instead, leaving an empty electron-download-* directory behind on every ReadOnly/Bypass download. Always use CLEAN mode for the validation tempFolder. --- src/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/index.ts b/src/index.ts index 51c1b7af..93c77407 100644 --- a/src/index.ts +++ b/src/index.ts @@ -115,9 +115,7 @@ async function validateArtifact( } } }, - doesCallerOwnTemporaryOutput(effectiveCacheMode(artifactDetails)) - ? TempDirCleanUpMode.ORPHAN - : TempDirCleanUpMode.CLEAN, + TempDirCleanUpMode.CLEAN, ); } From 78f5b383d882103f6fadeb2d63d7a6c4834fa805 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 08:29:38 +0000 Subject: [PATCH 7/9] refactor: replace deprecated url.parse with WHATWG URL url.parse() and url.format() (legacy) are deprecated since Node.js 11. Switch to the WHATWG URL API. The new implementation produces identical cache-directory hashes for all real-world Electron download URLs (verified against github.com, nightlies, ports, and userinfo). Only the synthetic test fixture URL with no path component hashes differently, so the hardcoded expected hash in Cache.spec.ts is updated. --- src/Cache.ts | 11 +++++------ test/Cache.spec.ts | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Cache.ts b/src/Cache.ts index 011bd25d..d8de8f4d 100644 --- a/src/Cache.ts +++ b/src/Cache.ts @@ -5,7 +5,6 @@ import fs from 'graceful-fs'; import crypto from 'node:crypto'; import path from 'node:path'; import { pipeline } from 'node:stream/promises'; -import url from 'node:url'; const d = debug('@electron/get:cache'); @@ -17,12 +16,12 @@ export class Cache { constructor(private cacheRoot = defaultCacheRoot) {} public static getCacheDirectory(downloadUrl: string): string { - const parsedDownloadUrl = url.parse(downloadUrl); - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { search, hash, pathname, ...rest } = parsedDownloadUrl; - const strippedUrl = url.format({ ...rest, pathname: path.dirname(pathname || 'electron') }); + const parsed = new URL(downloadUrl); + parsed.hash = ''; + parsed.search = ''; + parsed.pathname = path.posix.dirname(parsed.pathname); - return crypto.createHash('sha256').update(strippedUrl).digest('hex'); + return crypto.createHash('sha256').update(parsed.toString()).digest('hex'); } public getCachePath(downloadUrl: string, fileName: string): string { diff --git a/test/Cache.spec.ts b/test/Cache.spec.ts index ccf5445d..3cb98508 100644 --- a/test/Cache.spec.ts +++ b/test/Cache.spec.ts @@ -12,7 +12,7 @@ describe('Cache', () => { let cache: Cache; const dummyUrl = 'dummy://dummypath'; - const sanitizedDummyUrl = '0c57d948bd4829db99d75c3b4a5d6836c37bc335f38012981baf5d1193b5a612'; + const sanitizedDummyUrl = 'a1f9d38d51b311ef8e4a0accc1c4d98b8b17b8b7469c5dc987b6f7ba9b6ff350'; beforeEach(async () => { cacheDir = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'electron-download-spec-')); From b7f983d5d3c044a828445c2baee02cf2992e819e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 08:30:23 +0000 Subject: [PATCH 8/9] test: reorganize new tests into appropriately named files Split the tests from bugs.spec.ts into: - test/GotDownloader.spec.ts (progress bar and timer cleanup) - test/proxy.spec.ts (createRequire guard) - test/index.spec.ts (tempDirectory handling, added to existing downloadArtifact describe block) --- test/GotDownloader.spec.ts | 100 ++++++++++++++ test/bugs.spec.ts | 271 ------------------------------------- test/index.spec.ts | 64 +++++++++ test/proxy.spec.ts | 15 ++ 4 files changed, 179 insertions(+), 271 deletions(-) create mode 100644 test/GotDownloader.spec.ts delete mode 100644 test/bugs.spec.ts create mode 100644 test/proxy.spec.ts diff --git a/test/GotDownloader.spec.ts b/test/GotDownloader.spec.ts new file mode 100644 index 00000000..5948212f --- /dev/null +++ b/test/GotDownloader.spec.ts @@ -0,0 +1,100 @@ +import fs from 'graceful-fs'; +import os from 'node:os'; +import path from 'node:path'; + +import { PathLike } from 'node:fs'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { GotDownloader } from '../src/GotDownloader'; + +async function flushMicrotasks(): Promise { + for (let i = 0; i < 10; i++) { + await Promise.resolve(); + } +} + +describe('GotDownloader', () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'got-spec-')); + }); + + afterEach(async () => { + await fs.promises.rm(tmpDir, { recursive: true, force: true }); + delete process.env.ELECTRON_GET_NO_PROGRESS; + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + describe('progress bar suppression', () => { + it('should not schedule a progress bar timer when quiet: true', async () => { + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + vi.useFakeTimers(); + + const downloader = new GotDownloader(); + const target = path.resolve(tmpDir, 'out.txt'); + + const p = downloader.download('http://127.0.0.1:1/nope', target, { quiet: true }); + p.catch(() => { + /* ignore */ + }); + + await flushMicrotasks(); + + expect(vi.getTimerCount()).toBe(0); + + vi.useRealTimers(); + await p.catch(() => { + /* ignore */ + }); + }); + + it('should not schedule a progress bar timer when ELECTRON_GET_NO_PROGRESS is set', async () => { + process.env.ELECTRON_GET_NO_PROGRESS = '1'; + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + vi.useFakeTimers(); + + const downloader = new GotDownloader(); + const target = path.resolve(tmpDir, 'out.txt'); + + const p = downloader.download('http://127.0.0.1:1/nope', target); + p.catch(() => { + /* ignore */ + }); + + await flushMicrotasks(); + + expect(vi.getTimerCount()).toBe(0); + + vi.useRealTimers(); + await p.catch(() => { + /* ignore */ + }); + }); + }); + + describe('timer cleanup', () => { + it('should clear the progress timer even when the download fails', async () => { + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + + const realCreateWriteStream = fs.createWriteStream; + vi.spyOn(fs, 'createWriteStream').mockImplementationOnce((p: PathLike) => { + const stream = realCreateWriteStream(p); + setImmediate(() => stream.emit('error', new Error('boom'))); + return stream; + }); + + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); + + const downloader = new GotDownloader(); + const target = path.resolve(tmpDir, 'out.txt'); + + await expect(downloader.download('http://127.0.0.1:1/nope', target)).rejects.toThrow(); + + expect(vi.getTimerCount()).toBe(0); + + vi.clearAllTimers(); + }); + }); +}); diff --git a/test/bugs.spec.ts b/test/bugs.spec.ts deleted file mode 100644 index 89113716..00000000 --- a/test/bugs.spec.ts +++ /dev/null @@ -1,271 +0,0 @@ -import fs from 'graceful-fs'; -import os from 'node:os'; -import path from 'node:path'; - -import { PathLike } from 'node:fs'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; - -import { GotDownloader } from '../src/GotDownloader'; -import { downloadArtifact } from '../src'; -import { ElectronDownloadCacheMode } from '../src/types'; -import { FixtureDownloader } from './FixtureDownloader'; - -async function flushMicrotasks(): Promise { - for (let i = 0; i < 10; i++) { - await Promise.resolve(); - } -} - -describe('Bug: GotDownloader progress bar suppression', () => { - // The documentation for `quiet` says: - // "if `true`, disables the console progress bar (setting the - // `ELECTRON_GET_NO_PROGRESS` environment variable to a non-empty value - // also does this)." - // - // The condition at src/GotDownloader.ts:52 is: - // if (!quiet || !process.env.ELECTRON_GET_NO_PROGRESS) { - // - // With `||`, the progress timer is scheduled whenever EITHER side is - // truthy, meaning you need BOTH `quiet: true` AND the env var set to - // suppress it. The correct operator is `&&`. - - let tmpDir: string; - - beforeEach(async () => { - tmpDir = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'got-bug-')); - }); - - afterEach(async () => { - await fs.promises.rm(tmpDir, { recursive: true, force: true }); - delete process.env.ELECTRON_GET_NO_PROGRESS; - vi.useRealTimers(); - vi.restoreAllMocks(); - }); - - it('should not schedule a progress bar timer when quiet: true', async () => { - vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); - vi.useFakeTimers(); - - const downloader = new GotDownloader(); - const target = path.resolve(tmpDir, 'out.txt'); - - const p = downloader.download('http://127.0.0.1:1/nope', target, { quiet: true }); - p.catch(() => { - /* ignore */ - }); - - // Let the `await fs.promises.mkdir()` inside download() resolve so the - // progress-bar scheduling code runs. - await flushMicrotasks(); - - // With the bug: a 30-second timer IS scheduled because - // !quiet || !env -> false || true -> true - // Expected: no timer scheduled. - expect(vi.getTimerCount()).toBe(0); - - vi.useRealTimers(); - await p.catch(() => { - /* ignore */ - }); - }); - - it('should not schedule a progress bar timer when ELECTRON_GET_NO_PROGRESS is set', async () => { - process.env.ELECTRON_GET_NO_PROGRESS = '1'; - vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); - vi.useFakeTimers(); - - const downloader = new GotDownloader(); - const target = path.resolve(tmpDir, 'out.txt'); - - const p = downloader.download('http://127.0.0.1:1/nope', target); - p.catch(() => { - /* ignore */ - }); - - await flushMicrotasks(); - - // With the bug: a 30-second timer IS scheduled because - // !quiet || !env -> true || false -> true - // Expected: no timer scheduled. - expect(vi.getTimerCount()).toBe(0); - - vi.useRealTimers(); - await p.catch(() => { - /* ignore */ - }); - }); -}); - -describe('Bug: GotDownloader timer leak on download error', () => { - // At src/GotDownloader.ts:79-91, if `pipeline()` throws, the error is - // re-thrown before `clearTimeout(timeout)` runs. The 30-second timer - // remains armed and will fire, creating a progress bar for a download - // that already failed. The cleanup should be in a `finally` block. - - let tmpDir: string; - - beforeEach(async () => { - tmpDir = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'got-bug-')); - }); - - afterEach(async () => { - await fs.promises.rm(tmpDir, { recursive: true, force: true }); - vi.useRealTimers(); - vi.restoreAllMocks(); - }); - - it('should clear the progress timer even when the download fails', async () => { - vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); - - const realCreateWriteStream = fs.createWriteStream; - vi.spyOn(fs, 'createWriteStream').mockImplementationOnce((p: PathLike) => { - const stream = realCreateWriteStream(p); - setImmediate(() => stream.emit('error', new Error('boom'))); - return stream; - }); - - vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); - - const downloader = new GotDownloader(); - const target = path.resolve(tmpDir, 'out.txt'); - - await expect(downloader.download('http://127.0.0.1:1/nope', target)).rejects.toThrow(); - - // With the bug: the 30-second progress-bar timer is still armed after - // the download has already rejected. - // Expected: all timers scheduled by download() are cleared on failure. - expect(vi.getTimerCount()).toBe(0); - - vi.clearAllTimers(); - }); -}); - -describe('Bug: initializeProxy uses require() in an ESM module', () => { - // src/proxy.ts:36 calls `require('global-agent')`. Since package.json - // declares `"type": "module"`, `require` is not defined at runtime in the - // built `dist/` output. The try/catch swallows the ReferenceError, so - // proxy support silently never works. - // - // NOTE: vitest injects a `require` shim into the test environment, so this - // bug does NOT reproduce under vitest. It only manifests when the built - // package is imported by plain Node.js. Run the built output directly to - // observe the ReferenceError: - // - // yarn build && node -e "import('./dist/proxy.js').then(m => m.initializeProxy())" - // - // Output (with DEBUG=@electron/get:proxy): - // ReferenceError: require is not defined - - it('defines require via createRequire before using it', () => { - const source = fs.readFileSync(path.resolve(__dirname, '../src/proxy.ts'), 'utf8'); - // The source calls `require('global-agent')`; in ESM that identifier must - // be explicitly defined via `createRequire(import.meta.url)` (or the call - // replaced with a dynamic `import()`). - expect(source).toMatch(/createRequire\s*\(\s*import\.meta\.url\s*\)/); - }); -}); - -describe('Bug: SHASUMS256.txt download ignores custom tempDirectory', () => { - // When validateArtifact() downloads SHASUMS256.txt (src/index.ts:77-88), - // it does not forward `artifactDetails.tempDirectory` to the nested - // downloadArtifact call. The checksum file is therefore written to - // os.tmpdir() even when the caller explicitly requested a different - // temp directory (e.g. because os.tmpdir() is read-only or on a - // different filesystem). - - let cacheRoot: string; - let customTemp: string; - - beforeEach(async () => { - cacheRoot = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'cache-')); - customTemp = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'custom-temp-')); - }); - - afterEach(async () => { - await fs.promises.rm(cacheRoot, { recursive: true, force: true }); - await fs.promises.rm(customTemp, { recursive: true, force: true }); - }); - - it('should use the custom tempDirectory for the SHASUMS256.txt download', async () => { - const shasumsTargetDirs: string[] = []; - - const trackingDownloader = { - async download(url: string, targetFilePath: string): Promise { - if (url.endsWith('SHASUMS256.txt')) { - shasumsTargetDirs.push(path.dirname(targetFilePath)); - } - return new FixtureDownloader().download(url, targetFilePath); - }, - }; - - await downloadArtifact({ - artifactName: 'electron', - version: '2.0.9', - platform: 'darwin', - arch: 'x64', - cacheRoot, - tempDirectory: customTemp, - downloader: trackingDownloader, - cacheMode: ElectronDownloadCacheMode.WriteOnly, - }); - - expect(shasumsTargetDirs.length).toBeGreaterThan(0); - for (const dir of shasumsTargetDirs) { - // With the bug: SHASUMS256.txt is downloaded under os.tmpdir(), not customTemp. - // Expected: all temp paths should be under the user-supplied tempDirectory. - expect(dir.startsWith(customTemp)).toBe(true); - } - }); -}); - -describe('Bug: validateArtifact leaks its temp directory in ORPHAN mode', () => { - // validateArtifact() (src/index.ts:47-121) creates its own temp directory - // via withTempDirectoryIn(). When the outer cacheMode makes - // doesCallerOwnTemporaryOutput() true (ReadOnly or Bypass), it passes - // TempDirCleanUpMode.ORPHAN, relying on the `finally` at line 113 to - // clean up. But when checksums are NOT provided, shasumPath lives in a - // *different* temp directory (created by the nested downloadArtifact for - // SHASUMS256.txt), so the `finally` deletes that other directory and the - // validateArtifact tempFolder is never removed. - - let cacheRoot: string; - let customTemp: string; - - beforeEach(async () => { - cacheRoot = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'cache-')); - customTemp = await fs.promises.mkdtemp(path.resolve(os.tmpdir(), 'custom-temp-')); - }); - - afterEach(async () => { - await fs.promises.rm(cacheRoot, { recursive: true, force: true }); - await fs.promises.rm(customTemp, { recursive: true, force: true }); - }); - - it('should not leave empty electron-download-* directories behind', async () => { - const downloader = new FixtureDownloader(); - - const before = await fs.promises.readdir(customTemp); - expect(before).toEqual([]); - - const artifactPath = await downloadArtifact({ - artifactName: 'electron', - version: '2.0.9', - platform: 'darwin', - arch: 'x64', - cacheRoot, - tempDirectory: customTemp, - downloader, - cacheMode: ElectronDownloadCacheMode.Bypass, - }); - - // The caller owns the output directory and is expected to clean it up. - await fs.promises.rm(path.dirname(artifactPath), { recursive: true, force: true }); - - // After the caller has cleaned up their artifact, nothing else should - // remain in the custom temp directory. With the bug, an empty - // `electron-download-XXXXXX` directory created by validateArtifact() - // is left behind. - const after = await fs.promises.readdir(customTemp); - expect(after).toEqual([]); - }); -}); diff --git a/test/index.spec.ts b/test/index.spec.ts index ef2fbe25..cc215ce4 100644 --- a/test/index.spec.ts +++ b/test/index.spec.ts @@ -271,6 +271,70 @@ describe('Public API', () => { expect(await util.promisify(fs.readFile)(driverPath2, 'utf8')).toEqual('cached content'); }); + describe('tempDirectory', () => { + let customTemp: string; + + beforeEach(async () => { + customTemp = await fs.promises.mkdtemp( + path.resolve(os.tmpdir(), 'electron-download-spec-temp-'), + ); + }); + + afterEach(async () => { + await fs.promises.rm(customTemp, { recursive: true, force: true }); + }); + + it('should use the custom tempDirectory for the SHASUMS256.txt download', async () => { + const shasumsTargetDirs: string[] = []; + + const trackingDownloader = { + async download(url: string, targetFilePath: string): Promise { + if (url.endsWith('SHASUMS256.txt')) { + shasumsTargetDirs.push(path.dirname(targetFilePath)); + } + return downloader.download(url, targetFilePath); + }, + }; + + await downloadArtifact({ + artifactName: 'electron', + version: '2.0.9', + platform: 'darwin', + arch: 'x64', + cacheRoot, + tempDirectory: customTemp, + downloader: trackingDownloader, + cacheMode: ElectronDownloadCacheMode.WriteOnly, + }); + + expect(shasumsTargetDirs.length).toBeGreaterThan(0); + for (const dir of shasumsTargetDirs) { + expect(dir.startsWith(customTemp)).toBe(true); + } + }); + + it('should not leak temp directories when cacheMode=Bypass', async () => { + const before = await fs.promises.readdir(customTemp); + expect(before).toEqual([]); + + const artifactPath = await downloadArtifact({ + artifactName: 'electron', + version: '2.0.9', + platform: 'darwin', + arch: 'x64', + cacheRoot, + tempDirectory: customTemp, + downloader, + cacheMode: ElectronDownloadCacheMode.Bypass, + }); + + await fs.promises.rm(path.dirname(artifactPath), { recursive: true, force: true }); + + const after = await fs.promises.readdir(customTemp); + expect(after).toEqual([]); + }); + }); + describe('sumchecker', () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/test/proxy.spec.ts b/test/proxy.spec.ts new file mode 100644 index 00000000..8c96b861 --- /dev/null +++ b/test/proxy.spec.ts @@ -0,0 +1,15 @@ +import fs from 'graceful-fs'; +import path from 'node:path'; + +import { describe, expect, it } from 'vitest'; + +describe('initializeProxy', () => { + // `require` is not defined in ESM modules. vitest injects a shim, so the + // runtime failure only manifests when running the built package under + // plain Node.js. Guard against regressions by checking the source uses + // createRequire(import.meta.url) to define require before calling it. + it('defines require via createRequire before using it', () => { + const source = fs.readFileSync(path.resolve(__dirname, '../src/proxy.ts'), 'utf8'); + expect(source).toMatch(/createRequire\s*\(\s*import\.meta\.url\s*\)/); + }); +}); From e2a6e91f8fc37bc5305c6395c2a9199c1cfef5cd Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 25 Mar 2026 23:25:19 +0000 Subject: [PATCH 9/9] test: add stable cache-key fixture for real download URL Lock in the cache-directory hash for a real Electron release URL so any future URL-parsing change that would invalidate on-disk caches fails this test instead of silently shipping. --- test/Cache.spec.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/Cache.spec.ts b/test/Cache.spec.ts index 3cb98508..8bfd3b48 100644 --- a/test/Cache.spec.ts +++ b/test/Cache.spec.ts @@ -21,6 +21,19 @@ describe('Cache', () => { afterEach(() => fs.promises.rm(cacheDir, { recursive: true, force: true })); + describe('getCacheDirectory()', () => { + it('should produce a stable cache key for real-world download URLs', () => { + // This hash is part of the on-disk cache layout. Changing it invalidates + // every user's existing Electron cache, so any diff here must be + // deliberate and called out. + expect( + Cache.getCacheDirectory( + 'https://github.com/electron/electron/releases/download/v2.0.9/electron-v2.0.9-darwin-x64.zip', + ), + ).toEqual('7658513ccebc15fd4c4dec9ff8cdef8eb586f7fe5bad56f08b218650df37b1b6'); + }); + }); + describe('getCachePath()', () => { it('should strip the hash and query params off the url', async () => { const firstUrl = 'https://example.com?foo=1';