fix(integ-runner): catch snapshot errors, treat --from-file as comm…

…and-line

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.

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.

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`). We pass the data record to worker threads.

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);
@@ -55,19 +56,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
@@ -84,7 +84,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
@@ -169,7 +169,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.info.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[]> {