diff --git a/packages/aws-cdk-lib/core/test/aspect.prop.test.ts b/packages/aws-cdk-lib/core/test/aspect.prop.test.ts index 4d58b9092a979..de32caa7dc19a 100644 --- a/packages/aws-cdk-lib/core/test/aspect.prop.test.ts +++ b/packages/aws-cdk-lib/core/test/aspect.prop.test.ts @@ -3,6 +3,14 @@ import * as fc from 'fast-check'; import * as fs from 'fs-extra'; import { App, AppProps, AspectApplication, Aspects, IAspect } from '../lib'; +// Control number of runs from an env var for a burn-in test +if (process.env.FAST_CHECK_NUM_RUNS) { + fc.configureGlobal({ + ...fc.readConfigureGlobal(), + numRuns: Number(process.env.FAST_CHECK_NUM_RUNS), + }); +} + ////////////////////////////////////////////////////////////////////// // Tests @@ -11,9 +19,9 @@ describe('every aspect gets invoked exactly once', () => { fc.assert( fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => { afterSynth((testApp) => { - forEveryVisitPair(testApp.visitLog, (a, b) => { + forEveryVisitPair(testApp.actionLog, (a, b) => { if (sameConstruct(a, b) && sameAspect(a, b)) { - throw new Error(`Duplicate visit: ${a.index} and ${b.index}`); + throw new Error(`Duplicate visit: t=${a.index} and t=${b.index}`); } }); }, stabilizeAspects)(app); @@ -27,14 +35,14 @@ describe('every aspect gets invoked exactly once', () => { const originalConstructsOnApp = app.cdkApp.node.findAll(); const originalAspectApplications = getAllAspectApplications(originalConstructsOnApp); afterSynth((testApp) => { - const visitsMap = getVisitsMap(testApp.visitLog); + const visitsMap = getVisitsMap(testApp.actionLog); for (const aspectApplication of originalAspectApplications) { // Check that each original AspectApplication also visited all child nodes of its original scope: for (const construct of originalConstructsOnApp) { if (isAncestorOf(aspectApplication.construct, construct)) { if (!visitsMap.get(construct)!.includes(aspectApplication.aspect)) { - throw new Error(`Aspect ${aspectApplication.aspect.constructor.name} applied on ${aspectApplication.construct.node.path} did not visit construct ${construct.node.path} in its original scope.`); + throw new Error(`Aspect ${aspectApplication.aspect} applied on ${aspectApplication.construct.node.path} did not visit construct ${construct.node.path} in its original scope.`); } } } @@ -51,7 +59,7 @@ describe('every aspect gets invoked exactly once', () => { const allConstructsOnApp = testApp.cdkApp.node.findAll(); const allAspectApplications = getAllAspectApplications(allConstructsOnApp); - const visitsMap = getVisitsMap(testApp.visitLog); + const visitsMap = getVisitsMap(testApp.actionLog); for (const aspectApplication of allAspectApplications) { // Check that each AspectApplication also visited all child nodes of its scope: @@ -147,7 +155,7 @@ test('visitLog is nonempty', () => fc.assert( fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => { afterSynth((testApp) => { - expect(testApp.visitLog).not.toEqual([]); + expect(testApp.actionLog).not.toEqual([]); }, stabilizeAspects)(app); }), ), @@ -156,11 +164,11 @@ test('visitLog is nonempty', () => ////////////////////////////////////////////////////////////////////// // Test Helpers -function afterSynth(block: (x: PrettyApp) => void, stabilizeAspects: boolean) { - return (app) => { +function afterSynth(block: (x: PrettyApp) => void, aspectStabilization: boolean) { + return (app: PrettyApp) => { let asm; try { - asm = app.cdkApp.synth({ aspectStabilization: stabilizeAspects }); + asm = app.cdkApp.synth({ aspectStabilization }); } catch (error) { if (error.message.includes('Cannot invoke Aspect')) { return; @@ -183,7 +191,7 @@ function implies(a: boolean, b: boolean) { } interface AspectVisitWithIndex extends AspectVisit { - index: number; + readonly index: number; } /** @@ -192,12 +200,14 @@ interface AspectVisitWithIndex extends AspectVisit { * This is humongously inefficient at large scale, so we might need more clever * algorithms to keep this tractable. */ -function forEveryVisitPair(log: AspectVisitLog, block: (a: AspectVisitWithIndex, b: AspectVisitWithIndex) => void) { +function forEveryVisitPair(log: AspectActionLog, block: (a: AspectVisitWithIndex, b: AspectVisitWithIndex) => void) { for (let i = 0; i < log.length; i++) { for (let j = 0; j < log.length; j++) { - if (i === j) { continue; } + const logI = log[i]; + const logJ = log[j]; + if (i ===j || logI.action !== 'visit' || logJ.action !== 'visit') { continue; } - block({ ...log[i], index: i }, { ...log[j], index: j }); + block({ ...logI, index: i }, { ...logJ, index: j }); } } } @@ -218,10 +228,11 @@ function arraysEqualUnordered(arr1: T[], arr2: T[]): boolean { * Given an AspectVisitLog, returns a map of Constructs with a list of all Aspects that * visited the construct. */ -function getVisitsMap(log: AspectVisitLog): Map { +function getVisitsMap(log: AspectActionLog): Map { const visitsMap = new Map(); for (let i = 0; i < log.length; i++) { const visit = log[i]; + if (visit.action !== 'visit') { continue; } if (!visitsMap.has(visit.construct)) { visitsMap.set(visit.construct, []); } @@ -270,8 +281,8 @@ function ancestors(a: Construct): IConstruct[] { /** * Returns all aspects of the given construct's ancestors (excluding its own locally defined aspects) */ -function allAncestorAspects(a: IConstruct): IAspect[] { - const ancestorConstructs = ancestors(a); +function allAncestorAspects(c: IConstruct): IAspect[] { + const ancestorConstructs = ancestors(c); // Filter out the current node and get aspects of the ancestors return ancestorConstructs @@ -292,11 +303,17 @@ function allAspectApplicationsInScope(a: Construct): AspectApplication[] { * Take the minimum of all added applications that could lead to this execution. */ function aspectAppliedT(prettyApp: PrettyApp, a: IAspect, c: Construct): number { - const potentialTimes = [...prettyApp.initialAspects, ...prettyApp.addedAspects].filter((appl) => { - return appl.aspect === a && isAncestorOf(appl.construct, c); - }).map((appl) => appl.t); + for (let i = 0; i < prettyApp.actionLog.length; i++) { + const visit = prettyApp.actionLog[i]; + if (visit.action !== 'aspectApplied') { continue; } + + if (visit.aspect === a && isAncestorOf(visit.construct, c)) { + return i; + } + } - return Math.min(...potentialTimes); + // Must have been there already from the start + return -1; } /** @@ -320,9 +337,9 @@ function lowestAspectPrioFor(a: IAspect, c: IConstruct) { // Arbitraries function appWithAspects() { - return arbCdkAppFactory().chain(arbAddAspects).map(([a, l]) => { - return new PrettyApp(a, l); - }); + return arbCdkAppFactory() + .chain((a) => fc.tuple(fc.constant(a), arbAspectApplications(a))) + .map(([a, l]) => buildApplication(a, l)); } /** @@ -368,40 +385,27 @@ function arbConstructTreeFactory(): fc.Arbitrary { class PrettyApp { private readonly initialTree: Set; private readonly _initialAspects: Map>; - public readonly initialAspects: RecordedAspectApplication[]; constructor(public readonly cdkApp: App, public readonly executionState: ExecutionState) { const constructs = cdkApp.node.findAll(); this.initialTree = new Set(constructs.map(c => c.node.path)); this._initialAspects = new Map(constructs.map(c => [c.node.path, new Set(renderAspects(c))])); - - this.initialAspects = constructs.flatMap(c => Aspects.of(c).applied.map(a => ({ - aspect: a.aspect, - construct: a.construct, - priority: a.priority, - t: 0, - } as RecordedAspectApplication))); } /** * Return the log of all aspect visits */ - public get visitLog() { - return this.executionState.visitLog; - } - - /** - * Return a list of all constructs added by aspects - */ - public get addedConstructs() { - return this.executionState.addedConstructs; + public get actionLog() { + return this.executionState.actionLog; } /** * Return a list of all aspects added by other aspects */ public get addedAspects() { - return this.executionState.addedAspects; + return this.executionState.actionLog + .map((visit, i) => [i, visit] as const) + .filter(([_, visit]) => visit.action === 'aspectApplied'); } public toString() { @@ -417,8 +421,8 @@ class PrettyApp { lines.push('TREE'); recurse(this.cdkApp); lines.push('VISITS'); - this.visitLog.forEach((visit, i) => { - lines.push(` ${i}. ${visit.construct} <-- ${visit.aspect}`); + this.actionLog.forEach((visit, i) => { + lines.push(` t=${i}. ${renderAspectAction(visit)}`); }); lines.push('', ''); @@ -454,6 +458,17 @@ class PrettyApp { } } +function renderAspectAction(v: AspectAction): string { + switch (v.action) { + case 'visit': + return ` ${v.construct} <-- ${v.aspect}`; + case 'constructAdded': + return `add ${v.construct}`; + case 'aspectApplied': + return `apply ${v.aspect}@${v.priority} to ${v.construct}`; + } +} + function renderAspects(c: Construct) { return unique(Aspects.of(c).applied.map(a => `${a.aspect}@${a.priority}`)); } @@ -470,49 +485,60 @@ function unique(xs: string[]): string[] { } interface AspectVisit { - construct: IConstruct; - aspect: TracingAspect; + readonly action: 'visit'; + readonly construct: IConstruct; + readonly aspect: TracingAspect; +} + +interface ConstructAdded { + readonly action: 'constructAdded'; + readonly construct: IConstruct; +} + +interface AspectApplied extends PartialTestAspectApplication { + readonly action: 'aspectApplied'; + readonly construct: IConstruct; } -type AspectVisitLog = AspectVisit[]; +type AspectAction = AspectVisit | ConstructAdded | AspectApplied; + +type AspectActionLog = AspectAction[]; /** * Add arbitrary aspects to the given tree */ -function arbAddAspects(appFac: AppFactory): fc.Arbitrary<[App, ExecutionState]> { +function arbAspectApplications(appFac: AppFactory): fc.Arbitrary { // Synthesize the tree, but just to get the construct paths to apply aspects to. // We won't hold on to the instances, because we will clone the tree later (or // regenerate it, which is easier), and attach the aspects in the clone. const baseTree = appFac(); const constructs = baseTree.node.findAll(); - const applications = fc.array(arbAspectApplication(constructs), { + return fc.array(arbAspectApplication(constructs), { size: 'small', minLength: 1, maxLength: 5, }); +} - return applications.map((appls) => { - // A fresh tree copy for every tree with aspects. `fast-check` may re-use old values - // when generating variants, so if we mutate the tree in place different runs will - // interfere with each other. Also a different aspect invocation log for every tree. - const tree = appFac(); - const state: ExecutionState = { - visitLog: [], - addedConstructs: [], - addedAspects: [], - }; - tree[EXECUTIONSTATE_SYM] = state; // Stick this somewhere the aspects can find it +function buildApplication(appFac: AppFactory, appls: TestAspectApplication[]): PrettyApp { + // A fresh tree copy for every tree with aspects. `fast-check` may re-use old values + // when generating variants, so if we mutate the tree in place different runs will + // interfere with each other. Also a different aspect invocation log for every tree. + const tree = appFac(); + const state: ExecutionState = { + actionLog: [], + }; + tree[EXECUTIONSTATE_SYM] = state; // Stick this somewhere the aspects can find it - for (const app of appls) { - const ctrs = app.constructPaths.map((p) => findConstructDeep(tree, p)); - for (const ctr of ctrs) { - Aspects.of(ctr).add(app.aspect, { priority: app.priority }); - } + for (const app of appls) { + const ctrs = app.constructPaths.map((p) => findConstructDeep(tree, p)); + for (const ctr of ctrs) { + Aspects.of(ctr).add(app.aspect, { priority: app.priority }); } + } - return [tree, state]; - }); + return new PrettyApp(tree, state); } function arbAspectApplication(constructs: Construct[]): fc.Arbitrary { @@ -559,8 +585,8 @@ function arbConstructLoc(constructs: Construct[]): fc.Arbitrary { * A location for a construct (scope and id) */ interface ConstructLoc { - scope: string; - id: string; + readonly scope: string; + readonly id: string; } /** @@ -580,17 +606,7 @@ interface ExecutionState { /** * Visit log of all aspects */ - visitLog: AspectVisitLog; - - /** - * Constructs that were added by aspects - */ - addedConstructs: Construct[]; - - /** - * Record where we added an aspect - */ - addedAspects: RecordedAspectApplication[]; + readonly actionLog: AspectActionLog; } /** @@ -665,7 +681,8 @@ abstract class TracingAspect implements IAspect { } visit(node: IConstruct): void { - this.executionState(node).visitLog.push({ + this.executionState(node).actionLog.push({ + action: 'visit', aspect: this, construct: node, }); @@ -703,23 +720,15 @@ class MutatingAspect extends TracingAspect { * Contains just the aspect and priority */ interface PartialTestAspectApplication { - aspect: IAspect; - priority?: number; + readonly aspect: IAspect; + readonly priority?: number; } interface TestAspectApplication extends PartialTestAspectApplication { /** * Need to go by path because the constructs themselves are mutable and these paths remain valid in multiple trees */ - constructPaths: string[]; -} - -/** - * An aspect application added by another aspect, during execution - */ -interface RecordedAspectApplication extends PartialTestAspectApplication { - t: number; - construct: Construct; + readonly constructPaths: string[]; } /** @@ -738,19 +747,23 @@ class NodeAddingAspect extends TracingAspect { return; } + const executionState = this.executionState(node); + const newConstruct = new ArbConstruct(scope, this.loc.id); + executionState.actionLog.push({ + action: 'constructAdded', + construct: newConstruct, + }); + for (const { aspect, priority } of this.newAspects) { Aspects.of(newConstruct).add(aspect, { priority }); - const executionState = this.executionState(node); - executionState.addedAspects.push({ - t: executionState.visitLog.length, + executionState.actionLog.push({ + action: 'aspectApplied', construct: newConstruct, aspect, priority, }); } - // Log that we added this construct - this.executionState(node).addedConstructs.push(newConstruct); } public toString() { @@ -771,14 +784,19 @@ class AspectAddingAspect extends TracingAspect { const constructs = this.newAspect.constructPaths.map((p) => findConstructDeep(node.node.root, p)); for (const construct of constructs) { - Aspects.of(construct).add(this.newAspect.aspect, { priority: this.newAspect.priority }); - const executionState = this.executionState(node); - executionState.addedAspects.push({ - t: executionState.visitLog.length, - construct, - aspect: this.newAspect.aspect, - priority: this.newAspect.priority, - }); + const constructAspects = Aspects.of(construct); + const cnt = constructAspects.applied.length; + constructAspects.add(this.newAspect.aspect, { priority: this.newAspect.priority }); + + if (constructAspects.applied.length !== cnt) { + const executionState = this.executionState(node); + executionState.actionLog.push({ + action: 'aspectApplied', + construct, + aspect: this.newAspect.aspect, + priority: this.newAspect.priority, + }); + } } }