Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
24919b7
add test for minimum ssm actions
Hweinstock Oct 11, 2024
61f95f0
refactor pathfinder
Hweinstock Oct 11, 2024
12cf3ec
fix tests
Hweinstock Oct 11, 2024
423ca0c
assert called with resulting ssh path
Hweinstock Oct 11, 2024
9a72b73
add ui tests
Hweinstock Oct 11, 2024
b741e83
rename files back
Hweinstock Oct 17, 2024
d6c052b
fix imports
Hweinstock Oct 17, 2024
b2890bd
add more testing
Hweinstock Oct 17, 2024
697f4bc
avoid unnecessary churn
Hweinstock Oct 17, 2024
b47c788
fix imports
Hweinstock Oct 17, 2024
57eb886
avoid circular dependency
Hweinstock Oct 17, 2024
8a1131d
add offset for windows calls
Hweinstock Oct 17, 2024
4071645
move tryRun back
Hweinstock Oct 17, 2024
36c133b
write tests that do not stub fs
Hweinstock Oct 17, 2024
4a55e56
remove duplicate func
Hweinstock Oct 17, 2024
a92ba99
add docstring to testUtil
Hweinstock Oct 17, 2024
c2e17bd
add tests for path utility;
Hweinstock Oct 17, 2024
4cb7b3c
avoid moving function
Hweinstock Oct 17, 2024
60c2bed
refactor
Hweinstock Oct 17, 2024
5c48e66
avoid with paradigm
Hweinstock Oct 17, 2024
eff906a
make param optional
Hweinstock Oct 17, 2024
622c4d2
try case sensitive version
Hweinstock Oct 18, 2024
07026be
add sanity tests for util
Hweinstock Oct 18, 2024
f13cf65
avoid .cmd extension
Hweinstock Oct 18, 2024
a11c2fc
remove cmd
Hweinstock Oct 18, 2024
b910f85
remove linux-command in exec
Hweinstock Oct 18, 2024
bb1c3b9
add tests for executable file
Hweinstock Oct 18, 2024
5697da1
add extension to make it executable
Hweinstock Oct 18, 2024
b57053d
try .cmd instead of .exe
Hweinstock Oct 18, 2024
ba9fa6f
replace manual childprocess with tryRun
Hweinstock Oct 18, 2024
ff0cf37
remove cmd extension
Hweinstock Oct 18, 2024
bd292c2
re-write unit tests
Hweinstock Oct 18, 2024
6bec701
adjust tests
Hweinstock Oct 18, 2024
3d1740a
adjust windows cases
Hweinstock Oct 18, 2024
6b1bf0f
make a windows specific test
Hweinstock Oct 18, 2024
69f09ab
fix tests to be windows/mac specific
Hweinstock Oct 18, 2024
d10f613
Update packages/core/src/test/shared/utilities/testUtils.test.ts
Hweinstock Oct 18, 2024
cfcc6bb
Update packages/core/src/test/testUtil.ts
Hweinstock Oct 18, 2024
4b0785f
refactor to shallow copy
Hweinstock Oct 18, 2024
deff2a2
Update packages/core/src/test/shared/utilities/pathFind.test.ts
Hweinstock Oct 21, 2024
c6f02ac
Update packages/core/src/test/shared/utilities/pathFind.test.ts
Hweinstock Oct 21, 2024
5810995
Update packages/core/src/test/shared/utilities/pathFind.test.ts
Hweinstock Oct 21, 2024
2d0d4fb
remove unneeded utility function
Hweinstock Oct 21, 2024
912d579
fix dupe var in test
Hweinstock Oct 21, 2024
f2441ea
Merge branch 'master' into securityTests
Hweinstock Oct 21, 2024
7e7b0cb
write only to PATH
Hweinstock Oct 21, 2024
a28c33e
rewrite only the PATH var
Hweinstock Oct 21, 2024
3c80c2f
skip path test on windows, avoid generate assertion on windows
Hweinstock Oct 21, 2024
b82672a
Update packages/core/src/test/shared/utilities/pathFind.test.ts
Hweinstock Oct 21, 2024
e103c1e
Update packages/core/src/test/shared/utilities/pathFind.test.ts
Hweinstock Oct 21, 2024
a8880de
Update packages/core/src/test/shared/utilities/testUtils.test.ts
Hweinstock Oct 21, 2024
6cc9743
fix env ref
Hweinstock Oct 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/core/src/codewhisperer/commands/basicCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ import { ToolkitError, getTelemetryReason, getTelemetryReasonDesc } from '../../
import { isRemoteWorkspace } from '../../shared/vscode/env'
import { isBuilderIdConnection } from '../../auth/connection'
import globals from '../../shared/extensionGlobals'
import { getVscodeCliPath, tryRun } from '../../shared/utilities/pathFind'
import { getVscodeCliPath } from '../../shared/utilities/pathFind'
import { setContext } from '../../shared/vscode/setContext'
import { tryRun } from '../../shared/utilities/pathFind'

const MessageTimeOut = 5_000

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/shared/remoteSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export interface MissingTool {
readonly reason?: string
}

const minimumSsmActions = [
export const minimumSsmActions = [
'ssmmessages:CreateControlChannel',
'ssmmessages:CreateDataChannel',
'ssmmessages:OpenControlChannel',
Expand Down
64 changes: 32 additions & 32 deletions packages/core/src/shared/utilities/pathFind.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,44 +6,17 @@
import * as vscode from 'vscode'
import * as path from 'path'
import fs from '../../shared/fs/fs'
import { ChildProcess, ChildProcessOptions } from './processUtils'
import { GitExtension } from '../extensions/git'
import { Settings } from '../settings'
import { getLogger } from '../logger/logger'
import { ChildProcess, ChildProcessOptions } from './processUtils'
import { getLogger } from '..'

/** Full path to VSCode CLI. */
let vscPath: string
let sshPath: string
let gitPath: string
let bashPath: string

/**
* Tries to execute a program at path `p` with the given args and
* optionally checks the output for `expected`.
*
* @param p path to a program to execute
* @param args program args
* @param doLog log failures
* @param expected output must contain this string
*/
export async function tryRun(
p: string,
args: string[],
logging: 'yes' | 'no' | 'noresult' = 'yes',
expected?: string,
opt?: ChildProcessOptions
): Promise<boolean> {
const proc = new ChildProcess(p, args, { logging: 'no' })
const r = await proc.run(opt)
const ok = r.exitCode === 0 && (expected === undefined || r.stdout.includes(expected))
if (logging === 'noresult') {
getLogger().info('tryRun: %s: %s', ok ? 'ok' : 'failed', proc)
} else if (logging !== 'no') {
getLogger().info('tryRun: %s: %s %O', ok ? 'ok' : 'failed', proc, proc.result())
}
return ok
}

/**
* Gets the fullpath to `code` (VSCode CLI), or falls back to "code" (not
* absolute) if it works.
Expand Down Expand Up @@ -115,8 +88,8 @@ export async function findTypescriptCompiler(): Promise<string | undefined> {
* Gets the configured `ssh` path, or falls back to "ssh" (not absolute),
* or tries common locations, or returns undefined.
*/
export async function findSshPath(): Promise<string | undefined> {
if (sshPath !== undefined) {
export async function findSshPath(useCache: boolean = true): Promise<string | undefined> {
if (useCache && sshPath !== undefined) {
return sshPath
}

Expand All @@ -133,7 +106,7 @@ export async function findSshPath(): Promise<string | undefined> {
continue
}
if (await tryRun(p, ['-G', 'x'], 'noresult' /* "ssh -G" prints quasi-sensitive info. */)) {
sshPath = p
sshPath = useCache ? p : sshPath
return p
}
}
Expand Down Expand Up @@ -180,3 +153,30 @@ export async function findBashPath(): Promise<string | undefined> {
}
}
}

/**
* Tries to execute a program at path `p` with the given args and
* optionally checks the output for `expected`.
*
* @param p path to a program to execute
* @param args program args
* @param doLog log failures
* @param expected output must contain this string
*/
export async function tryRun(
p: string,
args: string[],
logging: 'yes' | 'no' | 'noresult' = 'yes',
expected?: string,
opt?: ChildProcessOptions
): Promise<boolean> {
const proc = new ChildProcess(p, args, { logging: 'no' })
const r = await proc.run(opt)
const ok = r.exitCode === 0 && (expected === undefined || r.stdout.includes(expected))
if (logging === 'noresult') {
getLogger().info('tryRun: %s: %s', ok ? 'ok' : 'failed', proc)
} else if (logging !== 'no') {
getLogger().info('tryRun: %s: %s %O', ok ? 'ok' : 'failed', proc, proc.result())
}
return ok
}
32 changes: 32 additions & 0 deletions packages/core/src/test/shared/remoteSession.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import assert from 'assert'
import { minimumSsmActions, promptToAddInlinePolicy } from '../../shared/remoteSession'
import { IamClient } from '../../shared/clients/iamClient'
import { getTestWindow } from './vscode/window'
import { cancel } from '../../shared'

describe('minimumSsmActions', function () {
it('should contain minimal actions needed for ssm connection', function () {
assert.deepStrictEqual(minimumSsmActions, [
'ssmmessages:CreateControlChannel',
'ssmmessages:CreateDataChannel',
'ssmmessages:OpenControlChannel',
'ssmmessages:OpenDataChannel',
'ssm:DescribeAssociation',
'ssm:ListAssociations',
'ssm:UpdateInstanceInformation',
])
})

it('prompts the user for confirmation before adding policies and allow cancels', async function () {
getTestWindow().onDidShowMessage((message) => {
assert.ok(message.message.includes('add'), 'should prompt to add policies')
getTestWindow().getFirstMessage().selectItem(cancel)
})
const added = await promptToAddInlinePolicy({} as IamClient, 'roleArnTest')
assert.ok(!added, 'should not add policies by default')
})
})
53 changes: 49 additions & 4 deletions packages/core/src/test/shared/utilities/pathFind.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,15 @@ import assert from 'assert'
import * as vscode from 'vscode'
import * as os from 'os'
import * as path from 'path'

import * as testutil from '../../testUtil'
import { findTypescriptCompiler, getVscodeCliPath } from '../../../shared/utilities/pathFind'
import { fs } from '../../../shared'
import { findSshPath, findTypescriptCompiler, getVscodeCliPath } from '../../../shared/utilities/pathFind'
import { isWin } from '../../../shared/vscode/env'

describe('pathFind', function () {
it('findTypescriptCompiler()', async function () {
const iswin = process.platform === 'win32'
const workspace = vscode.workspace.workspaceFolders![0]
const tscNodemodules = path.join(workspace.uri.fsPath, `foo/bar/node_modules/.bin/tsc${iswin ? '.cmd' : ''}`)
const tscNodemodules = path.join(workspace.uri.fsPath, `foo/bar/node_modules/.bin/tsc${isWin() ? '.cmd' : ''}`)
await fs.delete(tscNodemodules, { force: true })

// The test workspace normally doesn't have node_modules so this will
Expand All @@ -42,4 +41,50 @@ describe('pathFind', function () {
const regex = /bin[\\\/](code|code-insiders)$/
assert.ok(regex.test(vscPath), `expected regex ${regex} to match: "${vscPath}"`)
})

describe('findSshPath', function () {
it('first tries ssh in $PATH', async function () {
const workspace = await testutil.createTestWorkspaceFolder()
const fakeSshPath = path.join(workspace.uri.fsPath, `ssh${isWin() ? '.cmd' : ''}`)

await testutil.withEnvPath(workspace.uri.fsPath, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we just set $PATH here directly, can afterEach restore it?

the "with" pattern can be useful but it might be confusing if we have different ways of saving/storing state in tests (but maybe there is a good reason for it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this definitely reduces the amount of code as well. My only concern is that adding more behavior into beforeEach and afterEach can make it hard to isolate the behavior of each test, and sometimes makes it harder to add tests if it doesn't fit with the pattern.

I think this is still the better route since it is inline with the rest of the test code (ex. sinon, clock).

const firstResult = await findSshPath(false)

await testutil.createExecutableFile(fakeSshPath, 'echo "this is ssh"')

const secondResult = await findSshPath(false)

assert.notStrictEqual(firstResult, secondResult)
assert.strictEqual(secondResult, 'ssh')
})
})

it('only returns executable ssh path', async function () {
const workspace = await testutil.createTestWorkspaceFolder()
const fakeSshPath = path.join(workspace.uri.fsPath, `ssh${isWin() ? '.cmd' : ''}`)
await fs.writeFile(fakeSshPath, 'this is not executable')

await testutil.withEnvPath(workspace.uri.fsPath, async () => {
const firstResult = await findSshPath(false)
assert.notStrictEqual(firstResult, 'ssh')
})
})

it('caches result from previous runs', async function () {
const workspace = await testutil.createTestWorkspaceFolder()
const fakeSshPath = path.join(workspace.uri.fsPath, `ssh${isWin() ? '.cmd' : ''}`)
await testutil.createExecutableFile(fakeSshPath, 'echo "this is ssh"')

await testutil.withEnvPath(workspace.uri.fsPath, async () => {
const firstResult = await findSshPath(true)

await fs.delete(fakeSshPath)

const secondResult = await findSshPath(true)

assert.strictEqual(firstResult, secondResult)
assert.strictEqual(secondResult, 'ssh')
})
})
})
})
28 changes: 28 additions & 0 deletions packages/core/src/test/shared/utilities/testUtil.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import assert from 'assert'
import { createTestWorkspaceFolder, readEnvPath, withEnvPath } from '../../testUtil'

describe('withEnvPath', function () {
it('resets path when error in task', async function () {
const originalPath = readEnvPath()
const tempFolder = await createTestWorkspaceFolder()
try {
await withEnvPath(tempFolder.uri.fsPath, async () => {
throw new Error()
})
} catch {}
assert.strictEqual(readEnvPath(), originalPath)
})

it('changes $PATH temporarily', async function () {
const originalPath = readEnvPath()
const tempFolder = await createTestWorkspaceFolder()
await withEnvPath(tempFolder.uri.fsPath, async () => {
assert.strictEqual(readEnvPath(), tempFolder.uri.fsPath)
})
assert.strictEqual(readEnvPath(), originalPath)
})
})
22 changes: 22 additions & 0 deletions packages/core/src/test/testUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -614,3 +614,25 @@ export function tryRegister(command: DeclaredCommand<() => Promise<any>>) {
export function getFetchStubWithResponse(response: Partial<Response>) {
return stub(request, 'fetch').returns({ response: new Promise((res, _) => res(response)) } as any)
}

export function setEnvPath(newPath: string): void {
process.env.PATH = newPath
}

export function readEnvPath(): string {
return process.env.PATH || ''
}
/**
* Overwrite the $PATH environment variable for the span of the provided task, then reset it.
* @param newPath temporary $PATH value
* @param task work to be done with $PATH set.
*/
export async function withEnvPath(newPath: string, task: () => Promise<void>): Promise<void | never> {
const originalPath = readEnvPath()
setEnvPath(newPath)
try {
await task()
} finally {
setEnvPath(originalPath)
}
}
Loading