From 0150854bc2e2e021b1f3e0da8e9a2a866afd8e81 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Mon, 23 Dec 2024 21:36:52 +0100 Subject: [PATCH] chore: fix property test for aspects (#32639) The test was asserting that aspects with lower priority would be executed before aspects with higher priority; but this disregarded their application time. If an aspect with lower priority is added before the execution of an aspect happens, there is no way it can execute before it. This fixes error assertions that look like this: ``` TREE +- (root) <-- AddAspect_2822([First,], Mutate_2743@0)@0, Mutate_2743@0 (added), Mutate_2751@0 (added) +- First <-- AddAspect_2822([First,], Mutate_2743@0)@0, AddAspect_2809([], Mutate_2751@0)@0, Mutate_2743@0 (added) +- (added) Tree VISITS 0. <-- AddAspect_2822([First,], Mutate_2743@0) 1. First <-- AddAspect_2822([First,], Mutate_2743@0) 2. First <-- AddAspect_2809([], Mutate_2751@0) 3. First <-- Mutate_2743 4. Tree <-- AddAspect_2822([First,], Mutate_2743@0) Got error: Aspect Mutate_2743@0 at 3 should have been before AddAspect_2809([], Mutate_2751@0)@0 at 2, but was after ``` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-cdk-lib/core/test/aspect.prop.test.ts | 48 +++++++++++++++---- 1 file changed, 39 insertions(+), 9 deletions(-) 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 df2c70a1ade0a..4d58b9092a979 100644 --- a/packages/aws-cdk-lib/core/test/aspect.prop.test.ts +++ b/packages/aws-cdk-lib/core/test/aspect.prop.test.ts @@ -32,7 +32,7 @@ describe('every aspect gets invoked exactly once', () => { 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 (ancestorOf(aspectApplication.construct, construct)) { + 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.`); } @@ -56,7 +56,7 @@ describe('every aspect gets invoked exactly once', () => { for (const aspectApplication of allAspectApplications) { // Check that each AspectApplication also visited all child nodes of its scope: for (const construct of allConstructsOnApp) { - if (ancestorOf(aspectApplication.construct, construct)) { + 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 scope.`); } @@ -107,7 +107,11 @@ test('for every construct, lower priorities go before higher priorities', () => const aPrio = lowestAspectPrioFor(a.aspect, a.construct); const bPrio = lowestAspectPrioFor(b.aspect, b.construct); - if (!implies(aPrio < bPrio, a.index < b.index)) { + // But only if the application of aspect A exists at least as long as the application of aspect B + const aAppliedT = aspectAppliedT(testApp, a.aspect, a.construct); + const bAppliedT = aspectAppliedT(testApp, b.aspect, b.construct); + + if (!implies(aPrio < bPrio && aAppliedT <= bAppliedT, a.index < b.index)) { throw new Error( `Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`, ); @@ -250,7 +254,7 @@ function sameAspect(a: AspectVisit, b: AspectVisit) { /** * Returns whether `a` is an ancestor of `b` (or if they are the same construct) */ -function ancestorOf(a: Construct, b: Construct) { +function isAncestorOf(a: Construct, b: Construct) { return b.node.path === a.node.path || b.node.path.startsWith(a.node.path + '/'); } @@ -282,6 +286,19 @@ function allAspectApplicationsInScope(a: Construct): AspectApplication[] { return ancestors(a).flatMap((c) => Aspects.of(c).applied); } +/** + * Find the lowest timestamp that could lead to the execution of the given aspect on the given construct + * + * 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); + + return Math.min(...potentialTimes); +} + /** * Return the lowest priority of Aspect `a` inside the given list of applications */ @@ -350,12 +367,20 @@ function arbConstructTreeFactory(): fc.Arbitrary { */ class PrettyApp { private readonly initialTree: Set; - private readonly initialAspects: Map>; + 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 = 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))); } /** @@ -408,7 +433,7 @@ class PrettyApp { const aspects = renderAspects(construct); for (let i = 0; i < aspects.length; i++) { - if (!(self.initialAspects.get(construct.node.path) ?? new Set()).has(aspects[i])) { + if (!(self._initialAspects.get(construct.node.path) ?? new Set()).has(aspects[i])) { aspects[i] += ' (added)'; } } @@ -693,6 +718,7 @@ interface TestAspectApplication extends PartialTestAspectApplication { * An aspect application added by another aspect, during execution */ interface RecordedAspectApplication extends PartialTestAspectApplication { + t: number; construct: Construct; } @@ -715,7 +741,9 @@ class NodeAddingAspect extends TracingAspect { const newConstruct = new ArbConstruct(scope, this.loc.id); for (const { aspect, priority } of this.newAspects) { Aspects.of(newConstruct).add(aspect, { priority }); - this.executionState(node).addedAspects.push({ + const executionState = this.executionState(node); + executionState.addedAspects.push({ + t: executionState.visitLog.length, construct: newConstruct, aspect, priority, @@ -744,7 +772,9 @@ 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 }); - this.executionState(node).addedAspects.push({ + const executionState = this.executionState(node); + executionState.addedAspects.push({ + t: executionState.visitLog.length, construct, aspect: this.newAspect.aspect, priority: this.newAspect.priority,