fix: this.node.addError does not cause cdk diff to fail #4700

[nideveloper]

Adding in appStacks.processMetadata(stacks); command into the diff method so that metadata is processed correctly and the diff should fail

This is for issue #4700

I am unsure how to go about effectively testing this change so I wanted to open the pull request early and start some conversation.

---

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

[mergify]

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

• PR title type(scope): text
  • type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • text: use all lower-case, do not end with a period, do not include issue refs
• PR Description
  • Rationale: describe rationale of change and approach taken
  • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
  • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
• Testing
  • Unit test added. Prefer to add a new test rather than modify existing tests
  • CLI or init templates change? Re-run/add CLI integration tests
• Documentation
  • README: update module README to describe new features
  • API docs: public APIs must be documented. Copy from official AWS docs when possible
  • Design: for significant features, follow design process

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[shivlaks]

@nideveloper thanks for taking a run at this issue!

When you say you're not sure testing the change, did you mean locally or writing unit/integ tests?

The PR checklist should point you towards running/adding integ tests, but I'm wondering if the CONTRIBUTING guide itself needs to be beefed up?

[nideveloper]

@shivlaks sorry I should have been way more specific. So when I looked into the existing tests for the diff functionality it doesn't actually test the inner workings of diff. A flag is passed in to say pass or fail, this overrides everything that diff actually does internally. So I don't really have an example of a test that uses diff to fail for two mock stacks without setting this flag which for my purposes of this change wouldn't be a very good test as the change is to fail diff if error flags are in the stack.

test
https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/test/test.diff.ts#L78

diff use of fail flag
https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/lib/cdk-toolkit.ts#L75

I just wanted to make sure before I go down a certain path with testing this that I was going the right way or that I am not missing something obvious

[nideveloper]

I should add I got all the existing tests running and passing locally etc, I didn't have any issues with that so I think the documentation is probably ok.

[nideveloper]

I worked it out, going through the commit process now.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[shivlaks]

I worked it out, going through the commit process now.

that's great, looking good. i'm wondering if we can also add an integ test for verifying this scenario

[nideveloper]

I worked it out, going through the commit process now.

that's great, looking good. i'm wondering if we can also add an integ test for verifying this scenario

Correct me if i'm wrong but since this ticket is about testing a scenario where you do a diff on a stack with an error I would need to modify this file to create a test stack that has an error?

https://github.com/nideveloper/aws-cdk/blob/master/packages/aws-cdk/test/integ/cli/app/app.js#L147

That way I could create a file like this one to test it fails

https://github.com/nideveloper/aws-cdk/blob/master/packages/aws-cdk/test/integ/cli/test-cdk-diff.sh

[nideveloper]

@shivlaks I was writing some integration tests there but the existing tests all seem to be based around stacks that can deploy and be interrogated via the cli. A stack with an error on it cannot be deployed. I am pretty sure I can keep modifying until it works but it seems to be against the spirit of the int tests

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[shivlaks]

@shivlaks I was writing some integration tests there but the existing tests all seem to be based around stacks that can deploy and be interrogated via the cli. A stack with an error on it cannot be deployed. I am pretty sure I can keep modifying until it works but it seems to be against the spirit of the int tests

@nideveloper fair point, I don't think it's necessary. have you run the cli integ tests and verified that they all pass?

[shivlaks]

please confirm integ tests run successfully, otherwise LGTM! 🎉

[nideveloper]

Awesome, let me revert my local changes and run them again today then confirm back here!

[nideveloper]

@shivlaks just ran the integration tests and they pass. Will update the branch to be up to date with master and then it should be good to go

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[nideveloper]

@shivlaks just ran the integration tests and they pass. Will update the branch to be up to date with master and then it should be good to go

Code updated to latest from master, automated builds ran and passed. Should be good to go