fix(integ-runner): fail with correct error when integ manifest exists but cannot be loaded (#692)

Previously if an integ manifest exists, but could not be loaded for some
reason we would be falling back to the deprecated legacy style tests.
This code path will than always fail with a misleading error:
`"integ-runner" can only operate on apps with a single stack.` The error
is because the modern integ tests always have include stacks, but legacy
tests only handle a single stack or a pragma list.

This change improves the error handling to distinguish between "no integ
manifest" and "failed to load manifest". For the former, we still go
down the legacy test route. For the latter we can now fail the test with
an accurate error message.

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

Author: mrgrain

packages/@aws-cdk/cloud-assembly-schema/lib/integ-tests/schema.ts

@@ -11,7 +11,7 @@ export interface IntegManifest {
   /**
    * Enable lookups for this test. If lookups are enabled
    * then `stackUpdateWorkflow` must be set to false.
-   * Lookups should only be enabled when you are explicitely testing
+   * Lookups should only be enabled when you are explicitly testing
    * lookups.
    *
    * @default false

packages/@aws-cdk/cloud-assembly-schema/projenrc/versioning.ts

@@ -4,14 +4,15 @@ import * as path from 'path';
 import { SCHEMA_DIR } from './schema-definition';
 
 export function maybeBumpVersion(schemas: Record<string, any>) {
+  const $comment = "Do not hold back the version on additions: jsonschema validation of the manifest by the consumer will trigger errors on unexpected fields.";
   const serializedSchema = JSON.stringify(sortJson(schemas), null, 2);
 
   const versionFile = path.join(SCHEMA_DIR, 'version.json');
   let current: SchemaVersionFile = JSON.parse(fs.readFileSync(versionFile, 'utf8'));
   const schemaHash = sha256(serializedSchema);
 
   if (current.schemaHash !== schemaHash) {
-    current = { schemaHash, revision: current.revision + 1 };
+    current = { schemaHash, $comment, revision: current.revision + 1 };
     console.log(`Schemas changed, bumping version to ${current.revision}`);
   }
 
@@ -26,6 +27,7 @@ function sha256(x: string) {
 
 interface SchemaVersionFile {
   revision: number;
+  $comment?: string;
   schemaHash: string;
 }
 

packages/@aws-cdk/cloud-assembly-schema/schema/integ.schema.json

@@ -10,7 +10,7 @@
                     "type": "string"
                 },
                 "enableLookups": {
-                    "description": "Enable lookups for this test. If lookups are enabled\nthen `stackUpdateWorkflow` must be set to false.\nLookups should only be enabled when you are explicitely testing\nlookups.",
+                    "description": "Enable lookups for this test. If lookups are enabled\nthen `stackUpdateWorkflow` must be set to false.\nLookups should only be enabled when you are explicitly testing\nlookups.",
                     "default": false,
                     "type": "boolean"
                 },

packages/@aws-cdk/cloud-assembly-schema/schema/version.json

@@ -1,5 +1,5 @@
 {
-  "schemaHash": "4709e40bc5dd6ad7d2bb6c6bd0315c622b7f4aa07ee3af66ff07f9bc77fdf783",
+  "schemaHash": "0807b42f8220bdafddeb4dad246a696721ed86e99ac6b13aa83359bab834d60d",
   "$comment": "Do not hold back the version on additions: jsonschema validation of the manifest by the consumer will trigger errors on unexpected fields.",
   "revision": 45
 }

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

@@ -18,3 +18,4 @@ export const error = logger(stderr, [chalk.red]);
 export const warning = logger(stderr, [chalk.yellow]);
 export const success = logger(stderr, [chalk.green]);
 export const highlight = logger(stderr, [chalk.bold]);
+export const trace = logger(stderr, [chalk.gray]);

packages/@aws-cdk/integ-runner/lib/runner/private/integ-manifest.ts

@@ -3,6 +3,44 @@ import type { IntegManifest, TestCase } from '@aws-cdk/cloud-assembly-schema';
 import { Manifest } from '@aws-cdk/cloud-assembly-schema';
 import * as fs from 'fs-extra';
 
+/**
+ * An error indicating that the manifest file does not exists.
+ *
+ * This can signal to treat the integ test case as a legacy test.
+ */
+export class NoManifestError extends Error {
+  public readonly manifestPath: string;
+  public readonly cause: Error;
+
+  constructor(manifestPath: string, cause: Error) {
+    super(`Cannot read integ manifest '${manifestPath}': ${cause.message}`);
+    this.name = 'NoManifestError';
+    this.manifestPath = manifestPath;
+    this.cause = cause;
+
+    Object.setPrototypeOf(this, NoManifestError.prototype);
+  }
+}
+
+/**
+ * An error indicating that the manifest failed to load.
+ *
+ * The error implies the the manifest file exists, but is invalid.
+ */
+export class ManifestLoadError extends Error {
+  public readonly manifestPath: string;
+  public readonly cause: Error;
+
+  constructor(manifestPath: string, cause: Error) {
+    super(`Failed to load integ manifest '${manifestPath}': ${cause.message}`);
+    this.name = 'ManifestLoadingError';
+    this.manifestPath = manifestPath;
+    this.cause = cause;
+
+    Object.setPrototypeOf(this, ManifestLoadError.prototype);
+  }
+}
+
 /**
  * Test case configuration read from the integ manifest
  */
@@ -39,11 +77,17 @@ export class IntegManifestReader {
    * Reads an integration test manifest from the specified file
    */
   public static fromFile(fileName: string): IntegManifestReader {
+    try {
+      fs.statSync(fileName);
+    } catch (e: any) {
+      throw new NoManifestError(fileName, e);
+    }
+
     try {
       const obj = Manifest.loadIntegManifest(fileName);
       return new IntegManifestReader(path.dirname(fileName), obj);
-    } catch (e: any) {
-      throw new Error(`Cannot read integ manifest '${fileName}': ${e.message}`);
+    } catch (loadErr: any) {
+      throw new ManifestLoadError(fileName, loadErr);
     }
   }
 
@@ -57,7 +101,7 @@ export class IntegManifestReader {
     try {
       st = fs.statSync(filePath);
     } catch (e: any) {
-      throw new Error(`Cannot read integ manifest at '${filePath}': ${e.message}`);
+      throw new NoManifestError(filePath, e);
     }
     if (st.isDirectory()) {
       return IntegManifestReader.fromFile(path.join(filePath, IntegManifestReader.DEFAULT_FILENAME));

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

@@ -9,9 +9,11 @@ import type { IntegTest } from './integration-tests';
 import * as recommendedFlagsFile from '../recommended-feature-flags.json';
 import { flatten } from '../utils';
 import { makeEngine, type EngineOptions } from './engine';
+import * as logger from '../logger';
 import type { ManifestTrace } from './private/cloud-assembly';
 import { AssemblyManifestReader } from './private/cloud-assembly';
 import type { DestructiveChange } from '../workers/common';
+import { NoManifestError } from './private/integ-manifest';
 
 const DESTRUCTIVE_CHANGES = '!!DESTRUCTIVE_CHANGES:';
 
@@ -128,6 +130,11 @@ export abstract class IntegRunner {
    */
   protected readonly profile?: string;
 
+  /**
+   * Show output from the integ test run.
+   */
+  protected readonly showOutput: boolean;
+
   protected _destructiveChanges?: DestructiveChange[];
   private legacyContext?: Record<string, any>;
   private _expectedTestSuite?: IntegTestSuite | LegacyIntegTestSuite;
@@ -139,14 +146,14 @@ export abstract class IntegRunner {
     this.testName = this.test.testName;
     this.snapshotDir = this.test.snapshotDir;
     this.cdkContextPath = path.join(this.directory, 'cdk.context.json');
+    this.profile = options.profile;
+    this.showOutput = options.showOutput ?? false;
 
     this.cdk = options.cdk ?? makeEngine(options);
     this.cdkOutDir = options.integOutDir ?? this.test.temporaryOutputDir;
 
     const testRunCommand = this.test.appCommand;
     this.cdkApp = testRunCommand.replace('{filePath}', path.relative(this.directory, this.test.fileName));
-
-    this.profile = options.profile;
   }
 
   /**
@@ -219,10 +226,25 @@ export abstract class IntegRunner {
    * "legacy mode" and create a manifest from pragma
    */
   protected async loadManifest(dir?: string): Promise<IntegTestSuite | LegacyIntegTestSuite> {
+    const manifest = dir ?? this.snapshotDir;
     try {
-      const testSuite = IntegTestSuite.fromPath(dir ?? this.snapshotDir);
+      const testSuite = IntegTestSuite.fromPath(manifest);
       return testSuite;
-    } catch {
+    } catch (modernError: any) {
+      // Only attempt legacy test case if the integ test manifest was not found
+      // For any other errors, e.g. when parsing the manifest fails, we abort.
+      if (!(modernError instanceof NoManifestError)) {
+        throw modernError;
+      }
+
+      if (this.showOutput) {
+        logger.trace(
+          "Failed to load integ test manifest for '%s'. Attempting as deprecated legacy test instead. Error was: %s",
+          manifest,
+          modernError.message ?? String(modernError),
+        );
+      }
+
       const testCases = await LegacyIntegTestSuite.fromLegacy({
         cdk: this.cdk,
         testName: this.test.normalizedTestName,

packages/@aws-cdk/integ-runner/lib/workers/extract/extract_worker.ts

@@ -143,6 +143,7 @@ export async function snapshotTestWorker(testInfo: IntegTestInfo, options: Snaps
     const runner = new IntegSnapshotRunner({
       engine: options.engine,
       test,
+      showOutput: options.verbose ?? false,
     });
     if (!runner.hasSnapshot()) {
       workerpool.workerEmit({

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

@@ -1,5 +1,4 @@
-import type { ChildProcess } from 'child_process';
-import { Readable, Writable } from 'stream';
+import * as path from 'path';
 import type { CdkCliWrapperOptions, DeployOptions, ICdk, ListOptions, SynthFastOptions, SynthOptions } from '@aws-cdk/cdk-cli-wrapper';
 import type { DestroyOptions } from '@aws-cdk/cloud-assembly-schema/lib/integ-tests';
 import { IntegSnapshotRunner, IntegTest } from '../lib/runner';
@@ -106,3 +105,10 @@ export class MockCdkProvider {
     return results;
   }
 }
+
+/**
+ * Get the absolute path to a data located the test-data directory
+ */
+export function testDataPath(...location: string[]): string {
+  return path.join(__dirname, 'test-data', ...location);
+}

packages/@aws-cdk/integ-runner/test/runner/private/invalid-manifest.test.ts

@@ -0,0 +1,21 @@
+import { IntegTestSuite } from '../../../lib/runner/integ-test-suite';
+import { IntegManifestReader, ManifestLoadError } from '../../../lib/runner/private/integ-manifest';
+import { testDataPath } from '../../helpers';
+
+describe('Invalid Manifest Handling', () => {
+  test('throws ManifestLoadError when loading an invalid JSON manifest', () => {
+    // GIVEN
+    const invalidManifestPath = testDataPath('invalid-integ-manifest', 'integ.json');
+
+    // WHEN / THEN
+    expect(() => IntegManifestReader.fromFile(invalidManifestPath)).toThrow(ManifestLoadError);
+  });
+
+  test('IntegTestSuite.fromPath propagates ManifestLoadError', () => {
+    // GIVEN
+    const invalidManifestDir = testDataPath('invalid-integ-manifest');
+
+    // WHEN / THEN
+    expect(() => IntegTestSuite.fromPath(invalidManifestDir)).toThrow(ManifestLoadError);
+  });
+});