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

feat(assertions): major improvements to the capture feature #17713

Merged
merged 6 commits into from Dec 2, 2021

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Nov 25, 2021

There are three major changes around the capture feature of assertions.

Firstly, when there are multiple targets (say, Resource in the
CloudFormation template) that matches the given condition, any
Capture defined in the condition will contain only the last matched
resource.
Convert the Capture class into an iterable so all matching values can
be retrieved.

Secondly, add support to allow sub-patterns to be specified to the
Capture class. This allows further conditions be specified, via
Matchers or literals, when a value is to be captured.

Finally, this fixes a bug with the current implementation where
Capture contains the results of the last matched section, irrespective
of whether that section matched with the rest of the matcher or not.

fixes #17009


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

When there are multiple targets (say, Resource in the CloudFormation
template) that matches the given condition, any `Capture` defined in the
condition will contain only the last matched resource.

Convert the `Capture` class into an iterable so all matching values can
be retrieved.
@nija-at nija-at requested review from kaizencc and a team November 25, 2021 15:43
@nija-at nija-at self-assigned this Nov 25, 2021
@github-actions github-actions bot added the @aws-cdk/assertions Related to the @aws-cdk/assertv2 package label Nov 25, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 25, 2021
@gitpod-io
Copy link

gitpod-io bot commented Nov 30, 2021

@nija-at nija-at requested review from rix0rrr and a team November 30, 2021 15:09
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Looks sensible. I wonder if we could do some research to figure out how other pattern do it, if there's a clever coding pattern we might have missed.

But otherwise, yeah this seems to get the job done.

return this;
}

/**
* Prepare the result to be analyzed.
* Analyzing results before the finalize() API is called may be inaccurate in some cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

That is a weak statement. Should be made stronger (it's not allowed to analyze results before calling this?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to get hasFailed() etc on this having been called first?

So that it cannot be misused?

packages/@aws-cdk/assertions/lib/matcher.ts Outdated Show resolved Hide resolved
@nija-at nija-at changed the title feat(assertions): capture from multiple matching targets feat(assertions): major improvements to the capture feature Dec 2, 2021
@nija-at nija-at removed their assignment Dec 2, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 2, 2021

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 9a67ce7 into master Dec 2, 2021
@mergify mergify bot deleted the nija-at/capture-fix branch December 2, 2021 14:17
@mergify
Copy link
Contributor

mergify bot commented Dec 2, 2021

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).

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
There are three major changes around the capture feature of assertions.

Firstly, when there are multiple targets (say, Resource in the
CloudFormation template) that matches the given condition, any
`Capture` defined in the condition will contain only the last matched
resource.
Convert the `Capture` class into an iterable so all matching values can
be retrieved.

Secondly, add support to allow sub-patterns to be specified to the
`Capture` class. This allows further conditions be specified, via
Matchers or literals, when a value is to be captured.

Finally, this fixes a bug with the current implementation where
`Capture` contains the results of the last matched section, irrespective
of whether that section matched with the rest of the matcher or not.

fixes aws#17009

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/assertions Related to the @aws-cdk/assertv2 package contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(assertions): Capture captures value from last resource with matching type, not matched props
4 participants