Skip to content

Commit 27f869b

Browse files
test(ec2): fix flaky windows test. (#5661)
## Problem The test case here: https://github.com/aws/aws-toolkit-vscode/blob/359b1d8252a32e064482678110c5931630bd49e0/packages/core/src/test/awsService/ec2/model.test.ts#L132C9-L146C11 This test is unaware of the side-effect that it will attempt to explicitly delete the keys when generating. Originally, the delete was conditional on the keys existing in the directory, so this test case was not a problem since they never existed. However, on windows that check was faulty, leading to the case where the windows file system reported that the keys existed, which lead it to attempting to delete the root directory. ## Solution - Modify `tryRun` to accept an `onStdOut` optional parameter that lets us accept the overwrite prompt from `ssh-keygen`. - Do not explicitly delete the files in the generation function, make this part of the generation step. The benefits of this approach are that we moved the side-effects to a place where they are expected, in the key generation process itself. It also inherently avoids the potentially faulty check if the key files exist, as we only use `onStdOut` if when the keys exist. ### Notes - `packages/core/src/awsService/ec2/sshKeyPair.ts:49` includes `any` which should be `ChildProcess`. For some reason the import fails in CI, so I left it as `any`. Refer to `attempt to import child process` commit for logs. --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Justin M. Keyes <jmkeyes@amazon.com>
1 parent de3c246 commit 27f869b

File tree

5 files changed

+19
-13
lines changed

5 files changed

+19
-13
lines changed

packages/core/src/awsService/ec2/sshKeyPair.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { ToolkitError } from '../../shared/errors'
88
import { tryRun } from '../../shared/utilities/pathFind'
99
import { Timeout } from '../../shared/utilities/timeoutUtils'
1010
import { findAsync } from '../../shared/utilities/collectionUtils'
11+
import { RunParameterContext } from '../../shared/utilities/processUtils'
1112

1213
type sshKeyType = 'rsa' | 'ed25519'
1314

@@ -17,7 +18,7 @@ export class SshKeyPair {
1718
private deleted: boolean = false
1819

1920
private constructor(
20-
private keyPath: string,
21+
private readonly keyPath: string,
2122
lifetime: number
2223
) {
2324
this.publicKeyPath = `${keyPath}.pub`
@@ -29,10 +30,6 @@ export class SshKeyPair {
2930
}
3031

3132
public static async getSshKeyPair(keyPath: string, lifetime: number) {
32-
// Overwrite key if already exists
33-
if (await fs.existsFile(keyPath)) {
34-
await fs.delete(keyPath)
35-
}
3633
await SshKeyPair.generateSshKeyPair(keyPath)
3734
return new SshKeyPair(keyPath, lifetime)
3835
}
@@ -50,7 +47,12 @@ export class SshKeyPair {
5047
* @param keyType type of key to generate.
5148
*/
5249
public static async tryKeyGen(keyPath: string, keyType: sshKeyType): Promise<boolean> {
53-
return !(await tryRun('ssh-keygen', ['-t', keyType, '-N', '', '-q', '-f', keyPath], 'yes', 'unknown key type'))
50+
const overrideKeys = async (_t: string, proc: RunParameterContext) => {
51+
await proc.send('yes')
52+
}
53+
return !(await tryRun('ssh-keygen', ['-t', keyType, '-N', '', '-q', '-f', keyPath], 'yes', 'unknown key type', {
54+
onStdout: overrideKeys,
55+
}))
5456
}
5557

5658
public static async tryKeyTypes(keyPath: string, keyTypes: sshKeyType[]): Promise<boolean> {

packages/core/src/shared/utilities/pathFind.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import * as vscode from 'vscode'
77
import * as path from 'path'
88
import fs from '../../shared/fs/fs'
9-
import { ChildProcess } from './processUtils'
9+
import { ChildProcess, ChildProcessOptions } from './processUtils'
1010
import { GitExtension } from '../extensions/git'
1111
import { Settings } from '../settings'
1212
import { getLogger } from '../logger/logger'
@@ -30,10 +30,11 @@ export async function tryRun(
3030
p: string,
3131
args: string[],
3232
logging: 'yes' | 'no' | 'noresult' = 'yes',
33-
expected?: string
33+
expected?: string,
34+
opt?: ChildProcessOptions
3435
): Promise<boolean> {
3536
const proc = new ChildProcess(p, args, { logging: 'no' })
36-
const r = await proc.run()
37+
const r = await proc.run(opt)
3738
const ok = r.exitCode === 0 && (expected === undefined || r.stdout.includes(expected))
3839
if (logging === 'noresult') {
3940
getLogger().info('tryRun: %s: %s', ok ? 'ok' : 'failed', proc)

packages/core/src/shared/utilities/processUtils.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,13 @@ import * as logger from '../logger'
99
import { Timeout, CancellationError, waitUntil } from './timeoutUtils'
1010
import { isWeb } from '../extensionGlobals'
1111

12-
interface RunParameterContext {
12+
export interface RunParameterContext {
1313
/** Reports an error parsed from the stdin/stdout streams. */
1414
reportError(err: string | Error): void
1515
/** Attempts to stop the running process. See {@link ChildProcess.stop}. */
1616
stop(force?: boolean, signal?: string): void
17+
/** Send string to stdin */
18+
send(text: string): Promise<void>
1719
/** The active `Timeout` object (if applicable). */
1820
readonly timeout?: Timeout
1921
/** The logger being used by the process. */
@@ -161,6 +163,7 @@ export class ChildProcess {
161163
timeout,
162164
logger: this.#log,
163165
stop: this.stop.bind(this),
166+
send: this.send.bind(this),
164167
reportError: (err) => errorHandler(err instanceof Error ? err : new Error(err)),
165168
}
166169

packages/core/src/test/awsService/ec2/model.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,8 @@ describe('Ec2ConnectClient', function () {
139139
instanceId: 'test-id',
140140
region: 'test-region',
141141
}
142-
const mockKeys = await SshKeyPair.getSshKeyPair('', 30000)
143-
await client.sendSshKeyToInstance(testSelection, mockKeys, '')
142+
const mockKeys = await SshKeyPair.getSshKeyPair('fakeDir', 30000)
143+
await client.sendSshKeyToInstance(testSelection, mockKeys, 'test-user')
144144
sinon.assert.calledWith(sendCommandStub, testSelection.instanceId, 'AWS-RunShellScript')
145145
sinon.restore()
146146
})

packages/core/src/test/awsService/ec2/sshKeyPair.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ describe('SshKeyUtility', async function () {
3333
})
3434

3535
after(async function () {
36-
await keyPair.delete()
36+
await fs.delete(temporaryDirectory, { recursive: true, force: true })
3737
clock.uninstall()
3838
sinon.restore()
3939
})

0 commit comments

Comments
 (0)