From c8de5bef0304f85aee69b2b646c90653bfe11fff Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Tue, 28 Jan 2025 10:18:02 +0100 Subject: [PATCH] chore(cli-integ): make it possible to run on GitHub Actions (#33175) Migrate some changes back from the new testing repo. These changes are necessary to make the tests run on GitHub Actions. If we keep them here, in the future we can do a `cp -R` on the test directory. If not, we'll have to do manual sorting on every copy over, which is annoying and easy to make mistakes in. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../@aws-cdk-testing/cli-integ/lib/aws.ts | 2 +- .../cli-integ/lib/integ-test.ts | 48 +++++++++++++++++-- .../@aws-cdk-testing/cli-integ/lib/shell.ts | 25 ++++++---- .../cli-integ/lib/with-cdk-app.ts | 7 ++- .../cli-integ/lib/with-cli-lib.ts | 3 ++ .../cli-integ/lib/with-sam.ts | 45 +++++++++-------- .../cli-integ/resources/cdk-apps/app/app.js | 6 +-- .../cdk-apps/rollback-test-app/app.js | 2 +- .../sam_cdk_integ_app/lib/test-stack.js | 4 +- .../v1.130.0/bootstrapping.integtest.js | 2 +- .../tests/init-go/init-go.integtest.ts | 7 ++- .../init-typescript-app.integtest.ts | 7 +-- 12 files changed, 111 insertions(+), 47 deletions(-) diff --git a/packages/@aws-cdk-testing/cli-integ/lib/aws.ts b/packages/@aws-cdk-testing/cli-integ/lib/aws.ts index 64c5b7c2f38fe..ab8006572eff5 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/aws.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/aws.ts @@ -252,7 +252,7 @@ export async function sleep(ms: number) { } function chainableCredentials(region: string): AwsCredentialIdentityProvider | undefined { - if (process.env.CODEBUILD_BUILD_ARN && process.env.AWS_PROFILE) { + if ((process.env.CODEBUILD_BUILD_ARN || process.env.GITHUB_RUN_ID) && process.env.AWS_PROFILE) { // in codebuild we must assume the role that the cdk uses // otherwise credentials will just be picked up by the normal sdk // heuristics and expire after an hour. diff --git a/packages/@aws-cdk-testing/cli-integ/lib/integ-test.ts b/packages/@aws-cdk-testing/cli-integ/lib/integ-test.ts index 85650d932ef5d..df9d37d134ede 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/integ-test.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/integ-test.ts @@ -11,6 +11,12 @@ if (SKIP_TESTS) { process.stderr.write(`ℹ️ Skipping tests: ${JSON.stringify(SKIP_TESTS)}\n`); } +// Whether we want to stop after the first failure, for quicker debugging (hopefully). +const FAIL_FAST = process.env.FAIL_FAST === 'true'; + +// Keep track of whether the suite has failed. If so, we stop running. +let failed = false; + export interface TestContext { readonly randomString: string; readonly name: string; @@ -49,6 +55,10 @@ export function integTest( const now = Date.now(); process.stderr.write(`[INTEG TEST::${name}] Starting (pid ${process.pid})...\n`); try { + if (FAIL_FAST && failed) { + throw new Error('FAIL_FAST requested and currently failing. Stopping test early.'); + } + return await callback({ output, randomString: randomString(), @@ -58,13 +68,41 @@ export function integTest( }, }); } catch (e: any) { - process.stderr.write(`[INTEG TEST::${name}] Failed: ${e}\n`); + failed = true; + + // Print the buffered output, only if the test fails. output.write(e.message); output.write(e.stack); - // Print output only if the test fails. Use 'console.log' so the output is buffered by - // jest and prints without a stack trace (if verbose: false). - // eslint-disable-next-line no-console - console.log(output.buffer().toString()); + process.stderr.write(`[INTEG TEST::${name}] Failed: ${e}\n`); + + const isGitHub = !!process.env.GITHUB_RUN_ID; + + if (isGitHub) { + // GitHub Actions compatible output formatting + // https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-an-error-message + let written = process.stderr.write(`::error title=Failed ${name}::${e.message}\n`); + if (!written) { + // Wait for drain + await new Promise((ok) => process.stderr.once('drain', ok)); + } + + // Print output only if the test fails. Use 'console.log' so the output is buffered by + // jest and prints without a stack trace (if verbose: false). + written = process.stdout.write([ + `::group::Failure details: ${name} (click to expand)\n`, + `${output.buffer().toString()}\n`, + '::endgroup::\n', + ].join('')); + if (!written) { + // Wait for drain + await new Promise((ok) => process.stdout.once('drain', ok)); + } + } else { + // Use 'console.log' so the output is buffered by + // jest and prints without a stack trace (if verbose: false). + // eslint-disable-next-line no-console + console.log(output.buffer().toString()); + } throw e; } finally { const duration = Date.now() - now; diff --git a/packages/@aws-cdk-testing/cli-integ/lib/shell.ts b/packages/@aws-cdk-testing/cli-integ/lib/shell.ts index 4d376b636024f..c462676604ce4 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/shell.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/shell.ts @@ -25,10 +25,6 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom writeToOutputs(`💻 ${command.join(' ')}\n`); const show = options.show ?? 'always'; - if (process.env.VERBOSE) { - outputs.add(process.stdout); - } - const env = options.env ?? (options.modEnv ? { ...process.env, ...options.modEnv } : process.env); const child = child_process.spawn(command[0], command.slice(1), { @@ -81,7 +77,7 @@ export interface ShellOptions extends child_process.SpawnOptions { /** * Properties to add to 'env' */ - readonly modEnv?: Record; + readonly modEnv?: Record; /** * Don't fail when exiting with an error @@ -138,22 +134,33 @@ export class ShellHelper { /** * rm -rf reimplementation, don't want to depend on an NPM package for this + * + * Returns `true` if everything got deleted, or `false` if some files could + * not be deleted due to permissions issues. */ -export function rimraf(fsPath: string) { +export function rimraf(fsPath: string): boolean { try { + let success = true; const isDir = fs.lstatSync(fsPath).isDirectory(); if (isDir) { for (const file of fs.readdirSync(fsPath)) { - rimraf(path.join(fsPath, file)); + success &&= rimraf(path.join(fsPath, file)); } fs.rmdirSync(fsPath); } else { fs.unlinkSync(fsPath); } + return success; } catch (e: any) { - // We will survive ENOENT - if (e.code !== 'ENOENT') { throw e; } + // Can happen if some files got generated inside a Docker container and are now inadvertently owned by `root`. + // We can't ever clean those up anymore, but since it only happens inside GitHub Actions containers we also don't care too much. + if (e.code === 'EACCES' || e.code === 'ENOTEMPTY') { return false; } + + // Already gone + if (e.code === 'ENOENT') { return true; } + + throw e; } } diff --git a/packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts b/packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts index 03325ad771ed8..966becf2c2f64 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts @@ -513,6 +513,8 @@ export class TestFixture extends ShellHelper { AWS_DEFAULT_REGION: this.aws.region, STACK_NAME_PREFIX: this.stackNamePrefix, PACKAGE_LAYOUT_VERSION: this.packages.majorVersion(), + // CI must be unset, because we're trying to capture stdout in a bunch of tests + CI: 'false', ...awsCreds, ...options.modEnv, }, @@ -607,7 +609,10 @@ export class TestFixture extends ShellHelper { // If the tests completed successfully, happily delete the fixture // (otherwise leave it for humans to inspect) if (success) { - rimraf(this.integTestDir); + const cleaned = rimraf(this.integTestDir); + if (!cleaned) { + console.error(`Failed to clean up ${this.integTestDir} due to permissions issues (Docker running as root?)`); + } } } diff --git a/packages/@aws-cdk-testing/cli-integ/lib/with-cli-lib.ts b/packages/@aws-cdk-testing/cli-integ/lib/with-cli-lib.ts index 8b319b9ed13a3..7bc98bc0b9735 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/with-cli-lib.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/with-cli-lib.ts @@ -131,6 +131,9 @@ __EOS__`], { AWS_DEFAULT_REGION: this.aws.region, STACK_NAME_PREFIX: this.stackNamePrefix, PACKAGE_LAYOUT_VERSION: this.packages.majorVersion(), + // Unset CI because we need to distinguish stdout/stderr and this variable + // makes everything go to stdout + CI: 'false', ...options.modEnv, }, }); diff --git a/packages/@aws-cdk-testing/cli-integ/lib/with-sam.ts b/packages/@aws-cdk-testing/cli-integ/lib/with-sam.ts index 9f71386d77028..5e09762f21a6f 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/with-sam.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/with-sam.ts @@ -1,5 +1,4 @@ import * as child_process from 'child_process'; -import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import axios from 'axios'; @@ -139,6 +138,7 @@ export class SamIntegrationTestFixture extends TestFixture { args.push('--port'); args.push(port.toString()); + // "Press Ctrl+C to quit" looks to be printed by a Flask server built into SAM CLI. return this.samShell(['sam', 'local', 'start-api', ...args], 'Press CTRL+C to quit', ()=>{ return new Promise((resolve, reject) => { axios.get(`http://127.0.0.1:${port}${apiPath}`).then( resp => { @@ -157,7 +157,11 @@ export class SamIntegrationTestFixture extends TestFixture { // If the tests completed successfully, happily delete the fixture // (otherwise leave it for humans to inspect) if (success) { - rimraf(this.integTestDir); + const cleaned = rimraf(this.integTestDir); + if (!cleaned) { + // eslint-disable-next-line no-console + console.error(`Failed to clean up ${this.integTestDir} due to permissions issues (Docker running as root?)`); + } } } } @@ -207,23 +211,24 @@ export async function shellWithAction( let actionOutput: any; let actionExecuted = false; - function executeAction(chunk: any) { + async function maybeExecuteAction(chunk: any) { out.push(Buffer.from(chunk)); if (!actionExecuted && typeof filter === 'string' && Buffer.concat(out).toString('utf-8').includes(filter) && typeof action === 'function') { actionExecuted = true; - writeToOutputs('before executing action'); - action().then((output) => { - writeToOutputs(`action output is ${output}`); + writeToOutputs('before executing action\n'); + try { + const output = await action(); + writeToOutputs(`action output is ${JSON.stringify(output)}\n`); actionOutput = output; actionSucceeded = true; - }).catch((error) => { - writeToOutputs(`action error is ${error}`); + } catch (error: any) { + writeToOutputs(`action error is ${error}\n`); actionSucceeded = false; actionOutput = error; - }).finally(() => { - writeToOutputs('terminate sam sub process'); + } finally { + writeToOutputs('terminate sam sub process\n'); killSubProcess(child, command.join(' ')); - }); + } } } @@ -234,6 +239,7 @@ export async function shellWithAction( () => { if (!actionExecuted) { reject(new Error(`Timed out waiting for filter ${JSON.stringify(filter)} to appear in command output after ${actionTimeoutSeconds} seconds\nOutput so far:\n${Buffer.concat(out).toString('utf-8')}`)); + killSubProcess(child, command.join(' ')); } }, actionTimeoutSeconds * 1_000, ).unref(); @@ -242,7 +248,7 @@ export async function shellWithAction( child.stdout!.on('data', chunk => { writeToOutputs(chunk); stdout.push(chunk); - executeAction(chunk); + void maybeExecuteAction(chunk); }); child.stderr!.on('data', chunk => { @@ -250,12 +256,14 @@ export async function shellWithAction( if (options.captureStderr ?? true) { stderr.push(chunk); } - executeAction(chunk); + void maybeExecuteAction(chunk); }); child.once('error', reject); - child.once('close', code => { + // Wait for 'exit' instead of close, don't care about reading the streams all the way to the end + child.once('exit', (code, signal) => { + writeToOutputs(`Subprocess has exited with code ${code}, signal ${signal}\n`); const output = (Buffer.concat(stdout).toString('utf-8') + Buffer.concat(stderr).toString('utf-8')).trim(); if (code == null || code === 0 || options.allowErrExit) { let result = new Array(); @@ -270,7 +278,6 @@ export async function shellWithAction( reject(new Error(`'${command.join(' ')}' exited with error code ${code}. Output: \n${output}`)); } }); - }); } @@ -279,10 +286,6 @@ function killSubProcess(child: child_process.ChildProcess, command: string) { * Check if the sub process is running in container, so child_process.spawn will * create multiple processes, and to kill all of them we need to run different logic */ - if (fs.existsSync('/.dockerenv')) { - child_process.exec(`for pid in $(ps -ef | grep "${command}" | awk '{print $2}'); do kill -2 $pid; done`); - } else { - child.kill('SIGINT'); - } - + child.kill('SIGINT'); + child_process.exec(`for pid in $(ps -ef | grep "${command}" | awk '{print $2}'); do kill -2 $pid; done`); } diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js index 61e58871ed2c1..c0ce0ed383886 100755 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js @@ -748,7 +748,7 @@ class NotificationArnsStack extends cdk.Stack { const arnsFromEnv = process.env.INTEG_NOTIFICATION_ARNS; super(parent, id, { ...props, - // comma separated list of arns. + // comma separated list of arns. // empty string means empty list. // undefined means undefined notificationArns: arnsFromEnv == '' ? [] : (arnsFromEnv ? arnsFromEnv.split(',') : undefined) @@ -793,12 +793,12 @@ class MetadataStack extends cdk.Stack { const handle = new cdk.CfnWaitConditionHandle(this, 'WaitConditionHandle'); handle.addMetadata('Key', process.env.INTEG_METADATA_VALUE ?? 'default') - } + } } const app = new cdk.App({ context: { - '@aws-cdk/core:assetHashSalt': process.env.CODEBUILD_BUILD_ID, // Force all assets to be unique, but consistent in one build + '@aws-cdk/core:assetHashSalt': process.env.CODEBUILD_BUILD_ID ?? process.env.GITHUB_RUN_ID, // Force all assets to be unique, but consistent in one build }, }); diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/rollback-test-app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/rollback-test-app/app.js index dd117b62a9dd9..413bcbd1972c9 100644 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/rollback-test-app/app.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/rollback-test-app/app.js @@ -91,7 +91,7 @@ class RollbacktestStack extends cdk.Stack { const app = new cdk.App({ context: { - '@aws-cdk/core:assetHashSalt': process.env.CODEBUILD_BUILD_ID, // Force all assets to be unique, but consistent in one build + '@aws-cdk/core:assetHashSalt': process.env.CODEBUILD_BUILD_ID ?? process.env.GITHUB_RUN_ID, // Force all assets to be unique, but consistent in one build }, }); diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/sam_cdk_integ_app/lib/test-stack.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/sam_cdk_integ_app/lib/test-stack.js index 3e9a071de8ff1..935edcf15bf7c 100644 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/sam_cdk_integ_app/lib/test-stack.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/sam_cdk_integ_app/lib/test-stack.js @@ -21,6 +21,8 @@ if (process.env.PACKAGE_LAYOUT_VERSION === '1') { } const isRunningOnCodeBuild = !!process.env.CODEBUILD_BUILD_ID; +const isRunningOnGitHubActions = !!process.env.GITHUB_RUN_ID; +const isRunningOnCi = isRunningOnCodeBuild || isRunningOnGitHubActions; class CDKSupportDemoRootStack extends Stack{ constructor(scope, id, props) { @@ -102,7 +104,7 @@ class CDKSupportDemoRootStack extends Stack{ bundling: { forcedDockerBundling: true, // Only use Google proxy in the CI tests, as it is blocked on workstations - goProxies: isRunningOnCodeBuild ? [GoFunction.GOOGLE_GOPROXY, 'direct'] : undefined, + goProxies: isRunningOnCi ? [GoFunction.GOOGLE_GOPROXY, 'direct'] : undefined, }, }); diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cli-regression-patches/v1.130.0/bootstrapping.integtest.js b/packages/@aws-cdk-testing/cli-integ/resources/cli-regression-patches/v1.130.0/bootstrapping.integtest.js index 5895d49ff1ad9..33881210be70c 100644 --- a/packages/@aws-cdk-testing/cli-integ/resources/cli-regression-patches/v1.130.0/bootstrapping.integtest.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cli-regression-patches/v1.130.0/bootstrapping.integtest.js @@ -4,7 +4,7 @@ const fs = require("fs"); const path = require("path"); const cdk_1 = require("../helpers/cdk"); const test_helpers_1 = require("../helpers/test-helpers"); -const timeout = process.env.CODEBUILD_BUILD_ID ? // if the process is running in CodeBuild +const timeout = (process.env.CODEBUILD_BUILD_ID ?? process.env.GITHUB_RUN_ID) ? // if the process is running in CodeBuild 3600000 : // 1 hour 600000; // 10 minutes jest.setTimeout(timeout); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/init-go/init-go.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/init-go/init-go.integtest.ts index 56d8b9486c6b2..ee5d9418fab37 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/init-go/init-go.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/init-go/init-go.integtest.ts @@ -13,7 +13,12 @@ import { integTest, withTemporaryDirectory, ShellHelper, withPackages } from '.. // Canaries will use the generated go.mod as is // For pipeline tests we replace the source with the locally build one if (!isCanary) { - await shell.shell(['go', 'mod', 'edit', '-replace', 'github.com/aws/aws-cdk-go/awscdk/v2=$CODEBUILD_SRC_DIR/go/awscdk']); + const dir = process.env.CODEBUILD_SRC_DIR ?? process.env.GITHUB_WORKSPACE; + if (!dir) { + throw new Error('Cannot figure out CI system root directory'); + } + + await shell.shell(['go', 'mod', 'edit', '-replace', `github.com/aws/aws-cdk-go/awscdk/v2=${dir}/go/awscdk`]); } await shell.shell(['go', 'mod', 'tidy']); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/init-typescript-app/init-typescript-app.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/init-typescript-app/init-typescript-app.integtest.ts index df1dde2e7b36b..a462a67cce8af 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/init-typescript-app/init-typescript-app.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/init-typescript-app/init-typescript-app.integtest.ts @@ -24,9 +24,6 @@ const TYPESCRIPT_VERSION_AGE_DAYS = 2 * 365; const TYPESCRIPT_VERSIONS = typescriptVersionsYoungerThanDaysSync(TYPESCRIPT_VERSION_AGE_DAYS, typescriptVersionsSync()); -// eslint-disable-next-line no-console -console.log(TYPESCRIPT_VERSIONS); - /** * Test our generated code with various versions of TypeScript */ @@ -45,6 +42,10 @@ TYPESCRIPT_VERSIONS.forEach(tsVersion => { await removeDevDependencies(context); await shell.shell(['npm', 'install', '--save-dev', `typescript@${tsVersion}`]); + + // After we've removed devDependencies we need to re-install ts-node because it's necessary for `cdk synth` + await shell.shell(['npm', 'install', '--save-dev', 'ts-node@^10']); + await shell.shell(['npm', 'install']); // Older versions of npm require this to be a separate step from the one above await shell.shell(['npx', 'tsc', '--version']); await shell.shell(['npm', 'prune']);