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

[kaizencc]

Closes #16242.

The asset template functions for mapping and output do not consider the logicalId of the cloudformation output, leading to possible ambiguity in the testing framework. For example, one could have these outputs:

{
  "Outputs": {
    "Out1": { "Value": "Val1" },
    "Out2": { "Value": "Val2" }
  }
}

Using template.hasOutput({ Value: "Val1" }) does not give the user control over which output they are expecting the value to be under. Potentially, the user could assert for val1 and expect it to be under Out2 when in reality it is under Out1.

This PR fixes the API by requiring a new parameter, logicalId with the possibility of defaulting to all outputs via the '*' token. The original functionality still exists by sending in '*' as the logicalId and you get more fine-grained tests by being able to assert with a specific logicalId. This functionality is added to both the output methods and the mapping methods.

BREAKING CHANGE: hasOutput(props: any) becomes hasOutput(logicalId: string, props: any)

• assertions: findOutputs(props: any = {}) becomes findOutputs(logicalId: string, props: any = {})
• assertions: hasMapping(props: any) becomes hasMapping(logicalId: string, props: any)
• assertions: findMappings(props: any = {}) becomes findMappings(logicalId: string, props: any = {})

---

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

[gitpod-io]

[nija-at]

This is a good start.

1. Update the tests that are failing to use the updated API

2. Any changes to the README.md?

3. You need a add breaking changes clause

4. PR description text gets converted into the final git commit message upon merge.
   I would suggest updating the message to spell out what the problem was or motivation and a brief on what the fix is.
   This is very useful when looking at a commit message many months from now to determine why a change was made and why it was made in a specific way.
   There are tons of articles online on writing good commit messages - check them out.

[nija-at]

We should probably also change findOutputs().

[kaizencc]

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?

[nija-at]

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.

[kaizencc]

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]

@nija-at thanks for the first review, I think I'm ready for another pass.

Re: README changes, there does not look to be much documentation for mapping and output - they are mentioned briefly under Other Sections. I'm happy to build out their README documentation more but wanted to check to make sure that's what you want.

Hopefully you like what I've done with the methods, because I know we talked about making outputName optional and I did not end up doing that. But I think that this idea is more robust in that it opens up the current functionality of hasMapping and hasOutput as well as the more fine-tuned options we now support. But I am as always happy to discuss/change the implementation.

[nija-at]

Code looks good to me. See some feedback around docs and param naming below.

Re: README changes, there does not look to be much documentation for mapping and output - they are mentioned briefly under Other Sections. I'm happy to build out their README documentation more but wanted to check to make sure that's what you want.

Would be nice to expand the Other Sections part of the README. Provide a short example or two, similar to the above sections, mention the * special case. It's fine to just expand either Output or Mapping and simply state that the other follows a similar pattern.

[kaizencc]

Hi @nija-at! Thanks for taking a look. I think this is ready for a final pass.

[nija-at]

Looks great. I've made some suggestions on the README.

Approved but I've added the 'do-not-merge' label. Remove this label once you've addressed my comments below.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[nideveloper]

Hey,

I want to make sure I understand this functionality correctly before taking other actions. I have functionality in a construct requested by users that allows them to specify the export name for an output but the actual logicalid doesn't matter. I want to be able to test for that test-export-name below without having to know websiteDeployURLC068EA32 but it seems like you have made an architectural decision that the logicalid should always be part of the tests?

  websiteDeployURLC068EA32:
    Description: The url of the website
    Value:
      Fn::GetAtt:
        - websiteDeployWebsiteBucket8C75CFAA
        - WebsiteURL
    Export:
      Name: test-export-name```