Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(assert): Adjust assertion behavior to be stricter #1289

Merged
merged 4 commits into from
Dec 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ export = nodeunit.testCase({
});
expect(pipelineStack).to(haveResource('AWS::IAM::Policy', {
PolicyDocument: {
Version: '2012-10-17',
Statement: [
{
Action: '*',
Expand Down Expand Up @@ -228,6 +229,7 @@ export = nodeunit.testCase({
expect(pipelineStack).to(countResources('AWS::IAM::Policy', 3));
expect(pipelineStack).to(haveResource('AWS::IAM::Policy', {
PolicyDocument: {
Version: '2012-10-17',
Statement: [
{
Action: [
Expand Down Expand Up @@ -324,7 +326,7 @@ function hasPipelineAction(expectedAction: any): (props: any) => boolean {
return (props: any) => {
for (const stage of props.Stages) {
for (const action of stage.Actions) {
if (isSuperObject(action, expectedAction)) {
if (isSuperObject(action, expectedAction, [], true)) {
return true;
}
}
Expand Down
63 changes: 49 additions & 14 deletions packages/@aws-cdk/assert/lib/assertions/have-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,28 @@ import { StackInspector } from "../inspector";
/**
* An assertion to check whether a resource of a given type and with the given properties exists, disregarding properties
*
* Properties can be:
*
* - An object, in which case its properties will be compared to those of the actual resource found
* - A callable, in which case it will be treated as a predicate that is applied to the Properties of the found resources.
* @param resourceType the type of the resource that is expected to be present.
* @param properties the properties that the resource is expected to have. A function may be provided, in which case
* it will be called with the properties of candidate resources and an ``InspectionFailure``
* instance on which errors should be appended, and should return a truthy value to denote a match.
* @param comparison the entity that is being asserted against.
* @param allowValueExtension if properties is an object, tells whether values must match exactly, or if they are
RomainMuller marked this conversation as resolved.
Show resolved Hide resolved
* allowed to be supersets of the reference values. Meaningless if properties is a function.
*/
export function haveResource(resourceType: string,
properties?: any,
comparison?: ResourcePart,
allowValueExtension: boolean = false): Assertion<StackInspector> {
return new HaveResourceAssertion(resourceType, properties, comparison, allowValueExtension);
}

/**
* Sugar for calling ``haveResources`` with ``allowValueExtension`` set to ``true``.
*/
export function haveResource(resourceType: string, properties?: any, comparison?: ResourcePart): Assertion<StackInspector> {
return new HaveResourceAssertion(resourceType, properties, comparison);
export function haveResourceLike(resourceType: string,
properties?: any,
comparison?: ResourcePart) {
return haveResource(resourceType, properties, comparison, true);
}

type PropertyPredicate = (props: any, inspection: InspectionFailure) => boolean;
Expand All @@ -22,10 +37,11 @@ class HaveResourceAssertion extends Assertion<StackInspector> {

constructor(private readonly resourceType: string,
private readonly properties?: any,
part?: ResourcePart) {
part?: ResourcePart,
allowValueExtension: boolean = false) {
super();

this.predicate = typeof properties === 'function' ? properties : makeSuperObjectPredicate(properties);
this.predicate = typeof properties === 'function' ? properties : makeSuperObjectPredicate(properties, allowValueExtension);
this.part = part !== undefined ? part : ResourcePart.Properties;
}

Expand Down Expand Up @@ -78,10 +94,10 @@ function indent(n: number, s: string) {
/**
* Make a predicate that checks property superset
*/
function makeSuperObjectPredicate(obj: any) {
function makeSuperObjectPredicate(obj: any, allowValueExtension: boolean) {
return (resourceProps: any, inspection: InspectionFailure) => {
const errors: string[] = [];
const ret = isSuperObject(resourceProps, obj, errors);
const ret = isSuperObject(resourceProps, obj, errors, allowValueExtension);
inspection.failureReason = errors.join(',');
return ret;
};
Expand All @@ -95,9 +111,9 @@ interface InspectionFailure {
/**
* Return whether `superObj` is a super-object of `obj`.
*
* A super-object has the same or more property values, recursing into nested objects.
* A super-object has the same or more property values, recursing into sub properties if ``allowValueExtension`` is true.
*/
export function isSuperObject(superObj: any, obj: any, errors: string[] = []): boolean {
export function isSuperObject(superObj: any, obj: any, errors: string[] = [], allowValueExtension: boolean = false): boolean {
if (obj == null) { return true; }
if (Array.isArray(superObj) !== Array.isArray(obj)) {
errors.push('Array type mismatch');
Expand All @@ -111,7 +127,7 @@ export function isSuperObject(superObj: any, obj: any, errors: string[] = []): b

// Do isSuperObject comparison for individual objects
for (let i = 0; i < obj.length; i++) {
if (!isSuperObject(superObj[i], obj[i], [])) {
if (!isSuperObject(superObj[i], obj[i], [], allowValueExtension)) {
errors.push(`Array element ${i} mismatch`);
}
}
Expand All @@ -128,7 +144,10 @@ export function isSuperObject(superObj: any, obj: any, errors: string[] = []): b
continue;
}

if (!isSuperObject(superObj[key], obj[key], [])) {
const valueMatches = allowValueExtension
? isSuperObject(superObj[key], obj[key], [], allowValueExtension)
: isStrictlyEqual(superObj[key], obj[key]);
if (!valueMatches) {
errors.push(`Field ${key} mismatch`);
}
}
Expand All @@ -139,6 +158,22 @@ export function isSuperObject(superObj: any, obj: any, errors: string[] = []): b
errors.push('Different values');
}
return errors.length === 0;

function isStrictlyEqual(left: any, right: any): boolean {
if (left === right) { return true; }
if (typeof left !== typeof right) { return false; }
if (typeof left === 'object' && typeof right === 'object') {
if (Array.isArray(left) !== Array.isArray(right)) { return false; }
const allKeys = new Set<string>([...Object.keys(left), ...Object.keys(right)]);
for (const key of allKeys) {
if (!isStrictlyEqual(left[key], right[key])) {
return false;
}
}
return true;
}
return false;
}
}

/**
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/assert/lib/assertions/match-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ class StackMatchesTemplateAssertion extends Assertion<StackInspector> {
case MatchStyle.SUPERSET:
for (const key of Object.keys(diff.resources.changes)) {
const change = diff.resources.changes[key]!;
return change.changeImpact === cfnDiff.ResourceImpact.WILL_CREATE;
if (change.changeImpact !== cfnDiff.ResourceImpact.WILL_CREATE) { return false; }
}
return true;
}
throw new Error(`Unsupported match style: ${this.matchStyle}`);
}
Expand Down
35 changes: 35 additions & 0 deletions packages/@aws-cdk/assert/test/test.assertions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,23 @@ passingExample('expect <stack> to match (no replaces) <template>', () => {
const expected = {};
expect(synthStack).to(matchTemplate(expected, MatchStyle.NO_REPLACES));
});
passingExample('expect <stack> to be a superset of <template>', () => {
const resourceType = 'Test::Resource';
const synthStack = synthesizedStack(stack => {
// Added
new TestResource(stack, 'NewResource', { type: 'AWS::S3::Bucket' });
// Expected
new TestResource(stack, 'TestResourceA', { type: resourceType });
new TestResource(stack, 'TestResourceB', { type: resourceType, properties: { Foo: 'Bar' } });
});
const expected = {
Resources: {
TestResourceA: { Type: 'Test::Resource' },
TestResourceB: { Type: 'Test::Resource', Properties: { Foo: 'Bar' } }
}
};
expect(synthStack).to(matchTemplate(expected, MatchStyle.SUPERSET));
});
passingExample('sugar for matching stack to a template', () => {
const stack = new Stack();
new TestResource(stack, 'TestResource', { type: 'Test::Resource' });
Expand Down Expand Up @@ -105,6 +122,24 @@ failingExample('expect <stack> to match (no replaces) <template>', () => {
};
expect(synthStack).to(matchTemplate(expected, MatchStyle.NO_REPLACES));
});
failingExample('expect <stack> to be a superset of <template>', () => {
const resourceType = 'Test::Resource';
const synthStack = synthesizedStack(stack => {
// Added
new TestResource(stack, 'NewResource', { type: 'AWS::S3::Bucket' });
// Expected
new TestResource(stack, 'TestResourceA', { type: resourceType });
// Expected, but has different properties - will break
new TestResource(stack, 'TestResourceB', { type: resourceType, properties: { Foo: 'Bar' } });
});
const expected = {
Resources: {
TestResourceA: { Type: 'Test::Resource' },
TestResourceB: { Type: 'Test::Resource', Properties: { Foo: 'Baz' } }
}
};
expect(synthStack).to(matchTemplate(expected, MatchStyle.SUPERSET));
});

// countResources

Expand Down
56 changes: 54 additions & 2 deletions packages/@aws-cdk/assert/test/test.have-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,59 @@ export = {
}, /PropA/);

test.done();
}
},

'haveResource value matching is strict by default'(test: Test) {
const synthStack = mkStack({
Resources: {
SomeResource: {
Type: 'Some::Resource',
Properties: {
PropA: {
foo: 'somevalue',
bar: 'This is unexpected, so the value of PropA doesn\'t strictly match - it shouldn\'t pass'
},
PropB: 'This property is unexpected, but it\'s allowed'
}
}
}
});

test.throws(() => {
expect(synthStack).to(haveResource('Some::Resource', {
PropA: {
foo: 'somevalue'
}
}));
}, /PropA/);

test.done();
},

'haveResource allows to opt in value extension'(test: Test) {
const synthStack = mkStack({
Resources: {
SomeResource: {
Type: 'Some::Resource',
Properties: {
PropA: {
foo: 'somevalue',
bar: 'Additional value is permitted, as we opted in'
},
PropB: 'Additional properties is always okay!'
}
}
}
});

expect(synthStack).to(haveResource('Some::Resource', {
PropA: {
foo: 'somevalue'
}
}, undefined, true));

test.done();
},
};

function mkStack(template: any) {
Expand All @@ -48,4 +100,4 @@ function mkStack(template: any) {
region: 'test'
}
};
}
}
30 changes: 17 additions & 13 deletions packages/@aws-cdk/assets/test/test.asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,22 +69,26 @@ export = {

expect(stack).to(haveResource('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Action: ["s3:GetObject*", "s3:GetBucket*", "s3:List*"],
Resource: [
{ "Fn::Join": ["", ["arn:", {Ref: "AWS::Partition"}, ":s3:::", {Ref: "MyAssetS3Bucket68C9B344"}]] },
{ "Fn::Join": ["",
[
"arn:", {Ref: "AWS::Partition"}, ":s3:::", {Ref: "MyAssetS3Bucket68C9B344"},
"/",
{ "Fn::Select": [0, { "Fn::Split": [ "||", { Ref: "MyAssetS3VersionKey68E1A45D" }] }] },
"*"
Version: '2012-10-17',
Statement: [
{
Action: ["s3:GetObject*", "s3:GetBucket*", "s3:List*"],
Effect: 'Allow',
Resource: [
{ "Fn::Join": ["", ["arn:", {Ref: "AWS::Partition"}, ":s3:::", {Ref: "MyAssetS3Bucket68C9B344"}]] },
{ "Fn::Join": ["",
[
"arn:", {Ref: "AWS::Partition"}, ":s3:::", {Ref: "MyAssetS3Bucket68C9B344"},
"/",
{ "Fn::Select": [0, { "Fn::Split": [ "||", { Ref: "MyAssetS3VersionKey68E1A45D" }] }] },
"*"
]
] }
]
] }
}
]
}
]}}));
}));

test.done();
},
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-apigateway/test/test.lambda.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, haveResource, not } from '@aws-cdk/assert';
import { expect, haveResource, haveResourceLike, not } from '@aws-cdk/assert';
import lambda = require('@aws-cdk/aws-lambda');
import cdk = require('@aws-cdk/cdk');
import { Test } from 'nodeunit';
Expand Down Expand Up @@ -118,7 +118,7 @@ export = {
api.root.addMethod('GET', integ);

// THEN
expect(stack).to(haveResource('AWS::ApiGateway::Method', {
expect(stack).to(haveResourceLike('AWS::ApiGateway::Method', {
Integration: {
Type: 'AWS'
}
Expand Down
Loading