feat(assertions): allResources and allResourcesProperties methods #22007

dontirun · 2022-09-12T18:32:38Z

Closes #21269

All Submissions:

[X ] Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-09-12T18:32:45Z

dontirun · 2022-09-12T19:46:03Z

I don't think that I need to write an integration test for this PR, but please let me know if I do

Naumel

Hello,

Thank you for your engagement and submitting a review.

I would like to prod a bit more in the reasoning that is behind your thinking that integ tests should not be part of this review: mind adding the insight?

Naumel · 2022-09-13T08:42:05Z

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

+  if (result.match) {
+    if (result.mismatches) {


How can a result both match and mismatch (naming might be decreasing the ease of reading)?

Why are there no tests for the new functionality?

I took another look at this and decided to take mismatches out of the MatchSuccess. I agree it's confusing and after testing the functionality I realized I wasn't getting the information that I wanted in the error output.

I reworked the functionality for easier readability and better messaging

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

Naumel · 2022-09-13T08:47:57Z

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

+
+```ts
+template.allResources('Foo::Bar', {
+  Properties: { Foo: 'Bar' },


Same remark about disambiguating stands for this example.

Naumel · 2022-09-13T08:49:13Z

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

        if (closestResult === undefined || closestResult.failCount > result.failCount) {
          closestResult = result;
        }
      }
    },
  );
  if (Object.keys(matching).length > 0) {
-    return { match: true, matches: matching };
+    return { match: true, matches: matching, analyzedCount: count, mismatches: closestResult };
  } else {
    return { match: false, closestResult, analyzedCount: count };


Please add testing for the change around count.

dontirun · 2022-09-13T12:17:24Z

Hello,

Thank you for your engagement and submitting a review.

I would like to prod a bit more in the reasoning that is behind your thinking that integ tests should not be part of this review: mind adding the insight?

Unit tests are important here since we're changing functionality.

I don't think Integration tests make sense since we aren't changing anything related to CloudFormation resources/asset deployment.

Pull request has been modified.

dontirun · 2022-09-13T14:28:18Z

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

+export type MatchSuccess = { match: true, matches: { [key: string]: any }, analyzed: { [key: string]: any }, analyzedCount: number };
+export type MatchFailure = { match: false, closestResult?: MatchResult, analyzed: { [key: string]: any }, analyzedCount: number };


I know we can technically remove analyzedCount here, but I kept it since it's being used in other places and convenient to use.

If we want I can remove this field and update the other places to use Object.keys(analyzed).length

Naumel · 2022-09-16T17:18:01Z

@Mergifyio update

mergify · 2022-09-16T17:18:41Z

update

✅ Branch has been successfully updated

github-actions

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

dontirun · 2022-10-06T14:31:13Z

@Naumel Is this good to go, or are there other changes needed?

joel-aws · 2022-10-19T17:07:47Z

As the submitter of issue #21269 (that this PR would close), I am really looking forward to seeing this merged!

@Naumel , are there any updates on the review?

Thanks!

Pull request has been modified.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

mergify · 2022-11-01T15:20:10Z

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

kaizencc · 2022-11-01T15:23:22Z

@Mergifyio refresh

mergify · 2022-11-01T15:23:57Z

refresh

✅ Pull request refreshed

Pull request has been modified.

aws-cdk-automation · 2022-11-01T17:19:01Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 6acd949
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-11-02T08:08:49Z

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

feat(assertions): allResources and allResourcesProperties methods

afc9f70

aws-cdk-automation requested a review from a team September 12, 2022 18:32

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Sep 12, 2022

Naumel previously requested changes Sep 13, 2022

View reviewed changes

Naumel reviewed Sep 13, 2022

View reviewed changes

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

Naumel reviewed Sep 13, 2022

View reviewed changes

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

Naumel reviewed Sep 13, 2022

View reviewed changes

dontirun added 3 commits September 13, 2022 08:54

docs: disambiguating examples

c7763a7

test: altering adding tests for analyzed count

b0b0f4d

refactor: removing mismatches field and changing error messages

e063824

dontirun added 2 commits September 13, 2022 10:24

refactor: formatAllMismatches

ed576a7

test: update tests

b35b524

dontirun commented Sep 13, 2022

View reviewed changes

dontirun requested a review from Naumel September 16, 2022 12:15

mergify bot and others added 2 commits September 16, 2022 17:18

Merge branch 'main' into feat-match-all-resources

b8fb019

Merge branch 'main' into feat-match-all-resources

ec54451

github-actions bot previously requested changes Sep 26, 2022

View reviewed changes

Merge branch 'main' into feat-match-all-resources

d9ccb2a

aws-cdk-automation previously requested changes Oct 19, 2022

View reviewed changes

TheRealAmazonKendra added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 1, 2022

TheRealAmazonKendra previously approved these changes Nov 1, 2022

View reviewed changes

Merge branch 'main' into feat-match-all-resources

cd9710a

Merge branch 'main' into feat-match-all-resources

6acd949

Naumel approved these changes Nov 2, 2022

View reviewed changes

mergify bot merged commit 2430537 into aws:main Nov 2, 2022

dontirun deleted the feat-match-all-resources branch November 2, 2022 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(assertions): allResources and allResourcesProperties methods #22007

feat(assertions): allResources and allResourcesProperties methods #22007

dontirun commented Sep 12, 2022

gitpod-io bot commented Sep 12, 2022

dontirun commented Sep 12, 2022

Naumel left a comment

Naumel Sep 13, 2022

dontirun Sep 13, 2022

Naumel Sep 13, 2022

Naumel Sep 13, 2022

dontirun commented Sep 13, 2022

dontirun Sep 13, 2022

Naumel commented Sep 16, 2022

mergify bot commented Sep 16, 2022

github-actions bot left a comment

dontirun commented Oct 6, 2022

joel-aws commented Oct 19, 2022

aws-cdk-automation left a comment

mergify bot commented Nov 1, 2022

kaizencc commented Nov 1, 2022

mergify bot commented Nov 1, 2022

aws-cdk-automation commented Nov 1, 2022

mergify bot commented Nov 2, 2022

		export type MatchSuccess = { match: true, matches: { [key: string]: any }, analyzed: { [key: string]: any }, analyzedCount: number };
		export type MatchFailure = { match: false, closestResult?: MatchResult, analyzed: { [key: string]: any }, analyzedCount: number };

feat(assertions): allResources and allResourcesProperties methods #22007

feat(assertions): allResources and allResourcesProperties methods #22007

Conversation

dontirun commented Sep 12, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Sep 12, 2022

dontirun commented Sep 12, 2022

Naumel left a comment

Choose a reason for hiding this comment

Naumel Sep 13, 2022

Choose a reason for hiding this comment

dontirun Sep 13, 2022

Choose a reason for hiding this comment

Naumel Sep 13, 2022

Choose a reason for hiding this comment

Naumel Sep 13, 2022

Choose a reason for hiding this comment

dontirun commented Sep 13, 2022

dontirun Sep 13, 2022

Choose a reason for hiding this comment

Naumel commented Sep 16, 2022

mergify bot commented Sep 16, 2022

✅ Branch has been successfully updated

github-actions bot left a comment

Choose a reason for hiding this comment

dontirun commented Oct 6, 2022

joel-aws commented Oct 19, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 1, 2022

kaizencc commented Nov 1, 2022

mergify bot commented Nov 1, 2022

✅ Pull request refreshed

aws-cdk-automation commented Nov 1, 2022

AWS CodeBuild CI Report

mergify bot commented Nov 2, 2022