From 11536586aa8dce8259748977cdf2e3dcaa0441bf Mon Sep 17 00:00:00 2001 From: Hweinstock <42325418+Hweinstock@users.noreply.github.com> Date: Tue, 12 Nov 2024 10:25:29 -0500 Subject: [PATCH] test(sam): slow SamCliLocalInvokeInvocation, ChildProcess tests #5964 ## Problem - The test failure is due to timeout on Windows when first searching for the SAM cli path in `src/shared/sam/cli/samCliLocator.ts`. This is a known slow process. - The verification of SAM is also slow (100-200ms), even if result is cached and we do this each time we read from cache. ## Solution - Verify the cache once, rather than on each read. - Find sam cli in the before hook to allow all sam tests to run under same conditions. Otherwise, the first test is then responsible for actually finding the path causing it to take way longer than the rest of the tests. - Allow 3x retries on before hook on windows to avoid flakiness. ## Notes - An alternative solution could involve manually inserting into the cache, or mocking a piece of the component, but wanted to keep the test coverage as is. - This approach also allows us to isolate the finding code into its own test (rather than whatever the first test is) to see if there are performance regressions. - Additionally, retry mechanism was more effective than increasing the timeout in reducing flakiness. --- .../src/shared/sam/cli/samCliLocalInvoke.ts | 7 +++---- .../core/src/shared/sam/cli/samCliLocator.ts | 18 ++++++++++++++---- .../core/src/shared/sam/cli/samCliSettings.ts | 1 - .../shared/sam/cli/samCliLocalInvoke.test.ts | 11 +++++++++++ 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/packages/core/src/shared/sam/cli/samCliLocalInvoke.ts b/packages/core/src/shared/sam/cli/samCliLocalInvoke.ts index 14b51de6b8c..d0bea65b118 100644 --- a/packages/core/src/shared/sam/cli/samCliLocalInvoke.ts +++ b/packages/core/src/shared/sam/cli/samCliLocalInvoke.ts @@ -6,7 +6,6 @@ import * as proc from 'child_process' import { pushIf } from '../../utilities/collectionUtils' import * as nls from 'vscode-nls' -import { fileExists } from '../../filesystemUtilities' import { getLogger, getDebugConsoleLogger, Logger } from '../../logger' import { ChildProcess } from '../../utilities/processUtils' import { Timeout } from '../../utilities/timeoutUtils' @@ -15,6 +14,7 @@ import * as vscode from 'vscode' import globals from '../../extensionGlobals' import { SamCliSettings } from './samCliSettings' import { addTelemetryEnvVar, collectSamErrors, SamCliError } from './samCliInvokerUtils' +import { fs } from '../../fs/fs' const localize = nls.loadMessageBundle() @@ -232,7 +232,6 @@ export class SamCliLocalInvokeInvocation { public async execute(timeout?: Timeout): Promise { await this.validate() - const sam = await this.config.getOrDetectSamCli() if (!sam.path) { getLogger().warn('SAM CLI not found and not configured') @@ -288,11 +287,11 @@ export class SamCliLocalInvokeInvocation { throw new Error('template resource name is missing or empty') } - if (!(await fileExists(this.args.templatePath))) { + if (!(await fs.exists(this.args.templatePath))) { throw new Error(`template path does not exist: ${this.args.templatePath}`) } - if (this.args.eventPath !== undefined && !(await fileExists(this.args.eventPath))) { + if (this.args.eventPath !== undefined && !(await fs.exists(this.args.eventPath))) { throw new Error(`event path does not exist: ${this.args.eventPath}`) } } diff --git a/packages/core/src/shared/sam/cli/samCliLocator.ts b/packages/core/src/shared/sam/cli/samCliLocator.ts index 7db6c163db4..87fe474c76e 100644 --- a/packages/core/src/shared/sam/cli/samCliLocator.ts +++ b/packages/core/src/shared/sam/cli/samCliLocator.ts @@ -13,13 +13,20 @@ import { PerfLog } from '../../logger/perfLogger' import { tryRun } from '../../utilities/pathFind' import { mergeResolvedShellPath } from '../../env/resolveEnv' +interface SamLocation { + path: string + version: string +} export class SamCliLocationProvider { private static samCliLocator: BaseSamCliLocator | undefined - protected static cachedSamLocation: { path: string; version: string } | undefined + protected static cachedSamLocation: SamLocation | undefined + private static samLocationValid: boolean = false /** Checks that the given `sam` actually works by invoking `sam --version`. */ private static async isValidSamLocation(samPath: string) { - return await tryRun(samPath, ['--version'], 'no', 'SAM CLI') + const isValid = await tryRun(samPath, ['--version'], 'no', 'SAM CLI') + this.samLocationValid = isValid + return isValid } /** @@ -31,11 +38,14 @@ export class SamCliLocationProvider { const cachedLoc = forceSearch ? undefined : SamCliLocationProvider.cachedSamLocation // Avoid searching the system for `sam` (especially slow on Windows). - if (cachedLoc && (await SamCliLocationProvider.isValidSamLocation(cachedLoc.path))) { + if ( + cachedLoc && + (SamCliLocationProvider.samLocationValid || + (await SamCliLocationProvider.isValidSamLocation(cachedLoc.path))) + ) { perflog.done() return cachedLoc } - SamCliLocationProvider.cachedSamLocation = await SamCliLocationProvider.getSamCliLocator().getLocation() perflog.done() return SamCliLocationProvider.cachedSamLocation diff --git a/packages/core/src/shared/sam/cli/samCliSettings.ts b/packages/core/src/shared/sam/cli/samCliSettings.ts index 5ef8b4c200c..9562493a75b 100644 --- a/packages/core/src/shared/sam/cli/samCliSettings.ts +++ b/packages/core/src/shared/sam/cli/samCliSettings.ts @@ -83,7 +83,6 @@ export class SamCliSettings extends fromExtensionManifest('aws.samcli', descript SamCliSettings.logIfChanged(`SAM CLI location (from settings): ${fromConfig}`) return { path: fromConfig, autoDetected: false } } - const fromSearch = await this.locationProvider.getLocation(forceSearch) SamCliSettings.logIfChanged(`SAM CLI location (version: ${fromSearch?.version}): ${fromSearch?.path}`) return { path: fromSearch?.path, autoDetected: true } diff --git a/packages/core/src/test/shared/sam/cli/samCliLocalInvoke.test.ts b/packages/core/src/test/shared/sam/cli/samCliLocalInvoke.test.ts index c9379fbb15e..7e61d507a19 100644 --- a/packages/core/src/test/shared/sam/cli/samCliLocalInvoke.test.ts +++ b/packages/core/src/test/shared/sam/cli/samCliLocalInvoke.test.ts @@ -14,6 +14,8 @@ import { import { ChildProcess } from '../../../../shared/utilities/processUtils' import { assertArgIsPresent, assertArgNotPresent, assertArgsContainArgument } from './samCliTestUtils' import { fs } from '../../../../shared' +import { SamCliSettings } from '../../../../shared/sam/cli/samCliSettings' +import { isWin } from '../../../../shared/vscode/env' describe('SamCliLocalInvokeInvocation', async function () { class TestSamLocalInvokeCommand implements SamLocalInvokeCommand { @@ -30,6 +32,15 @@ describe('SamCliLocalInvokeInvocation', async function () { let placeholderEventFile: string const nonRelevantArg = 'arg is not of interest to this test' + before(async function () { + // File system search on windows can take a while. + if (isWin()) { + this.retries(3) + } + // This will place the result in the cache allowing all tests to run under same conditions. + await SamCliSettings.instance.getOrDetectSamCli() + }) + beforeEach(async function () { tempFolder = await makeTemporaryToolkitFolder() placeholderTemplateFile = path.join(tempFolder, 'template.yaml')