chore(cli): integ tests don't buffer shell output (#31903)

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.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Author: rix0rrr

packages/@aws-cdk-testing/cli-integ/lib/shell.ts

@@ -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);
@@ -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);
       }
@@ -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}.`));
       }
@@ -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
@@ -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,
     });

packages/@aws-cdk-testing/cli-integ/lib/staging/maven.ts

@@ -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(),

packages/@aws-cdk-testing/cli-integ/lib/staging/npm.ts

@@ -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}`);

packages/@aws-cdk-testing/cli-integ/lib/staging/nuget.ts

@@ -25,7 +25,7 @@ export async function uploadDotnetPackages(packages: string[], usageDir: UsageDi
       '--disable-buffering',
       '--timeout', '600',
       '--skip-duplicate'], {
-      output,
+      outputs: [output],
     });
 
     console.log(`✅ ${pkg}`);

packages/@aws-cdk-testing/cli-integ/lib/staging/pypi.ts

@@ -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}`);

packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts

@@ -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 {

packages/@aws-cdk-testing/cli-integ/lib/with-sam.ts

@@ -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,
     });
@@ -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);
 
@@ -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(' '));
         });
       }
@@ -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);
       }

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

@@ -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,