fix(integ-runner): catch snapshot errors, treat --from-file as command-line (#20523)

Snapshot errors
---------------

The constructor of `IntegSnapshotRunner` calls `loadManifest()`, which
on my computer happened to fail and stopped the entire test suite
because this error happened outside the `try/catch` block. Same for
the integ runner itself.

Move it inside the try/catch block, needed a bit of refactoring to
make sure we could still get at the test name.

`--from-file`
-------------

Instead of having `--from-file` require a JSON file with its own
structure, interpret it as a text file which gets treated exactly
the same as the `[TEST [..]]` arguments on the command line.

This still allows for the `--exclude` behavior by setting that flag
on the command-line.

Also be very liberal on the pattern (file name or test name or display
name) that we accept, encoded in the `IntegTest.matches()` class.

Refactoring
-----------

Moved the logic around determining test names and directories into a
class (`IntegTest`) which is a convenience class on top of a static data
record (`IntegTestInfo`). The split between data and logic is so that we
can pass the data to worker threads where we can hydrate the helper
class on top again. I tried to be consistent: in memory, all the fields are 
with respect to `process.cwd()`, so valid file paths in the current
process. Only when they are passed to the CLI wrapper are the paths
made relative to the CLI wrapper directory.

Print snapshot validations that are running for a long time (1 minute).
This helps diagnose what is stuck, if anything is. On my machine, it
was tests using Docker because there was some issue with it, and this
lost me a day. Also change the test reporting formatting slightly.

--------------

*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/integ-runner/lib/cli.ts

@@ -1,9 +1,10 @@
 // Exercise all integ stacks and if they deploy, update the expected synth files
+import { promises as fs } from 'fs';
 import * as path from 'path';
 import * as chalk from 'chalk';
 import * as workerpool from 'workerpool';
 import * as logger from './logger';
-import { IntegrationTests, IntegTestConfig } from './runner/integration-tests';
+import { IntegrationTests, IntegTestInfo, IntegTest } from './runner/integration-tests';
 import { runSnapshotTests, runIntegrationTests, IntegRunnerMetrics, IntegTestWorkerConfig, DestructiveChange } from './workers';
 
 // https://github.com/yargs/yargs/issues/1929
@@ -25,8 +26,8 @@ async function main() {
     .options('directory', { type: 'string', default: 'test', desc: 'starting directory to discover integration tests. Tests will be discovered recursively from this directory' })
     .options('profiles', { type: 'array', desc: 'list of AWS profiles to use. Tests will be run in parallel across each profile+regions', nargs: 1, default: [] })
     .options('max-workers', { type: 'number', desc: 'The max number of workerpool workers to use when running integration tests in parallel', default: 16 })
-    .options('exclude', { type: 'boolean', desc: 'All tests should be run, except for the list of tests provided', default: false })
-    .options('from-file', { type: 'string', desc: 'Import tests to include or exclude from a file' })
+    .options('exclude', { type: 'boolean', desc: 'Run all tests in the directory, except the specified TESTs', default: false })
+    .options('from-file', { type: 'string', desc: 'Read TEST names from a file (one TEST per line)' })
     .option('inspect-failures', { type: 'boolean', desc: 'Keep the integ test cloud assembly if a failure occurs for inspection', default: false })
     .option('disable-update-workflow', { type: 'boolean', default: false, desc: 'If this is "true" then the stack update workflow will be disabled' })
     .strict()
@@ -39,7 +40,7 @@ async function main() {
   // list of integration tests that will be executed
   const testsToRun: IntegTestWorkerConfig[] = [];
   const destructiveChanges: DestructiveChange[] = [];
-  const testsFromArgs: IntegTestConfig[] = [];
+  const testsFromArgs: IntegTest[] = [];
   const parallelRegions = arrayFromYargs(argv['parallel-regions']);
   const testRegions: string[] = parallelRegions ?? ['us-east-1', 'us-east-2', 'us-west-2'];
   const profiles = arrayFromYargs(argv.profiles);
@@ -56,19 +57,18 @@ async function main() {
   try {
     if (argv.list) {
       const tests = await new IntegrationTests(argv.directory).fromCliArgs();
-      process.stdout.write(tests.map(t => t.fileName).join('\n') + '\n');
+      process.stdout.write(tests.map(t => t.discoveryRelativeFileName).join('\n') + '\n');
       return;
     }
 
     if (argv._.length > 0 && fromFile) {
       throw new Error('A list of tests cannot be provided if "--from-file" is provided');
-    } else if (argv._.length === 0 && !fromFile) {
-      testsFromArgs.push(...(await new IntegrationTests(path.resolve(argv.directory)).fromCliArgs()));
-    } else if (fromFile) {
-      testsFromArgs.push(...(await new IntegrationTests(path.resolve(argv.directory)).fromFile(path.resolve(fromFile))));
-    } else {
-      testsFromArgs.push(...(await new IntegrationTests(path.resolve(argv.directory)).fromCliArgs(argv._.map((x: any) => x.toString()), exclude)));
     }
+    const requestedTests = fromFile
+      ? (await fs.readFile(fromFile, { encoding: 'utf8' })).split('\n').filter(x => x)
+      : (argv._.length > 0 ? argv._ : undefined); // 'undefined' means no request
+
+    testsFromArgs.push(...(await new IntegrationTests(path.resolve(argv.directory)).fromCliArgs(requestedTests, exclude)));
 
     // always run snapshot tests, but if '--force' is passed then
     // run integration tests on all failed tests, not just those that
@@ -85,7 +85,7 @@ async function main() {
     } else {
       // if any of the test failed snapshot tests, keep those results
       // and merge with the rest of the tests from args
-      testsToRun.push(...mergeTests(testsFromArgs, failedSnapshots));
+      testsToRun.push(...mergeTests(testsFromArgs.map(t => t.info), failedSnapshots));
     }
 
     // run integration tests if `--update-on-failed` OR `--force` is used
@@ -174,7 +174,7 @@ function arrayFromYargs(xs: string[]): string[] | undefined {
  * tests that failed snapshot tests. The failed snapshot tests have additional
  * information that we want to keep so this should override any test from args
  */
-function mergeTests(testFromArgs: IntegTestConfig[], failedSnapshotTests: IntegTestWorkerConfig[]): IntegTestWorkerConfig[] {
+function mergeTests(testFromArgs: IntegTestInfo[], failedSnapshotTests: IntegTestWorkerConfig[]): IntegTestWorkerConfig[] {
   const failedTestNames = new Set(failedSnapshotTests.map(test => test.fileName));
   const final: IntegTestWorkerConfig[] = failedSnapshotTests;
   final.push(...testFromArgs.filter(test => !failedTestNames.has(test.fileName)));

packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts

@@ -72,7 +72,8 @@ export class IntegTestRunner extends IntegRunner {
    * all branches and we then search for one that starts with `HEAD branch: `
    */
   private checkoutSnapshot(): void {
-    const cwd = path.dirname(this.snapshotDir);
+    const cwd = this.directory;
+
     // https://git-scm.com/docs/git-merge-base
     let baseBranch: string | undefined = undefined;
     // try to find the base branch that the working branch was created from
@@ -98,17 +99,19 @@ export class IntegTestRunner extends IntegRunner {
     // if we found the base branch then get the merge-base (most recent common commit)
     // and checkout the snapshot using that commit
     if (baseBranch) {
+      const relativeSnapshotDir = path.relative(this.directory, this.snapshotDir);
+
       try {
         const base = exec(['git', 'merge-base', 'HEAD', baseBranch], {
           cwd,
         });
-        exec(['git', 'checkout', base, '--', this.relativeSnapshotDir], {
+        exec(['git', 'checkout', base, '--', relativeSnapshotDir], {
           cwd,
         });
       } catch (e) {
         logger.warning('%s\n%s',
           `Could not checkout snapshot directory ${this.snapshotDir} using these commands: `,
-          `git merge-base HEAD ${baseBranch} && git checkout {merge-base} -- ${this.relativeSnapshotDir}`,
+          `git merge-base HEAD ${baseBranch} && git checkout {merge-base} -- ${relativeSnapshotDir}`,
         );
         logger.warning('error: %s', e);
       }
@@ -129,6 +132,9 @@ export class IntegTestRunner extends IntegRunner {
   public runIntegTestCase(options: RunOptions): AssertionResults | undefined {
     let assertionResults: AssertionResults | undefined;
     const actualTestCase = this.actualTestSuite.testSuite[options.testCaseName];
+    if (!actualTestCase) {
+      throw new Error(`Did not find test case name '${options.testCaseName}' in '${Object.keys(this.actualTestSuite.testSuite)}'`);
+    }
     const clean = options.clean ?? true;
     const updateWorkflowEnabled = (options.updateWorkflow ?? true)
       && (actualTestCase.stackUpdateWorkflow ?? true);
@@ -151,7 +157,7 @@ export class IntegTestRunner extends IntegRunner {
         this.cdk.synthFast({
           execCmd: this.cdkApp.split(' '),
           env,
-          output: this.cdkOutDir,
+          output: path.relative(this.directory, this.cdkOutDir),
         });
       }
       // only create the snapshot if there are no assertion assertion results
@@ -170,7 +176,7 @@ export class IntegTestRunner extends IntegRunner {
             all: true,
             force: true,
             app: this.cdkApp,
-            output: this.cdkOutDir,
+            output: path.relative(this.directory, this.cdkOutDir),
             ...actualTestCase.cdkCommandOptions?.destroy?.args,
             context: this.getContext(actualTestCase.cdkCommandOptions?.destroy?.args?.context),
           });
@@ -241,7 +247,7 @@ export class IntegTestRunner extends IntegRunner {
           stacks: expectedTestCase.stacks,
           ...expectedTestCase?.cdkCommandOptions?.deploy?.args,
           context: this.getContext(expectedTestCase?.cdkCommandOptions?.deploy?.args?.context),
-          app: this.relativeSnapshotDir,
+          app: path.relative(this.directory, this.snapshotDir),
           lookups: this.expectedTestSuite?.enableLookups,
         });
       }
@@ -255,9 +261,9 @@ export class IntegTestRunner extends IntegRunner {
           ...actualTestCase.assertionStack ? [actualTestCase.assertionStack] : [],
         ],
         rollback: false,
-        output: this.cdkOutDir,
+        output: path.relative(this.directory, this.cdkOutDir),
         ...actualTestCase?.cdkCommandOptions?.deploy?.args,
-        ...actualTestCase.assertionStack ? { outputsFile: path.join(this.cdkOutDir, 'assertion-results.json') } : undefined,
+        ...actualTestCase.assertionStack ? { outputsFile: path.relative(this.directory, path.join(this.cdkOutDir, 'assertion-results.json')) } : undefined,
         context: this.getContext(actualTestCase?.cdkCommandOptions?.deploy?.args?.context),
         app: this.cdkApp,
       });
@@ -270,7 +276,7 @@ export class IntegTestRunner extends IntegRunner {
 
       if (actualTestCase.assertionStack) {
         return this.processAssertionResults(
-          path.join(this.directory, this.cdkOutDir, 'assertion-results.json'),
+          path.join(this.cdkOutDir, 'assertion-results.json'),
           actualTestCase.assertionStack,
         );
       }

packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts

@@ -1,22 +1,118 @@
 import * as path from 'path';
 import * as fs from 'fs-extra';
 
+const CDK_OUTDIR_PREFIX = 'cdk-integ.out';
+
 /**
  * Represents a single integration test
+ *
+ * This type is a data-only structure, so it can trivially be passed to workers.
+ * Derived attributes are calculated using the `IntegTest` class.
  */
-export interface IntegTestConfig {
+export interface IntegTestInfo {
   /**
-   * The name of the file that contains the
-   * integration tests. This will be in the format
-   * of integ.{test-name}.js
+   * Path to the file to run
+   *
+   * Path is relative to the current working directory.
    */
   readonly fileName: string;
 
   /**
-   * The base directory where the tests are
-   * discovered from
+   * The root directory we discovered this test from
+   *
+   * Path is relative to the current working directory.
    */
-  readonly directory: string;
+  readonly discoveryRoot: string;
+}
+
+/**
+ * Derived information for IntegTests
+ */
+export class IntegTest {
+  /**
+   * The name of the file to run
+   *
+   * Path is relative to the current working directory.
+   */
+  public readonly fileName: string;
+
+  /**
+   * Relative path to the file to run
+   *
+   * Relative from the "discovery root".
+   */
+  public readonly discoveryRelativeFileName: string;
+
+  /**
+   * The absolute path to the file
+   */
+  public readonly absoluteFileName: string;
+
+  /**
+   * Directory the test is in
+   */
+  public readonly directory: string;
+
+  /**
+   * Display name for the test
+   *
+   * Depends on the discovery directory.
+   *
+   * Looks like `integ.mytest` or `package/test/integ.mytest`.
+   */
+  public readonly testName: string;
+
+  /**
+   * Path of the snapshot directory for this test
+   */
+  public readonly snapshotDir: string;
+
+  /**
+   * Path to the temporary output directory for this test
+   */
+  public readonly temporaryOutputDir: string;
+
+  constructor(public readonly info: IntegTestInfo) {
+    this.absoluteFileName = path.resolve(info.fileName);
+    this.fileName = path.relative(process.cwd(), info.fileName);
+
+    const parsed = path.parse(this.fileName);
+    this.discoveryRelativeFileName = path.relative(info.discoveryRoot, info.fileName);
+    this.directory = parsed.dir;
+
+    // if we are running in a package directory then just use the fileName
+    // as the testname, but if we are running in a parent directory with
+    // multiple packages then use the directory/filename as the testname
+    //
+    // Looks either like `integ.mytest` or `package/test/integ.mytest`.
+    const relDiscoveryRoot = path.relative(process.cwd(), info.discoveryRoot);
+    this.testName = this.directory === path.join(relDiscoveryRoot, 'test') || this.directory === path.join(relDiscoveryRoot)
+      ? parsed.name
+      : path.join(path.relative(this.info.discoveryRoot, parsed.dir), parsed.name);
+
+    const nakedTestName = parsed.name.slice(6); // Leave name without 'integ.' and '.ts'
+    this.snapshotDir = path.join(this.directory, `${nakedTestName}.integ.snapshot`);
+    this.temporaryOutputDir = path.join(this.directory, `${CDK_OUTDIR_PREFIX}.${nakedTestName}`);
+  }
+
+  /**
+   * Whether this test matches the user-given name
+   *
+   * We are very lenient here. A name matches if it matches:
+   *
+   * - The CWD-relative filename
+   * - The discovery root-relative filename
+   * - The suite name
+   * - The absolute filename
+   */
+  public matches(name: string) {
+    return [
+      this.fileName,
+      this.discoveryRelativeFileName,
+      this.testName,
+      this.absoluteFileName,
+    ].includes(name);
+  }
 }
 
 /**
@@ -49,7 +145,7 @@ export class IntegrationTests {
    * Takes a file name of a file that contains a list of test
    * to either run or exclude and returns a list of Integration Tests to run
    */
-  public async fromFile(fileName: string): Promise<IntegTestConfig[]> {
+  public async fromFile(fileName: string): Promise<IntegTest[]> {
     const file: IntegrationTestFileConfig = JSON.parse(fs.readFileSync(fileName, { encoding: 'utf-8' }));
     const foundTests = await this.discover();
 
@@ -65,32 +161,27 @@ export class IntegrationTests {
    *   If they have provided a test name that we don't find, then we write out that error message.
    * - If it is a list of tests to exclude, then we discover all available tests and filter out the tests that were provided by the user.
    */
-  private filterTests(discoveredTests: IntegTestConfig[], requestedTests?: string[], exclude?: boolean): IntegTestConfig[] {
-    if (!requestedTests || requestedTests.length === 0) {
+  private filterTests(discoveredTests: IntegTest[], requestedTests?: string[], exclude?: boolean): IntegTest[] {
+    if (!requestedTests) {
       return discoveredTests;
     }
-    const all = discoveredTests.map(x => {
-      return path.relative(x.directory, x.fileName);
-    });
-    let foundAll = true;
-    // Pare down found tests to filter
+
+
     const allTests = discoveredTests.filter(t => {
-      if (exclude) {
-        return (!requestedTests.includes(path.relative(t.directory, t.fileName)));
-      }
-      return (requestedTests.includes(path.relative(t.directory, t.fileName)));
+      const matches = requestedTests.some(pattern => t.matches(pattern));
+      return matches !== !!exclude; // Looks weird but is equal to (matches && !exclude) || (!matches && exclude)
     });
 
+    // If not excluding, all patterns must have matched at least one test
     if (!exclude) {
-      const selectedNames = allTests.map(t => path.relative(t.directory, t.fileName));
-      for (const unmatched of requestedTests.filter(t => !selectedNames.includes(t))) {
+      const unmatchedPatterns = requestedTests.filter(pattern => !discoveredTests.some(t => t.matches(pattern)));
+      for (const unmatched of unmatchedPatterns) {
         process.stderr.write(`No such integ test: ${unmatched}\n`);
-        foundAll = false;
       }
-    }
-    if (!foundAll) {
-      process.stderr.write(`Available tests: ${all.join(' ')}\n`);
-      return [];
+      if (unmatchedPatterns.length > 0) {
+        process.stderr.write(`Available tests: ${discoveredTests.map(t => t.discoveryRelativeFileName).join(' ')}\n`);
+        return [];
+      }
     }
 
     return allTests;
@@ -99,23 +190,26 @@ export class IntegrationTests {
   /**
    * Takes an optional list of tests to look for, otherwise
    * it will look for all tests from the directory
+   *
+   * @param tests Tests to include or exclude, undefined means include all tests.
+   * @param exclude Whether the 'tests' list is inclusive or exclusive (inclusive by default).
    */
-  public async fromCliArgs(tests?: string[], exclude?: boolean): Promise<IntegTestConfig[]> {
+  public async fromCliArgs(tests?: string[], exclude?: boolean): Promise<IntegTest[]> {
     const discoveredTests = await this.discover();
 
     const allTests = this.filterTests(discoveredTests, tests, exclude);
 
     return allTests;
   }
 
-  private async discover(): Promise<IntegTestConfig[]> {
+  private async discover(): Promise<IntegTest[]> {
     const files = await this.readTree();
     const integs = files.filter(fileName => path.basename(fileName).startsWith('integ.') && path.basename(fileName).endsWith('.js'));
     return this.request(integs);
   }
 
-  private request(files: string[]): IntegTestConfig[] {
-    return files.map(fileName => { return { directory: this.directory, fileName }; });
+  private request(files: string[]): IntegTest[] {
+    return files.map(fileName => new IntegTest({ discoveryRoot: this.directory, fileName }));
   }
 
   private async readTree(): Promise<string[]> {