Skip to content

Commit 4ae208b

Browse files
EhabYEhabY
committed
Fix binary manager tests
1 parent ffbdbf8 commit 4ae208b

File tree

3 files changed

+79
-75
lines changed

3 files changed

+79
-75
lines changed

‎src/__mocks__/testHelpers.ts‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,22 @@ export class MockConfigurationProvider {
4040
* Setup the vscode.workspace.getConfiguration mock to return our values
4141
*/
4242
private setupVSCodeMock(): void {
43-
vi.mocked(vscode.workspace.getConfiguration).mockReturnValue({
44-
get: vi.fn((key: string, defaultValue?: unknown) => {
45-
const value = this.config.get(key);
46-
return value !== undefined ? value : defaultValue;
47-
}),
48-
has: vi.fn((key: string) => this.config.has(key)),
49-
inspect: vi.fn(),
50-
update: vi.fn(),
51-
} as unknown as vscode.WorkspaceConfiguration);
43+
vi.mocked(vscode.workspace.getConfiguration).mockImplementation(
44+
(section?: string) =>
45+
({
46+
get: vi.fn((key: string, defaultValue?: unknown) => {
47+
const fullKey = section ? `${section}.${key}` : key;
48+
const value = this.config.get(fullKey);
49+
return value !== undefined ? value : defaultValue;
50+
}),
51+
has: vi.fn((key: string) => {
52+
const fullKey = section ? `${section}.${key}` : key;
53+
return this.config.has(fullKey);
54+
}),
55+
inspect: vi.fn(),
56+
update: vi.fn(),
57+
}) as unknown as vscode.WorkspaceConfiguration,
58+
);
5259
}
5360
}
5461

‎src/core/binaryManager.test.ts‎

Lines changed: 53 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,6 @@ describe("BinaryManager", () => {
136136
expect(mockLogger.warn).toHaveBeenCalledWith(
137137
expect.stringContaining("Unable to get version"),
138138
);
139-
140139
// Should attempt to download now
141140
expect(mockAxios.get).toHaveBeenCalled();
142141
expect(mockLogger.info).toHaveBeenCalledWith(
@@ -150,6 +149,9 @@ describe("BinaryManager", () => {
150149
const result = await manager.fetchBinary(mockApi, "test");
151150
expect(result).toBe(BINARY_PATH);
152151
expect(mockAxios.get).toHaveBeenCalled();
152+
expect(mockLogger.info).toHaveBeenCalledWith(
153+
"No existing binary found, starting download",
154+
);
153155
expect(mockLogger.info).toHaveBeenCalledWith(
154156
"Downloaded binary version is",
155157
TEST_VERSION,
@@ -221,21 +223,8 @@ describe("BinaryManager", () => {
221223
});
222224

223225
it("backs up existing binary before replacement", async () => {
224-
// Setup existing old binary
225-
vi.mocked(cli.stat).mockReset();
226-
vi.mocked(cli.stat)
227-
.mockResolvedValueOnce({ size: 1024 } as fs.Stats) // Existing binary
228-
.mockResolvedValueOnce({ size: 5242880 } as fs.Stats); // After download
229-
vi.mocked(cli.version).mockReset();
230-
vi.mocked(cli.version)
231-
.mockResolvedValueOnce("1.0.0") // Old version
232-
.mockResolvedValueOnce(TEST_VERSION); // New version after download
233-
234-
// Setup download
235-
const stream = createMockStream();
236-
const writeStream = createMockWriteStream();
237-
withHttpResponse(200, { "content-length": "1024" }, stream);
238-
vi.mocked(fs.createWriteStream).mockReturnValue(writeStream);
226+
withExistingBinary("1.0.0");
227+
withSuccessfulDownload();
239228

240229
await manager.fetchBinary(mockApi, "test");
241230
expect(fse.rename).toHaveBeenCalledWith(
@@ -260,6 +249,9 @@ describe("BinaryManager", () => {
260249
withHttpResponse(304);
261250
const result = await manager.fetchBinary(mockApi, "test");
262251
expect(result).toBe(BINARY_PATH);
252+
expect(mockLogger.info).toHaveBeenCalledWith(
253+
"Using existing binary since server returned a 304",
254+
);
263255
});
264256

265257
it("handles 404 platform not supported", async () => {
@@ -268,14 +260,28 @@ describe("BinaryManager", () => {
268260
await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow(
269261
"Platform not supported",
270262
);
271-
expect(vscode.env.openExternal).toHaveBeenCalled();
263+
expect(vscode.env.openExternal).toHaveBeenCalledWith(
264+
expect.objectContaining({
265+
path: expect.stringContaining(
266+
"github.com/coder/vscode-coder/issues/new?",
267+
),
268+
}),
269+
);
272270
});
273271

274272
it("handles server errors", async () => {
275273
withHttpResponse(500);
274+
mockUI.setResponse("Open an Issue");
276275
await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow(
277276
"Failed to download binary",
278277
);
278+
expect(vscode.env.openExternal).toHaveBeenCalledWith(
279+
expect.objectContaining({
280+
path: expect.stringContaining(
281+
"github.com/coder/vscode-coder/issues/new?",
282+
),
283+
}),
284+
);
279285
});
280286
});
281287

@@ -336,35 +342,46 @@ describe("BinaryManager", () => {
336342

337343
describe("Signature Verification", () => {
338344
it("verifies valid signatures", async () => {
339-
withSuccessfulDownloadAndSignature();
340-
vi.mocked(pgp.verifySignature).mockResolvedValueOnce();
345+
withSuccessfulDownload();
346+
withSignatureResponses([200]);
341347
const result = await manager.fetchBinary(mockApi, "test");
342348
expect(result).toBe(BINARY_PATH);
343349
expect(pgp.verifySignature).toHaveBeenCalled();
344350
});
345351

346352
it("tries fallback signature on 404", async () => {
347-
withBinaryDownload();
353+
withSuccessfulDownload();
348354
withSignatureResponses([404, 200]);
349-
vi.mocked(pgp.verifySignature).mockResolvedValueOnce();
350355
mockUI.setResponse("Download signature");
351356
const result = await manager.fetchBinary(mockApi, "test");
352357
expect(result).toBe(BINARY_PATH);
358+
expect(vscode.window.showWarningMessage).toHaveBeenCalledWith(
359+
"Signature not found",
360+
expect.any(Object),
361+
expect.any(String),
362+
expect.any(String),
363+
);
364+
// First download and when verfiying twice (404 then 200)
353365
expect(mockAxios.get).toHaveBeenCalledTimes(3);
354366
});
355367

356368
it("allows running despite invalid signature", async () => {
357-
withSuccessfulDownloadAndSignature();
369+
withSuccessfulDownload();
370+
withSignatureResponses([200]);
358371
vi.mocked(pgp.verifySignature).mockRejectedValueOnce(
359372
createVerificationError("Invalid signature"),
360373
);
361374
mockUI.setResponse("Run anyway");
362375
const result = await manager.fetchBinary(mockApi, "test");
363376
expect(result).toBe(BINARY_PATH);
377+
expect(mockLogger.info).toHaveBeenCalledWith(
378+
"Binary will be ran anyway at user request",
379+
);
364380
});
365381

366382
it("aborts on signature rejection", async () => {
367-
withSuccessfulDownloadAndSignature();
383+
withSuccessfulDownload();
384+
withSignatureResponses([200]);
368385
vi.mocked(pgp.verifySignature).mockRejectedValueOnce(
369386
createVerificationError("Invalid signature"),
370387
);
@@ -380,19 +397,25 @@ describe("BinaryManager", () => {
380397
const result = await manager.fetchBinary(mockApi, "test");
381398
expect(result).toBe(BINARY_PATH);
382399
expect(pgp.verifySignature).not.toHaveBeenCalled();
400+
expect(mockLogger.info).toHaveBeenCalledWith(
401+
"Skipping binary signature verification due to settings",
402+
);
383403
});
384404

385405
it("allows skipping verification on 404", async () => {
386-
withBinaryDownload();
406+
withSuccessfulDownload();
387407
withHttpResponse(404);
388408
mockUI.setResponse("Run without verification");
389409
const result = await manager.fetchBinary(mockApi, "test");
390410
expect(result).toBe(BINARY_PATH);
391411
expect(pgp.verifySignature).not.toHaveBeenCalled();
412+
expect(mockLogger.info).toHaveBeenCalledWith(
413+
expect.stringMatching(/Signature download from (.+) declined/),
414+
);
392415
});
393416

394417
it("handles signature download failure", async () => {
395-
withBinaryDownload();
418+
withSuccessfulDownload();
396419
withHttpResponse(500);
397420
mockUI.setResponse("Run without verification");
398421
const result = await manager.fetchBinary(mockApi, "test");
@@ -406,7 +429,7 @@ describe("BinaryManager", () => {
406429
});
407430

408431
it("aborts when user declines missing signature", async () => {
409-
withBinaryDownload();
432+
withSuccessfulDownload();
410433
withHttpResponse(404);
411434
mockUI.setResponse(undefined); // User cancels
412435
await expect(manager.fetchBinary(mockApi, "test")).rejects.toThrow(
@@ -482,11 +505,12 @@ describe("BinaryManager", () => {
482505
vi.mocked(cli.stat).mockReset();
483506
vi.mocked(cli.stat).mockResolvedValueOnce({ size: 1024 } as fs.Stats); // Existing binary exists
484507
vi.mocked(cli.version).mockReset();
485-
vi.mocked(cli.version)
486-
.mockRejectedValueOnce(new Error("corrupted")) // Existing binary is corrupted
487-
.mockResolvedValueOnce(TEST_VERSION); // New download works
508+
vi.mocked(cli.version).mockRejectedValueOnce(new Error("corrupted")); // Existing binary is corrupted
488509
}
489510

511+
/**
512+
* Shouldn't reset mocks since this method is combined with other mocks.
513+
*/
490514
function withSuccessfulDownload(opts?: {
491515
headers?: Record<string, unknown>;
492516
}) {
@@ -506,25 +530,6 @@ describe("BinaryManager", () => {
506530
vi.mocked(cli.version).mockResolvedValueOnce(TEST_VERSION);
507531
}
508532

509-
function withBinaryDownload() {
510-
const stream = createMockStream();
511-
const writeStream = createMockWriteStream();
512-
withHttpResponse(200, { "content-length": "1024" }, stream);
513-
vi.mocked(fs.createWriteStream).mockReturnValue(writeStream);
514-
// Override to ensure no existing binary initially
515-
vi.mocked(cli.stat).mockReset();
516-
vi.mocked(cli.stat)
517-
.mockResolvedValueOnce(undefined) // No existing binary
518-
.mockResolvedValueOnce({ size: 5242880 } as fs.Stats); // After download
519-
vi.mocked(cli.version).mockReset();
520-
vi.mocked(cli.version).mockResolvedValueOnce(TEST_VERSION);
521-
}
522-
523-
function withSuccessfulDownloadAndSignature() {
524-
withBinaryDownload();
525-
withHttpResponse(200, { "content-length": "256" }, createMockStream());
526-
}
527-
528533
function withSignatureResponses(statuses: number[]) {
529534
statuses.forEach((status) => {
530535
if (status === 200) {

‎src/core/binaryManager.ts‎

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,10 @@ export class BinaryManager {
3535
* downloads being disabled.
3636
*/
3737
public async fetchBinary(restClient: Api, label: string): Promise<string> {
38+
const cfg = vscode.workspace.getConfiguration("coder");
3839
// Settings can be undefined when set to their defaults (true in this case),
3940
// so explicitly check against false.
40-
const enableDownloads =
41-
vscode.workspace.getConfiguration().get("coder.enableDownloads") !==
42-
false;
41+
const enableDownloads = cfg.get("enableDownloads") !== false;
4342
this.output.info("Downloads are", enableDownloads ? "enabled" : "disabled");
4443

4544
// Get the build info to compare with the existing binary version, if any,
@@ -108,9 +107,7 @@ export class BinaryManager {
108107

109108
// Figure out where to get the binary.
110109
const binName = cli.name();
111-
const configSource = vscode.workspace
112-
.getConfiguration()
113-
.get("coder.binarySource");
110+
const configSource = cfg.get("binarySource");
114111
const binSource =
115112
configSource && String(configSource).trim().length > 0
116113
? String(configSource)
@@ -138,11 +135,7 @@ export class BinaryManager {
138135

139136
switch (status) {
140137
case 200: {
141-
if (
142-
vscode.workspace
143-
.getConfiguration()
144-
.get("coder.disableSignatureVerification")
145-
) {
138+
if (cfg.get("disableSignatureVerification")) {
146139
this.output.info(
147140
"Skipping binary signature verification due to settings",
148141
);
@@ -199,7 +192,6 @@ export class BinaryManager {
199192
vscode.window
200193
.showErrorMessage(
201194
"Coder isn't supported for your platform. Please open an issue, we'd love to support it!",
202-
{},
203195
"Open an Issue",
204196
)
205197
.then((value) => {
@@ -212,18 +204,17 @@ export class BinaryManager {
212204
title: `Support the \`${os}-${arch}\` platform`,
213205
body: `I'd like to use the \`${os}-${arch}\` architecture with the VS Code extension.`,
214206
});
215-
const url = vscode.Uri.parse(
207+
const uri = vscode.Uri.parse(
216208
`https://github.com/coder/vscode-coder/issues/new?${params.toString()}`,
217209
);
218-
vscode.env.openExternal(url);
210+
vscode.env.openExternal(uri);
219211
});
220212
throw new Error("Platform not supported");
221213
}
222214
default: {
223215
vscode.window
224216
.showErrorMessage(
225217
"Failed to download binary. Please open an issue.",
226-
{},
227218
"Open an Issue",
228219
)
229220
.then((value) => {
@@ -234,10 +225,10 @@ export class BinaryManager {
234225
title: `Failed to download binary on \`${cli.goos()}-${cli.goarch()}\``,
235226
body: `Received status code \`${status}\` when downloading the binary.`,
236227
});
237-
const url = vscode.Uri.parse(
228+
const uri = vscode.Uri.parse(
238229
`https://github.com/coder/vscode-coder/issues/new?${params.toString()}`,
239230
);
240-
vscode.env.openExternal(url);
231+
vscode.env.openExternal(uri);
241232
});
242233
throw new Error("Failed to download binary");
243234
}
@@ -285,8 +276,9 @@ export class BinaryManager {
285276

286277
const completed = await vscode.window.withProgress<boolean>(
287278
{
288-
title: `Downloading ${baseUrl}`,
289279
location: vscode.ProgressLocation.Notification,
280+
title: `Downloading ${baseUrl}`,
281+
cancellable: true,
290282
},
291283
async (progress, token) => {
292284
const readStream = resp.data as IncomingMessage;

0 commit comments

Comments
 (0)