feat(assertions): findXxx() APIs now includes the logical id as part of its result #16454

kaizencc · 2021-09-10T15:15:47Z

The current return type of findResources(), findOutputs(), and findMappings() omits the logicalId. This creates ambiguity because it is not trivial to figure out which resource/output/mapping is being matched. For example,

const result = assert.findOutputs('*', {
  Value: 'Fred',
});

Could return a list like:

expect(result).toEqual([
  { Value: 'Fred', Description: 'FooFred' },
  { Value: 'Fred', Description: 'BarFred' },
]);

This does not provide information on which output(s) are being matched to the Value: 'Fred' property. Under the new return type proposed in this PR, the output would look like this:

expect(result).toEqual({
  Foo: { Value: 'Fred', Description: 'FooFred' },
  Bar: { Value: 'Fred', Description: 'BarFred' },
});

Returning the logicalId in this way for all find*() APIs will provide the full picture back to the user.

BREAKING CHANGE: the findResources() API previously returned a list of resources, but now returns a map of logical id to resource.

assertions: the findOutputs() API previously returned a list of outputs, but now returns a map of logical id to output.
assertions: the findMappings() API previously returned a list of mappings, but now returns a map of logical id to mapping.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2021-09-10T15:15:49Z

packages/@aws-cdk/assertions/README.md

packages/@aws-cdk/assertions/test/template.test.ts

nija-at

The breaking change entries needs to be fixed -

the findOutputs() API previously returned a list of resources, but now returns a map of logical id to resource

"the findOutputs() API previously returned a list of outputs, but now returns a map of logical id to output"

The other breaking change entry also needs to be fixed.

packages/@aws-cdk/assertions/README.md

packages/@aws-cdk/assertions/lib/private/outputs.ts

packages/@aws-cdk/assertions/test/private/section.test.ts

packages/@aws-cdk/assertions/test/template.test.ts

kaizencc · 2021-09-14T16:29:52Z

Thanks for the review @nija-at, I'm ready for another pass. Sorry about the continued copy/paste mishaps in the PR description... there should be no more. Build failure has nothing to do with me (I don't think), anything I can do to get it to build correctly?

nija-at

Just one comment about return type when no matches.

The rest look good.

nija-at · 2021-09-15T13:28:51Z

packages/@aws-cdk/assertions/lib/template.ts

@@ -101,7 +101,7 @@ export class Template {
   * When a literal is provided, performs a partial match via `Match.objectLike()`.
   * Use the `Match` APIs to configure a different behaviour.
   */
-  public findResources(type: string, props: any = {}): { [key: string]: any }[] {
+  public findResources(type: string, props: any = {}): { [key: string]: { [key: string]: any } } {


oops, I missed this in the last revision. We should be returning undefined here when there are no matches, not an empty object.

That would make the user experience better. Users can do -

const result = template.find...(...); if (result !== undefined) { // do something }

With what we have today, it'll be

const result = template.find...(...); if (Object.keys(result).length > 0) { // do something }

WDYT?

I like this in principle but it will mean that since result can be undefined, something like expect(result.Foo).toEqual({}) becomes expect(result!.Foo).toEqual({}). I could be wrong but forcing users to use the ! escape or otherwise dealing with the possibility of undefined might not make the user experience better.

WDYT, @nija-at? :)

Ok, let's go with what you have now. It might actually be the better way.

nija-at · 2021-09-15T13:30:53Z

packages/@aws-cdk/aws-apigatewayv2/test/http/api.test.ts

-    expect(Template.fromStack(stack).findResources('AWS::ApiGatewayV2::VpcLink', {
+    expect(Object.keys(Template.fromStack(stack).findResources('AWS::ApiGatewayV2::VpcLink', {
      Name: 'Link-2',
-    }).length).toEqual(0);
+    })).length).toEqual(0);


This is what I mean.

Would be nicer if we can do -

expect(Template.fromStack(stack).findResources('AWS::ApiGatewayV2::VpcLink', { Name: 'Link-2', })).toBeUndefined();

mergify · 2021-09-16T14:02:31Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-09-16T14:50:16Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: cbf3d7b
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-09-16T14:50:37Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

kaizencc added 3 commits September 9, 2021 15:39

return matches as dictionary with logicalId key

db42c11

linter

0d00bf4

make readme changes

208c842

kaizencc changed the title ~~feat(assert): find*() APIs return logicalId~~ feat(assertions): find*() APIs return logicalId Sep 10, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 10, 2021

kaizencc requested a review from nija-at September 10, 2021 15:22

kaizencc commented Sep 10, 2021

View reviewed changes

packages/@aws-cdk/assertions/README.md Show resolved Hide resolved

kaizencc commented Sep 10, 2021

View reviewed changes

packages/@aws-cdk/assertions/test/template.test.ts Outdated Show resolved Hide resolved

kaizencc added 2 commits September 10, 2021 11:56

fix cloudwatch findresource tests

9d61d44

fix apigwv2 findresource test

975c8cf

nija-at suggested changes Sep 14, 2021

View reviewed changes

kaizencc and others added 2 commits September 14, 2021 12:18

incorporate review comments

1b8df57

Merge branch 'master' into assert-find

4a78761

kaizencc changed the title ~~feat(assertions): find*() APIs return logicalId~~ feat(assertions): findXxx() APIs return map of logicalId to xxx Sep 14, 2021

kaizencc changed the title ~~feat(assertions): findXxx() APIs return map of logicalId to xxx~~ feat(assertions): findXxx() APIs return map of logicalId to xxx Sep 14, 2021

kaizencc changed the title ~~feat(assertions): findXxx() APIs return map of logicalId to xxx~~ feat(assertions): findXxx() APIs return map of logicalId to xxx Sep 14, 2021

kaizencc requested a review from nija-at September 14, 2021 16:27

Merge branch 'master' into assert-find

0f409cb

nija-at reviewed Sep 15, 2021

View reviewed changes

nija-at changed the title ~~feat(assertions): findXxx() APIs return map of logicalId to xxx~~ feat(assertions): findXxx() APIs now includes the logical id as part of its result Sep 16, 2021

nija-at approved these changes Sep 16, 2021

View reviewed changes

Merge branch 'master' into assert-find

cbf3d7b

mergify bot merged commit 532a72b into aws:master Sep 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(assertions): findXxx() APIs now includes the logical id as part of its result #16454

feat(assertions): findXxx() APIs now includes the logical id as part of its result #16454

kaizencc commented Sep 10, 2021 •

edited

Loading

gitpod-io bot commented Sep 10, 2021

nija-at left a comment

kaizencc commented Sep 14, 2021

nija-at left a comment

nija-at Sep 15, 2021

kaizencc Sep 15, 2021 •

edited

Loading

nija-at Sep 16, 2021

nija-at Sep 15, 2021

mergify bot commented Sep 16, 2021

aws-cdk-automation commented Sep 16, 2021

mergify bot commented Sep 16, 2021

feat(assertions): findXxx() APIs now includes the logical id as part of its result #16454

feat(assertions): findXxx() APIs now includes the logical id as part of its result #16454

Conversation

kaizencc commented Sep 10, 2021 • edited Loading

gitpod-io bot commented Sep 10, 2021

nija-at left a comment

Choose a reason for hiding this comment

kaizencc commented Sep 14, 2021

nija-at left a comment

Choose a reason for hiding this comment

nija-at Sep 15, 2021

Choose a reason for hiding this comment

kaizencc Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

nija-at Sep 16, 2021

Choose a reason for hiding this comment

nija-at Sep 15, 2021

Choose a reason for hiding this comment

mergify bot commented Sep 16, 2021

aws-cdk-automation commented Sep 16, 2021

AWS CodeBuild CI Report

mergify bot commented Sep 16, 2021

kaizencc commented Sep 10, 2021 •

edited

Loading

kaizencc Sep 15, 2021 •

edited

Loading