Skip to content

Commit

Permalink
chore: CLI integ tests don't buffer shell output
Browse files Browse the repository at this point in the history
What should happen is that the output of shell commands gets buffered,
and only printed if the test running them fails.

In practice, we see the output being printed directly.

The buffering code is a bit confusing, and we don't exactly understand
why it's not working. Try to simplify the code a bit and remove mutable
variable manipulation, to address the above.
  • Loading branch information
rix0rrr committed Oct 25, 2024
1 parent d1d179f commit 0d9070e
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 37 deletions.
42 changes: 21 additions & 21 deletions packages/@aws-cdk-testing/cli-integ/lib/shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,19 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom
throw new Error('Use either env or modEnv but not both');
}

const outputs = new Set(options.outputs);
const writeToOutputs = (x: string) => {
for (const outputStream of outputs) {
outputStream.write(x);
}
};

// Always output the command
(options.output ?? process.stdout).write(`💻 ${command.join(' ')}\n`);

let output: NodeJS.WritableStream | undefined = options.output ?? process.stdout;
switch (options.show ?? 'always') {
case 'always':
break;
case 'never':
case 'error':
output = undefined;
break;
}
writeToOutputs(`💻 ${command.join(' ')}\n`);
const show = options.show ?? 'always';

if (process.env.VERBOSE) {
output = process.stdout;
outputs.add(process.stdout);
}

const env = options.env ?? (options.modEnv ? { ...process.env, ...options.modEnv } : process.env);
Expand All @@ -46,12 +44,16 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom
const stderr = new Array<Buffer>();

child.stdout!.on('data', chunk => {
output?.write(chunk);
if (show === 'always') {
writeToOutputs(chunk);
}
stdout.push(chunk);
});

child.stderr!.on('data', chunk => {
output?.write(chunk);
if (show === 'always') {
writeToOutputs(chunk);
}
if (options.captureStderr ?? true) {
stderr.push(chunk);
}
Expand All @@ -66,8 +68,8 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom
if (code === 0 || options.allowErrExit) {
resolve(out);
} else {
if (options.show === 'error') {
(options.output ?? process.stdout).write(out + '\n');
if (show === 'error') {
writeToOutputs(`${out}\n`);
}
reject(new Error(`'${command.join(' ')}' exited with error code ${code}.`));
}
Expand Down Expand Up @@ -97,10 +99,8 @@ export interface ShellOptions extends child_process.SpawnOptions {

/**
* Pass output here
*
* @default stdout unless quiet=true
*/
readonly output?: NodeJS.WritableStream;
readonly outputs?: NodeJS.WritableStream[];

/**
* Only return stderr. For example, this is used to validate
Expand All @@ -127,9 +127,9 @@ export class ShellHelper {
private readonly _cwd: string,
private readonly _output: NodeJS.WritableStream) { }

public async shell(command: string[], options: Omit<ShellOptions, 'cwd' | 'output'> = {}): Promise<string> {
public async shell(command: string[], options: Omit<ShellOptions, 'cwd' | 'outputs'> = {}): Promise<string> {
return shell(command, {
output: this._output,
outputs: [this._output],
cwd: this._cwd,
...options,
});
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk-testing/cli-integ/lib/staging/maven.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export async function uploadJavaPackages(packages: string[], login: LoginInforma
`-Dfile=${pkg.replace(/.pom$/, '.jar')}`,
...await pathExists(sourcesFile) ? [`-Dsources=${sourcesFile}`] : [],
...await pathExists(javadocFile) ? [`-Djavadoc=${javadocFile}`] : []], {
output,
outputs: [output],
modEnv: {
// Do not try to JIT the Maven binary
MAVEN_OPTS: `${NO_JIT} ${process.env.MAVEN_OPTS ?? ''}`.trim(),
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk-testing/cli-integ/lib/staging/npm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export async function uploadNpmPackages(packages: string[], login: LoginInformat
await shell(['node', require.resolve('npm'), 'publish', path.resolve(pkg)], {
modEnv: npmEnv(usageDir, login),
show: 'error',
output,
outputs: [output],
});

console.log(`✅ ${pkg}`);
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk-testing/cli-integ/lib/staging/nuget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export async function uploadDotnetPackages(packages: string[], usageDir: UsageDi
'--disable-buffering',
'--timeout', '600',
'--skip-duplicate'], {
output,
outputs: [output],
});

console.log(`✅ ${pkg}`);
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk-testing/cli-integ/lib/staging/pypi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export async function uploadPythonPackages(packages: string[], login: LoginInfor
TWINE_REPOSITORY_URL: login.pypiEndpoint,
},
show: 'error',
output,
outputs: [output],
});

console.log(`✅ ${pkg}`);
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,9 @@ export interface CdkCliOptions extends ShellOptions {
* Prepare a target dir byreplicating a source directory
*/
export async function cloneDirectory(source: string, target: string, output?: NodeJS.WritableStream) {
await shell(['rm', '-rf', target], { output });
await shell(['mkdir', '-p', target], { output });
await shell(['cp', '-R', source + '/*', target], { output });
await shell(['rm', '-rf', target], { outputs: output ? [output] : [] });
await shell(['mkdir', '-p', target], { outputs: output ? [output] : [] });
await shell(['cp', '-R', source + '/*', target], { outputs: output ? [output] : [] });
}

interface CommonCdkBootstrapCommandOptions {
Expand Down
21 changes: 13 additions & 8 deletions packages/@aws-cdk-testing/cli-integ/lib/with-sam.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export function withSamIntegrationFixture(block: (context: SamIntegrationTestFix
export class SamIntegrationTestFixture extends TestFixture {
public async samShell(command: string[], filter?: string, action?: () => any, options: Omit<ShellOptions, 'cwd' | 'output'> = {}): Promise<ActionOutput> {
return shellWithAction(command, filter, action, {
output: this.output,
outputs: [this.output],
cwd: path.join(this.integTestDir, 'cdk.out').toString(),
...options,
});
Expand Down Expand Up @@ -182,7 +182,12 @@ export async function shellWithAction(
throw new Error('Use either env or modEnv but not both');
}

options.output?.write(`💻 ${command.join(' ')}\n`);
const writeToOutputs = (x: string) => {
for (const output of options.outputs ?? []) {
output.write(x);
}
};
writeToOutputs(`💻 ${command.join(' ')}\n`);

const env = options.env ?? (options.modEnv ? { ...process.env, ...options.modEnv } : undefined);

Expand All @@ -206,17 +211,17 @@ export async function shellWithAction(
out.push(Buffer.from(chunk));
if (!actionExecuted && typeof filter === 'string' && Buffer.concat(out).toString('utf-8').includes(filter) && typeof action === 'function') {
actionExecuted = true;
options.output?.write('before executing action');
writeToOutputs('before executing action');
action().then((output) => {
options.output?.write(`action output is ${output}`);
writeToOutputs(`action output is ${output}`);
actionOutput = output;
actionSucceeded = true;
}).catch((error) => {
options.output?.write(`action error is ${error}`);
writeToOutputs(`action error is ${error}`);
actionSucceeded = false;
actionOutput = error;
}).finally(() => {
options.output?.write('terminate sam sub process');
writeToOutputs('terminate sam sub process');
killSubProcess(child, command.join(' '));
});
}
Expand All @@ -235,13 +240,13 @@ export async function shellWithAction(
}

child.stdout!.on('data', chunk => {
options.output?.write(chunk);
writeToOutputs(chunk);
stdout.push(chunk);
executeAction(chunk);
});

child.stderr!.on('data', chunk => {
options.output?.write(chunk);
writeToOutputs(chunk);
if (options.captureStderr ?? true) {
stderr.push(chunk);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1662,7 +1662,7 @@ integTest(
const targetName = template.replace(/.js$/, '');
await shell([process.execPath, template, '>', targetName], {
cwd: cxAsmDir,
output: fixture.output,
outputs: [fixture.output],
modEnv: {
TEST_ACCOUNT: await fixture.aws.account(),
TEST_REGION: fixture.aws.region,
Expand Down

0 comments on commit 0d9070e

Please sign in to comment.