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.

Sign up for GitHub

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

Jump to bottom

fix(assertions): output and mapping assertions do not accept logical id #16329

Merged
merged 13 commits into from
Sep 8, 2021
Merged

fix(assertions): output and mapping assertions do not accept logical id #16329

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
46ea434
initial implementation of adding outputname to hasoutput
kaizencc Sep 1, 2021
0dc220f
outputName for has/find outputs and mapping
kaizencc Sep 7, 2021
a86aea8
linter
kaizencc Sep 7, 2021
8f9b74e
fix neptune tests
kaizencc Sep 7, 2021
5969171
fix firehose tests
kaizencc Sep 7, 2021
b3cd361
Merge branch 'master' into assertions-hasoutput
kaizencc Sep 7, 2021
9b3d40e
change param name from outputName to logicalId
kaizencc Sep 8, 2021
36d1f10
update readme
kaizencc Sep 8, 2021
a209bd3
better structure
kaizencc Sep 8, 2021
c8cc425
Merge branch 'master' into assertions-hasoutput
kaizencc Sep 8, 2021
5c670cd
readme CR suggestions
kaizencc Sep 8, 2021
9f3c090
Merge branch 'assertions-hasoutput' of https://github.com/kaizen30315…
kaizencc Sep 8, 2021
e3f8018
Merge branch 'master' into assertions-hasoutput
kaizencc Sep 8, 2021
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
10 changes: 5 additions & 5 deletions packages/@aws-cdk/assertions/lib/private/mappings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { StackInspector } from '../vendored/assert';
import { formatFailure, matchSection } from './section';
import { filterLogicalId, formatFailure, matchSection } from './section';

export function findMappings(inspector: StackInspector, props: any = {}): { [key: string]: any }[] {
export function findMappings(inspector: StackInspector, outputName: string, props: any = {}): { [key: string]: any }[] {
const section: { [key: string] : {} } = inspector.value.Mappings;
const result = matchSection(section, props);
const result = matchSection(filterLogicalId(section, outputName), props);

if (!result.match) {
return [];
Expand All @@ -12,9 +12,9 @@ export function findMappings(inspector: StackInspector, props: any = {}): { [key
return result.matches;
}

export function hasMapping(inspector: StackInspector, props: any): string | void {
export function hasMapping(inspector: StackInspector, outputName: string, props: any): string | void {
const section: { [key: string]: {} } = inspector.value.Mappings;
const result = matchSection(section, props);
const result = matchSection(filterLogicalId(section, outputName), props);

if (result.match) {
return;
Expand Down
17 changes: 8 additions & 9 deletions packages/@aws-cdk/assertions/lib/private/outputs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { StackInspector } from '../vendored/assert';
import { formatFailure, matchSection } from './section';
import { filterLogicalId, formatFailure, matchSection } from './section';

export function findOutputs(inspector: StackInspector, props: any = {}): { [key: string]: any }[] {
export function findOutputs(inspector: StackInspector, outputName: string, props: any = {}): { [key: string]: any }[] {
const section: { [key: string] : {} } = inspector.value.Outputs;
const result = matchSection(section, props);
const result = matchSection(filterLogicalId(section, outputName), props);

if (!result.match) {
return [];
Expand All @@ -12,20 +12,19 @@ export function findOutputs(inspector: StackInspector, props: any = {}): { [key:
return result.matches;
}

export function hasOutput(inspector: StackInspector, props: any): string | void {
export function hasOutput(inspector: StackInspector, outputName: string, props: any): string | void {
const section: { [key: string]: {} } = inspector.value.Outputs;
const result = matchSection(section, props);

const result = matchSection(filterLogicalId(section, outputName), props);
if (result.match) {
return;
}

if (result.closestResult === undefined) {
return 'No outputs found in the template';
return `No outputs named ${outputName} found in the template.`;
}

return [
`Template has ${result.analyzedCount} outputs, but none match as expected.`,
`Template has ${result.analyzedCount} outputs named ${outputName}, but none match as expected.`,
formatFailure(result.closestResult),
].join('\n');
}
}
9 changes: 9 additions & 0 deletions packages/@aws-cdk/assertions/lib/private/section.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,13 @@ export function formatFailure(closestResult: MatchResult): string {
function leftPad(x: string, indent: number = 2): string {
const pad = ' '.repeat(indent);
return pad + x.split('\n').join(`\n${pad}`);
}

export function filterLogicalId(section: { [key: string]: {} }, outputName: string): { [key: string]: {} } {
kaizencc marked this conversation as resolved.
Show resolved Hide resolved
// default signal for all outputs is '*'
if (outputName === '*') return section;

return Object.entries(section ?? {})
.filter(([k, _]) => k === outputName)
.reduce((agg, [k, v]) => { return { ...agg, [k]: v }; }, {});
}
20 changes: 12 additions & 8 deletions packages/@aws-cdk/assertions/lib/template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,46 +109,50 @@ export class Template {
* Assert that an Output with the given properties exists in the CloudFormation template.
* By default, performs partial matching on the resource, via the `Match.objectLike()`.
* To configure different behavour, use other matchers in the `Match` class.
* @param outputName the name of the output. Provide '*' to match all Output names in the template.
* @param props the output as should be expected in the template.
*/
public hasOutput(props: any): void {
const matchError = hasOutput(this.inspector, props);
public hasOutput(outputName: string, props: any): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably also change findOutputs().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like for findOutputs, we should be less stringent about the outputName and still allow for users to search for some value among all outputs like so:

test('matching', () => {
      const stack = new Stack();
      new CfnOutput(stack, 'Foo', {
        value: 'Fred',
        description: 'FooFred',
      });
      new CfnOutput(stack, 'Bar', {
        value: 'Fred',
        description: 'BarFred',
      });
      new CfnOutput(stack, 'Baz', {
        value: 'Waldo',
        description: 'BazWaldo',
      });

      const inspect = Template.fromStack(stack);
      const result = inspect.findOutputs({ Value: 'Fred' });
      expect(result).toEqual([
        { Value: 'Fred', Description: 'FooFred' },
        { Value: 'Fred', Description: 'BarFred' },
      ]);
    });

With that in mind, do you think we should:

a) still require an outputName in findOutputs (what I read that you are suggesting in the above comment)
b) make outputName optional
c) figure out some way to return the outputName so the user has the full picture.

If it were up to me I think that c) is the best option though it would require us to make a change to matchSection() so we return the logicalId along with the rest of the output. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your suggestion of making outputName optional and returning the logicalId. How about we split this into two changes? In this PR, implement (b) and in a subsequent PR, implement (c) but for all find*() APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like making outputName optional is problematic in its own right. We get public findOutputs(outputName?: string, props?: string) which doesn't really work because of the two optional parameters. The user experience for not wanting to specify an outputName would be findOutputs(undefined, MyProps). I feel like that is unacceptable, but I'm open to input.

Otherwise, the three options I can think of are overloading the method (but typescript barely supports this and I have no reference that this happens anywhere in the CDK) or refactoring to a method signature of findOutputs(options: findOptions). I am also sympathetic towards having all the find and has methods similar in signature, so perhaps it is time to start thinking about all of these methods taking in a findXXXOptions or hasXXXOptions instead.

My last option is the one I like best and the one I am currently implementing. I think we should require outputName and clearly document that a particular token ('*') represents all outputs. Then, the user experience for not wanting to specify an outputName would be findOutputs('*', MyProps) which I think makes more sense than undefined.

kaizencc marked this conversation as resolved.
Show resolved Hide resolved
const matchError = hasOutput(this.inspector, outputName, props);
if (matchError) {
throw new Error(matchError);
}
}

/**
* Get the set of matching Outputs that match the given properties in the CloudFormation template.
* @param outputName the name of the output. Provide '*' to match all Output names in the template.
* @param props by default, matches all Outputs in the template.
* When a literal object is provided, performs a partial match via `Match.objectLike()`.
* Use the `Match` APIs to configure a different behaviour.
*/
public findOutputs(props: any = {}): { [key: string]: any }[] {
return findOutputs(this.inspector, props);
public findOutputs(outputName: string, props: any = {}): { [key: string]: any }[] {
return findOutputs(this.inspector, outputName, props);
}

/**
* Assert that a Mapping with the given properties exists in the CloudFormation template.
* By default, performs partial matching on the resource, via the `Match.objectLike()`.
* To configure different behavour, use other matchers in the `Match` class.
* @param outputName the name of the output. Provide '*' to match all Output names in the template.
kaizencc marked this conversation as resolved.
Show resolved Hide resolved
* @param props the output as should be expected in the template.
*/
public hasMapping(props: any): void {
const matchError = hasMapping(this.inspector, props);
public hasMapping(outputName: string, props: any): void {
kaizencc marked this conversation as resolved.
Show resolved Hide resolved
const matchError = hasMapping(this.inspector, outputName, props);
if (matchError) {
throw new Error(matchError);
}
}

/**
* Get the set of matching Mappings that match the given properties in the CloudFormation template.
* @param outputName the name of the output. Provide '*' to match all Output names in the template.
* @param props by default, matches all Mappings in the template.
* When a literal object is provided, performs a partial match via `Match.objectLike()`.
* Use the `Match` APIs to configure a different behaviour.
*/
public findMappings(props: any = {}): { [key: string]: any }[] {
return findMappings(this.inspector, props);
public findMappings(outputName: string, props: any = {}): { [key: string]: any }[] {
return findMappings(this.inspector, outputName, props);
}

/**
Expand Down
181 changes: 172 additions & 9 deletions packages/@aws-cdk/assertions/test/template.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ describe('Template', () => {
});

const inspect = Template.fromStack(stack);
expect(() => inspect.hasOutput({ Value: 'Bar' })).not.toThrow();
expect(() => inspect.hasOutput('Foo', { Value: 'Bar' })).not.toThrow();
});

test('not matching', (done) => {
Expand All @@ -357,18 +357,62 @@ describe('Template', () => {

const inspect = Template.fromStack(stack);
expectToThrow(
() => inspect.hasOutput({
() => inspect.hasOutput('Foo', {
Value: 'Bar',
Export: { Name: 'ExportBaz' },
}),
[
/2 outputs/,
/1 outputs named Foo/,
/Expected ExportBaz but received ExportBar/,
],
done,
);
done();
});

test('value not matching with outputName', (done) => {
const stack = new Stack();
new CfnOutput(stack, 'Foo', {
value: 'Bar',
});
new CfnOutput(stack, 'Fred', {
value: 'Baz',
});

const inspect = Template.fromStack(stack);
expectToThrow(
() => inspect.hasOutput('Fred', {
Value: 'Bar',
}),
[
/1 outputs named Fred/,
/Expected Bar but received Baz/,
],
done,
);
done();
});
});

test('outputName not matching', (done) => {
const stack = new Stack();
new CfnOutput(stack, 'Foo', {
value: 'Bar',
exportName: 'ExportBar',
});

const inspect = Template.fromStack(stack);
expectToThrow(
() => inspect.hasOutput('Fred', {
Value: 'Bar',
Export: { Name: 'ExportBar' },
}),
[
/No outputs named Fred found in the template./,
],
done,
);
done();
});

describe('findOutputs', () => {
Expand All @@ -388,7 +432,7 @@ describe('Template', () => {
});

const inspect = Template.fromStack(stack);
const result = inspect.findOutputs({ Value: 'Fred' });
const result = inspect.findOutputs('*', { Value: 'Fred' });
expect(result).toEqual([
{ Value: 'Fred', Description: 'FooFred' },
{ Value: 'Fred', Description: 'BarFred' },
Expand All @@ -402,7 +446,37 @@ describe('Template', () => {
});

const inspect = Template.fromStack(stack);
const result = inspect.findOutputs({ Value: 'Waldo' });
const result = inspect.findOutputs('*', { Value: 'Waldo' });
expect(result.length).toEqual(0);
});

test('matching specific output', () => {
const stack = new Stack();
new CfnOutput(stack, 'Foo', {
value: 'Fred',
});
new CfnOutput(stack, 'Baz', {
value: 'Waldo',
});

const inspect = Template.fromStack(stack);
const result = inspect.findOutputs('Foo', { Value: 'Fred' });
expect(result).toEqual([
{ Value: 'Fred' },
]);
});

test('not matching specific output', () => {
const stack = new Stack();
new CfnOutput(stack, 'Foo', {
value: 'Fred',
});
new CfnOutput(stack, 'Baz', {
value: 'Waldo',
});

const inspect = Template.fromStack(stack);
const result = inspect.findOutputs('Foo', { Value: 'Waldo' });
expect(result.length).toEqual(0);
});
});
Expand All @@ -423,7 +497,7 @@ describe('Template', () => {
});

const inspect = Template.fromStack(stack);
expect(() => inspect.hasMapping({ Foo: { Bar: 'Lightning' } })).not.toThrow();
expect(() => inspect.hasMapping('*', { Foo: { Bar: 'Lightning' } })).not.toThrow();
});

test('not matching', (done) => {
Expand All @@ -442,7 +516,7 @@ describe('Template', () => {

const inspect = Template.fromStack(stack);
expectToThrow(
() => inspect.hasMapping({
() => inspect.hasMapping('*', {
Foo: { Bar: 'Qux' },
}),
[
Expand All @@ -453,6 +527,52 @@ describe('Template', () => {
);
done();
});

test('matching specific outputName', () => {
const stack = new Stack();
new CfnMapping(stack, 'Foo', {
mapping: {
Foo: { Bar: 'Lightning', Fred: 'Waldo' },
Baz: { Bar: 'Qux' },
},
});
new CfnMapping(stack, 'Fred', {
mapping: {
Foo: { Bar: 'Lightning' },
},
});

const inspect = Template.fromStack(stack);
expect(() => inspect.hasMapping('Foo', { Baz: { Bar: 'Qux' } })).not.toThrow();
});

test('not matching specific outputName', (done) => {
const stack = new Stack();
new CfnMapping(stack, 'Foo', {
mapping: {
Foo: { Bar: 'Fred', Baz: 'Waldo' },
Qux: { Bar: 'Fred' },
},
});
new CfnMapping(stack, 'Fred', {
mapping: {
Foo: { Baz: 'Baz' },
},
});

const inspect = Template.fromStack(stack);
expectToThrow(
() => inspect.hasMapping('Fred', {
Foo: { Baz: 'Fred' },
}),
[
/1 mappings/,
/Expected Fred but received Baz/,
],
done,
);
done();
});
});

describe('findMappings', () => {
Expand All @@ -471,7 +591,7 @@ describe('Template', () => {
});

const inspect = Template.fromStack(stack);
const result = inspect.findMappings({ Foo: { Bar: 'Lightning' } });
const result = inspect.findMappings('*', { Foo: { Bar: 'Lightning' } });
expect(result).toEqual([
{
Foo: { Bar: 'Lightning', Fred: 'Waldo' },
Expand All @@ -490,7 +610,50 @@ describe('Template', () => {
});

const inspect = Template.fromStack(stack);
const result = inspect.findMappings({ Foo: { Bar: 'Waldo' } });
const result = inspect.findMappings('*', { Foo: { Bar: 'Waldo' } });
expect(result.length).toEqual(0);
});

test('matching with specific outputName', () => {
const stack = new Stack();
new CfnMapping(stack, 'Foo', {
mapping: {
Foo: { Bar: 'Lightning', Fred: 'Waldo' },
Baz: { Bar: 'Qux' },
},
});
new CfnMapping(stack, 'Fred', {
mapping: {
Foo: { Bar: 'Lightning' },
},
});

const inspect = Template.fromStack(stack);
const result = inspect.findMappings('Foo', { Foo: { Bar: 'Lightning' } });
expect(result).toEqual([
{
Foo: { Bar: 'Lightning', Fred: 'Waldo' },
Baz: { Bar: 'Qux' },
},
]);
});

test('not matching', () => {
const stack = new Stack();
new CfnMapping(stack, 'Foo', {
mapping: {
Foo: { Bar: 'Lightning', Fred: 'Waldo' },
Baz: { Bar: 'Qux' },
},
});
new CfnMapping(stack, 'Fred', {
mapping: {
Foo: { Bar: 'Lightning' },
},
});

const inspect = Template.fromStack(stack);
const result = inspect.findMappings('Fred', { Baz: { Bar: 'Qux' } });
expect(result.length).toEqual(0);
});
});
Expand Down
Loading