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.

Sign up for GitHub

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

chore(doc): update INTEGRATION_TESTS.md #30880

Closed
wants to merge 3 commits into from
+2 −0
Closed

chore(doc): update INTEGRATION_TESTS.md #30880

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e06a2cb
chore(doc): Update INTEGRATION_TESTS.md
pahud Jul 17, 2024
fec6ce0
Update INTEGRATION_TESTS.md
pahud Jul 17, 2024
318e86b
Update INTEGRATION_TESTS.md
pahud Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions INTEGRATION_TESTS.md
View file
Original file line number Diff line number Diff line change
@@ -119,6 +119,9 @@ This will:
Now when you run `npm test` it will synth the integ app and compare the result with the snapshot.
If the snapshot has changed the same process must be followed to update the snapshot.

*Note - When running integration tests, refrain from defining the `env.account` property for stacks. This ensures that the synthesized
Copy link
Contributor

Choose a reason for hiding this comment

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

Note should be bold 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the answer to this issue. I don't think account numbers are inherently considered sensitive information, but even if we decide to them as such, we should write a linter rule that explicitly prevents users from including them in snapshots.

This might not apply to all tests, some of them need real account numbers (like cross account tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@comcalvi Agree. I have rephrased it this way

Note - When running integration tests, env.account is generally not required unless some special cases like cross-account tests.

Feel free to suggest changes. I think we still have to remind users that env.account is generally not required and not recommended. Thoughts?

CloudFormation template from the snapshots does not contain your current AWS account information, promoting better security and portability.

### New L2 Constructs

When creating a new L2 construct (or new construct library) it is important to ensure you have a good