Skip to content

Commit

Permalink
chore: fix property test for aspects (#32639)
Browse files Browse the repository at this point in the history
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*
  • Loading branch information
rix0rrr authored Dec 23, 2024
1 parent 8718767 commit 0150854
Showing 1 changed file with 39 additions and 9 deletions.
48 changes: 39 additions & 9 deletions packages/aws-cdk-lib/core/test/aspect.prop.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.`);
}
Expand All @@ -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.`);
}
Expand Down Expand Up @@ -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`,
);
Expand Down Expand Up @@ -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 + '/');
}

Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -350,12 +367,20 @@ function arbConstructTreeFactory(): fc.Arbitrary<ConstructFactory> {
*/
class PrettyApp {
private readonly initialTree: Set<string>;
private readonly initialAspects: Map<string, Set<string>>;
private readonly _initialAspects: Map<string, Set<string>>;
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)));
}

/**
Expand Down Expand Up @@ -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)';
}
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 0150854

Please sign in to comment.