Skip to content

Commit f0e2f2a

Browse files
rix0rrrrix0rrr
authored
chore: stop proptest failures by giving fewer guarantees (#32885)
The previous Aspect proptests were trying to assert ordering guarantees that the code didn't actually guarantee. This led to sporadic failures as the tests found scenarios in which the assertions were broken. In this PR, cut down on the guarantees that we make: only guarantee those things on aspect applications that exist at the start of iteration, and not on aspect applications that are added during iteration. This is not ideal, but it accurately reflects the current code and it stops intermittent failures. We will revisit tighter guarantees on aspects that are added during iteration in a later PR. Depends-On: #32884 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 97af31b commit f0e2f2a

File tree

1 file changed

+81
-61
lines changed

1 file changed

+81
-61
lines changed

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

+81-61
Original file line numberDiff line numberDiff line change
@@ -77,80 +77,100 @@ describe('every aspect gets invoked exactly once', () => {
7777
);
7878
});
7979

80-
test('inherited aspects get invoked before locally defined aspects, if both have the same priority', () =>
81-
fc.assert(
82-
fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => {
83-
afterSynth((testApp) => {
84-
forEveryVisitPair(testApp.actionLog, (a, b) => {
85-
if (!sameConstruct(a, b)) return;
80+
/**
81+
* This test suite is only checking guarantees for aspects that exist from the start of iterating.
82+
*
83+
* These rules are the same for both old and new invocation patterns.
84+
*
85+
* Aspects that get added during iteration have harder to specify rules, and I'm ripping my hair out
86+
* trying to come up with good specifications of them. Let's first scale down to these so that at least
87+
* these tests are stable.
88+
*/
89+
describe('ordering when all aspects exist from the start', () => {
90+
test('inherited aspects get invoked before locally defined aspects, if both have the same priority', () =>
91+
fc.assert(
92+
fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => {
93+
afterSynth((testApp) => {
94+
forEveryVisitPair(testApp.actionLog, (a, b) => {
95+
if (!sameConstruct(a, b)) return;
96+
if (aspectAppliedT(testApp, a.aspect, a.construct) !== -1 ||
97+
aspectAppliedT(testApp, b.aspect, b.construct) !== -1) return;
8698

87-
const aPrio = lowestAspectPrioFor(a.aspect, a.construct);
88-
const bPrio = lowestAspectPrioFor(b.aspect, b.construct);
99+
const aPrio = lowestAspectPrioFor(a.aspect, a.construct);
100+
const bPrio = lowestAspectPrioFor(b.aspect, b.construct);
89101

90-
if (!(aPrio == bPrio)) return;
102+
if (!(aPrio == bPrio)) return;
91103

92-
const aInherited = allAncestorAspects(a.construct).includes(a.aspect);
93-
const bInherited = allAncestorAspects(b.construct).includes(b.aspect);
104+
const aInherited = allAncestorAspects(a.construct).includes(a.aspect);
105+
const bInherited = allAncestorAspects(b.construct).includes(b.aspect);
94106

95-
if (!(aInherited == true && bInherited == false)) return;
107+
if (!(aInherited == true && bInherited == false)) return;
96108

97-
if (!(a.index < b.index)) {
98-
throw new Error(
99-
`Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`,
100-
);
101-
}
102-
});
103-
}, stabilizeAspects)(app);
104-
}),
105-
),
106-
);
109+
if (!(a.index < b.index)) {
110+
throw new Error(
111+
`Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`,
112+
);
113+
}
114+
});
115+
}, stabilizeAspects)(app);
116+
}),
117+
),
118+
);
107119

108-
test('for every construct, lower priorities go before higher priorities', () =>
109-
fc.assert(
110-
fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => {
111-
afterSynth((testApp) => {
112-
forEveryVisitPair(testApp.actionLog, (a, b) => {
113-
if (!sameConstruct(a, b)) return;
120+
test('for every construct, lower priorities go before higher priorities', () =>
121+
fc.assert(
122+
fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => {
123+
afterSynth((testApp) => {
124+
forEveryVisitPair(testApp.actionLog, (a, b) => {
125+
if (!sameConstruct(a, b)) return;
126+
if (aspectAppliedT(testApp, a.aspect, a.construct) !== -1 ||
127+
aspectAppliedT(testApp, b.aspect, b.construct) !== -1) return;
114128

115-
const aPrio = lowestAspectPrioFor(a.aspect, a.construct);
116-
const bPrio = lowestAspectPrioFor(b.aspect, b.construct);
129+
const aPrio = lowestAspectPrioFor(a.aspect, a.construct);
130+
const bPrio = lowestAspectPrioFor(b.aspect, b.construct);
117131

118-
// But only if the application of aspect A exists at least as long as the application of aspect B
119-
const aAppliedT = aspectAppliedT(testApp, a.aspect, a.construct);
120-
const bAppliedT = aspectAppliedT(testApp, b.aspect, b.construct);
132+
// But only if the application of aspect A exists at least as long as the application of aspect B
133+
const aAppliedT = aspectAppliedT(testApp, a.aspect, a.construct);
134+
const bAppliedT = aspectAppliedT(testApp, b.aspect, b.construct);
121135

122-
if (!implies(aPrio < bPrio && aAppliedT <= bAppliedT, a.index < b.index)) {
123-
throw new Error(
124-
`Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`,
125-
);
126-
}
127-
});
128-
}, stabilizeAspects)(app);
129-
}),
130-
),
131-
);
136+
if (!implies(aPrio < bPrio && aAppliedT <= bAppliedT, a.index < b.index)) {
137+
throw new Error(
138+
`Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`,
139+
);
140+
}
141+
});
142+
}, stabilizeAspects)(app);
143+
}),
144+
),
145+
);
132146

133-
test('for every construct, if a invokes before b that must mean it is of equal or lower priority', () =>
134-
fc.assert(
135-
fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => {
136-
afterSynth((testApp) => {
137-
forEveryVisitPair(testApp.actionLog, (a, b) => {
138-
if (!sameConstruct(a, b)) return;
147+
test('for every construct, if a invokes before b that must mean it is of equal or lower priority', () =>
148+
fc.assert(
149+
fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => {
150+
afterSynth((testApp) => {
151+
forEveryVisitPair(testApp.actionLog, (a, b) => {
152+
if (!sameConstruct(a, b)) return;
153+
if (aspectAppliedT(testApp, a.aspect, a.construct) !== -1 ||
154+
aspectAppliedT(testApp, b.aspect, b.construct) !== -1) return;
139155

140-
const aPrio = lowestAspectPrioFor(a.aspect, a.construct);
141-
const bPrio = lowestAspectPrioFor(b.aspect, b.construct);
156+
const aPrio = lowestAspectPrioFor(a.aspect, a.construct);
157+
const bPrio = lowestAspectPrioFor(b.aspect, b.construct);
142158

143-
if (!implies(a.index < b.index, aPrio <= bPrio)) {
144-
throw new Error(
145-
`Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`,
146-
);
147-
}
148-
});
149-
}, stabilizeAspects)(app);
150-
}),
151-
),
152-
);
159+
if (!implies(a.index < b.index, aPrio <= bPrio)) {
160+
throw new Error(
161+
`Aspect ${a.aspect}@${aPrio} at ${a.index} should have been before ${b.aspect}@${bPrio} at ${b.index}, but was after`,
162+
);
163+
}
164+
});
165+
}, stabilizeAspects)(app);
166+
}),
167+
),
168+
);
169+
});
153170

171+
/**
172+
* Sanity check to make sure the log actually records things
173+
*/
154174
test('visitLog is nonempty', () =>
155175
fc.assert(
156176
fc.property(appWithAspects(), fc.boolean(), (app, stabilizeAspects) => {

0 commit comments

Comments
 (0)