Skip to content

Commit 0150854

Browse files
authored
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. <root> <-- 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*
1 parent 8718767 commit 0150854

File tree

1 file changed

+39
-9
lines changed

1 file changed

+39
-9
lines changed

packages/aws-cdk-lib/core/test/aspect.prop.test.ts

+39-9
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ describe('every aspect gets invoked exactly once', () => {
3232
for (const aspectApplication of originalAspectApplications) {
3333
// Check that each original AspectApplication also visited all child nodes of its original scope:
3434
for (const construct of originalConstructsOnApp) {
35-
if (ancestorOf(aspectApplication.construct, construct)) {
35+
if (isAncestorOf(aspectApplication.construct, construct)) {
3636
if (!visitsMap.get(construct)!.includes(aspectApplication.aspect)) {
3737
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.`);
3838
}
@@ -56,7 +56,7 @@ describe('every aspect gets invoked exactly once', () => {
5656
for (const aspectApplication of allAspectApplications) {
5757
// Check that each AspectApplication also visited all child nodes of its scope:
5858
for (const construct of allConstructsOnApp) {
59-
if (ancestorOf(aspectApplication.construct, construct)) {
59+
if (isAncestorOf(aspectApplication.construct, construct)) {
6060
if (!visitsMap.get(construct)!.includes(aspectApplication.aspect)) {
6161
throw new Error(`Aspect ${aspectApplication.aspect.constructor.name} applied on ${aspectApplication.construct.node.path} did not visit construct ${construct.node.path} in its scope.`);
6262
}
@@ -107,7 +107,11 @@ test('for every construct, lower priorities go before higher priorities', () =>
107107
const aPrio = lowestAspectPrioFor(a.aspect, a.construct);
108108
const bPrio = lowestAspectPrioFor(b.aspect, b.construct);
109109

110-
if (!implies(aPrio < bPrio, a.index < b.index)) {
110+
// But only if the application of aspect A exists at least as long as the application of aspect B
111+
const aAppliedT = aspectAppliedT(testApp, a.aspect, a.construct);
112+
const bAppliedT = aspectAppliedT(testApp, b.aspect, b.construct);
113+
114+
if (!implies(aPrio < bPrio && aAppliedT <= bAppliedT, a.index < b.index)) {
111115
throw new Error(
112116
`Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`,
113117
);
@@ -250,7 +254,7 @@ function sameAspect(a: AspectVisit, b: AspectVisit) {
250254
/**
251255
* Returns whether `a` is an ancestor of `b` (or if they are the same construct)
252256
*/
253-
function ancestorOf(a: Construct, b: Construct) {
257+
function isAncestorOf(a: Construct, b: Construct) {
254258
return b.node.path === a.node.path || b.node.path.startsWith(a.node.path + '/');
255259
}
256260

@@ -282,6 +286,19 @@ function allAspectApplicationsInScope(a: Construct): AspectApplication[] {
282286
return ancestors(a).flatMap((c) => Aspects.of(c).applied);
283287
}
284288

289+
/**
290+
* Find the lowest timestamp that could lead to the execution of the given aspect on the given construct
291+
*
292+
* Take the minimum of all added applications that could lead to this execution.
293+
*/
294+
function aspectAppliedT(prettyApp: PrettyApp, a: IAspect, c: Construct): number {
295+
const potentialTimes = [...prettyApp.initialAspects, ...prettyApp.addedAspects].filter((appl) => {
296+
return appl.aspect === a && isAncestorOf(appl.construct, c);
297+
}).map((appl) => appl.t);
298+
299+
return Math.min(...potentialTimes);
300+
}
301+
285302
/**
286303
* Return the lowest priority of Aspect `a` inside the given list of applications
287304
*/
@@ -350,12 +367,20 @@ function arbConstructTreeFactory(): fc.Arbitrary<ConstructFactory> {
350367
*/
351368
class PrettyApp {
352369
private readonly initialTree: Set<string>;
353-
private readonly initialAspects: Map<string, Set<string>>;
370+
private readonly _initialAspects: Map<string, Set<string>>;
371+
public readonly initialAspects: RecordedAspectApplication[];
354372

355373
constructor(public readonly cdkApp: App, public readonly executionState: ExecutionState) {
356374
const constructs = cdkApp.node.findAll();
357375
this.initialTree = new Set(constructs.map(c => c.node.path));
358-
this.initialAspects = new Map(constructs.map(c => [c.node.path, new Set(renderAspects(c))]));
376+
this._initialAspects = new Map(constructs.map(c => [c.node.path, new Set(renderAspects(c))]));
377+
378+
this.initialAspects = constructs.flatMap(c => Aspects.of(c).applied.map(a => ({
379+
aspect: a.aspect,
380+
construct: a.construct,
381+
priority: a.priority,
382+
t: 0,
383+
} as RecordedAspectApplication)));
359384
}
360385

361386
/**
@@ -408,7 +433,7 @@ class PrettyApp {
408433
const aspects = renderAspects(construct);
409434

410435
for (let i = 0; i < aspects.length; i++) {
411-
if (!(self.initialAspects.get(construct.node.path) ?? new Set()).has(aspects[i])) {
436+
if (!(self._initialAspects.get(construct.node.path) ?? new Set()).has(aspects[i])) {
412437
aspects[i] += ' (added)';
413438
}
414439
}
@@ -693,6 +718,7 @@ interface TestAspectApplication extends PartialTestAspectApplication {
693718
* An aspect application added by another aspect, during execution
694719
*/
695720
interface RecordedAspectApplication extends PartialTestAspectApplication {
721+
t: number;
696722
construct: Construct;
697723
}
698724

@@ -715,7 +741,9 @@ class NodeAddingAspect extends TracingAspect {
715741
const newConstruct = new ArbConstruct(scope, this.loc.id);
716742
for (const { aspect, priority } of this.newAspects) {
717743
Aspects.of(newConstruct).add(aspect, { priority });
718-
this.executionState(node).addedAspects.push({
744+
const executionState = this.executionState(node);
745+
executionState.addedAspects.push({
746+
t: executionState.visitLog.length,
719747
construct: newConstruct,
720748
aspect,
721749
priority,
@@ -744,7 +772,9 @@ class AspectAddingAspect extends TracingAspect {
744772
const constructs = this.newAspect.constructPaths.map((p) => findConstructDeep(node.node.root, p));
745773
for (const construct of constructs) {
746774
Aspects.of(construct).add(this.newAspect.aspect, { priority: this.newAspect.priority });
747-
this.executionState(node).addedAspects.push({
775+
const executionState = this.executionState(node);
776+
executionState.addedAspects.push({
777+
t: executionState.visitLog.length,
748778
construct,
749779
aspect: this.newAspect.aspect,
750780
priority: this.newAspect.priority,

0 commit comments

Comments
 (0)