test: cdk watch integ tests for cli and toolkit lib#1139
test: cdk watch integ tests for cli and toolkit lib#1139
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main aws/aws-cdk-cli#1139 +/- ##
=======================================
Coverage 87.71% 87.72%
=======================================
Files 72 72
Lines 10129 10129
Branches 1338 1337 -1
=======================================
+ Hits 8885 8886 +1
+ Misses 1218 1217 -1
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Use separate output dir to avoid conflicts with lingering watch process | ||
| await fixture.cdkDestroy('test-1', { options: ['--output', 'cdk-destroy.out'] }); |
There was a problem hiding this comment.
No need to manually delete. The rest harness is doing this for us.
| // Use separate output dir to avoid conflicts with lingering watch process | |
| await fixture.cdkDestroy('test-1', { options: ['--output', 'cdk-destroy.out'] }); |
| }), | ||
| ); | ||
|
|
||
| integTest( |
There was a problem hiding this comment.
Only one test per file. Pls move to separate file.
| async function waitForOutput(getOutput: () => string, searchString: string, timeoutMs: number): Promise<void> { | ||
| const startTime = Date.now(); | ||
| return new Promise((resolve, reject) => { | ||
| const check = () => { | ||
| if (getOutput().includes(searchString)) return resolve(); | ||
| if (Date.now() - startTime > timeoutMs) { | ||
| return reject(new Error(`Timeout waiting for: "${searchString}"`)); | ||
| } | ||
| setTimeout(check, 1000); | ||
| }; | ||
| check(); | ||
| }); | ||
| } | ||
|
|
||
| async function waitForCondition(condition: () => boolean, timeoutMs: number, description: string): Promise<void> { | ||
| const startTime = Date.now(); | ||
| return new Promise((resolve, reject) => { | ||
| const check = () => { | ||
| if (condition()) return resolve(); | ||
| if (Date.now() - startTime > timeoutMs) { | ||
| return reject(new Error(`Timeout waiting for ${description}`)); | ||
| } | ||
| setTimeout(check, 1000); | ||
| }; | ||
| check(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Are there really no existing helpers for this?
| }); | ||
|
|
||
| try { | ||
| await waitForOutput(() => output, "Triggering initial 'cdk deploy'", 120000); |
There was a problem hiding this comment.
how did you come up with these timeout durations?
| ); | ||
|
|
||
| integTest( | ||
| 'toolkit watch excludes node_modules and dotfiles by default', |
There was a problem hiding this comment.
why is this test not part of the cli integ tests also?
There was a problem hiding this comment.
Because it will add more time to the test, while offering limited utility IMO. Probably another minute or so. See my PR description where I addressed this discrepancy.
There was a problem hiding this comment.
i dont understand why this would be the case. i think you need to
a) touch node_modules / dotfiles
b) assert that we do not see Detected change to '<file>' -- this happens nearly instantly if the file is being watched, so i dont see why it would take another minute.
as for whether this offers utility, i think its worse if we accidentally don't exclude files from watch than if we accidentally don't include files.
| const cdkJsonPath = path.join(fixture.integTestDir, 'cdk.json'); | ||
| const cdkJson = JSON.parse(fs.readFileSync(cdkJsonPath, 'utf-8')); | ||
| cdkJson.watch = { | ||
| include: ['**/*.ts', '**/*.js'], |
There was a problem hiding this comment.
nit: including the .js files is a bad idea for some use cases (say you're also auto-compiling ts changes, now you're double counting). doesn't seem necessary in your test
| const cdkJson = JSON.parse(fs.readFileSync(cdkJsonPath, 'utf-8')); | ||
| cdkJson.watch = { | ||
| include: ['**/*.ts', '**/*.js'], | ||
| exclude: ['node_modules/**', 'cdk.out/**', '**/*.d.ts'], |
There was a problem hiding this comment.
this part isn't tested. in this test or a new one, we should be testing that changes to exclude files do not trigger deployment.
| include: ['**/*.ts', '**/*.js'], | ||
| exclude: ['node_modules/**', 'cdk.out/**', '**/*.d.ts'], | ||
| }; | ||
| fs.writeFileSync(cdkJsonPath, JSON.stringify(cdkJson, null, 2)); |
There was a problem hiding this comment.
we can also just call touch on the file.
Tests #1134
Description of tests
cdk watchin CLIToolkit.watch()We used AI 🤖 to help with this contribution.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license