-
Notifications
You must be signed in to change notification settings - Fork 803
fix(desktop): guard installUpdate against repeat clicks on macOS #3549
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| import { beforeEach, describe, expect, mock, test } from "bun:test"; | ||
| import { EventEmitter } from "node:events"; | ||
|
|
||
| class FakeAutoUpdater extends EventEmitter { | ||
| autoDownload = false; | ||
| autoInstallOnAppQuit = false; | ||
| disableDifferentialDownload = false; | ||
| allowDowngrade = false; | ||
| setFeedURL = mock(() => {}); | ||
| checkForUpdates = mock(() => Promise.resolve(null)); | ||
| quitAndInstall = mock(() => {}); | ||
| } | ||
|
|
||
| const fakeAutoUpdater = new FakeAutoUpdater(); | ||
|
|
||
| mock.module("electron-updater", () => ({ | ||
| autoUpdater: fakeAutoUpdater, | ||
| })); | ||
|
|
||
| mock.module("electron", () => ({ | ||
| app: { | ||
| getPath: mock(() => ""), | ||
| getName: mock(() => "test-app"), | ||
| getVersion: mock(() => "1.0.0"), | ||
| getAppPath: mock(() => ""), | ||
| isPackaged: false, | ||
| isReady: mock(() => true), | ||
| whenReady: mock(() => Promise.resolve()), | ||
| }, | ||
| dialog: { | ||
| showMessageBox: mock(() => Promise.resolve({ response: 0 })), | ||
| }, | ||
| })); | ||
|
|
||
| mock.module("main/index", () => ({ | ||
| setSkipQuitConfirmation: mock(() => {}), | ||
| })); | ||
|
|
||
| const autoUpdater = await import("./auto-updater"); | ||
| const { AUTO_UPDATE_STATUS } = await import("shared/auto-update"); | ||
|
|
||
| describe("installUpdate", () => { | ||
| beforeEach(() => { | ||
| fakeAutoUpdater.removeAllListeners(); | ||
| fakeAutoUpdater.quitAndInstall.mockClear(); | ||
| fakeAutoUpdater.checkForUpdates.mockClear(); | ||
| fakeAutoUpdater.setFeedURL.mockClear(); | ||
| autoUpdater.setupAutoUpdater(); | ||
| // The module is a singleton; emit an error to reset isInstalling | ||
| // between tests (the error handler clears it). | ||
| fakeAutoUpdater.emit("error", new Error("reset")); | ||
| }); | ||
|
Comment on lines
+48
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On a Windows CI runner:
To make the tests portable, mock the platform dependency. One approach is to mock mock.module("shared/constants", () => ({
PLATFORM: { IS_MAC: true, IS_LINUX: false, IS_WINDOWS: false },
}));Alternatively, mock Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/main/lib/auto-updater.test.ts
Line: 42-52
Comment:
**Tests silently depend on the runtime platform**
`setupAutoUpdater()` early-returns if `IS_AUTO_UPDATE_PLATFORM` is false (i.e. `process.platform !== 'darwin' && process.platform !== 'linux'`). `shared/constants` is not mocked in the test file, so `PLATFORM` is resolved from the actual host OS at module-load time.
On a Windows CI runner:
- `setupAutoUpdater()` exits before registering the `update-downloaded` and `error` handlers.
- `fakeAutoUpdater.emit("error", new Error("reset"))` becomes a no-op — `isInstalling` is never reset.
- `fakeAutoUpdater.emit("update-downloaded", ...)` becomes a no-op — `currentStatus` never reaches `READY`.
- Test 2 ("collapses repeat install clicks") asserts `quitAndInstall` was called once, but it's called zero times → **failure**.
- Test 3 ("clears the in-flight guard") has the same problem.
To make the tests portable, mock the platform dependency. One approach is to mock `shared/constants`:
```ts
mock.module("shared/constants", () => ({
PLATFORM: { IS_MAC: true, IS_LINUX: false, IS_WINDOWS: false },
}));
```
Alternatively, mock `main/env.main` so `NODE_ENV` is always `"test"` regardless of the environment, since that early-return path also affects `installUpdate()` correctness in the tests.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| test("ignores install requests when no update is ready", () => { | ||
| expect(autoUpdater.getUpdateStatus().status).not.toBe( | ||
| AUTO_UPDATE_STATUS.READY, | ||
| ); | ||
|
|
||
| autoUpdater.installUpdate(); | ||
|
|
||
| expect(fakeAutoUpdater.quitAndInstall).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("collapses repeat install clicks into a single quitAndInstall call", () => { | ||
| fakeAutoUpdater.emit("update-downloaded", { version: "9.9.9" }); | ||
| expect(autoUpdater.getUpdateStatus().status).toBe(AUTO_UPDATE_STATUS.READY); | ||
|
|
||
| autoUpdater.installUpdate(); | ||
| autoUpdater.installUpdate(); | ||
| autoUpdater.installUpdate(); | ||
|
|
||
| expect(fakeAutoUpdater.quitAndInstall).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test("clears the in-flight guard when Squirrel surfaces an error", () => { | ||
| fakeAutoUpdater.emit("update-downloaded", { version: "9.9.9" }); | ||
| autoUpdater.installUpdate(); | ||
| expect(fakeAutoUpdater.quitAndInstall).toHaveBeenCalledTimes(1); | ||
|
|
||
| fakeAutoUpdater.emit("error", new Error("squirrel failed")); | ||
| fakeAutoUpdater.emit("update-downloaded", { version: "9.9.9" }); | ||
| autoUpdater.installUpdate(); | ||
|
|
||
| expect(fakeAutoUpdater.quitAndInstall).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.