Repository navigation
Conversation
The current version of the integ-runner does not pass the `CDK_DEFAULT_ACCOUNT` and `CDK_DEFAULT_REGION` environment variables to the app that's being tested if it is being deployed. This means that any feature that relies on the region being available, like lookups, doesn't work. Also tests that rely on `stack.accountId` and `stack.region` being resolved values (like the `ProductStack` tests) fail. Make sure the region gets resolved when we are synthesizing for deployments.
A `--to-file` option can be used to write a list of all files that need updatin' to a text file. That way, if some fail it will be easy to re-run the small subset of tests out of a very large pool without unnecessarily resynthesizing the tests that don't need any work.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit This PR adds a The async conversion is handled consistently — 🔴 1 blocking · 🟡 3 recommended · ⚪ 0 optional Files with findings (2)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
![maintainer-for-aws[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| @@ -16,7 +16,7 @@ import { watchIntegrationTest } from './workers/integ-watch-worker'; | |||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | |||
| const yargs = require('yargs'); | |||
|
|
|||
There was a problem hiding this comment.
🔴 Blocking — Making parseCliArgs async changes its return type to a Promise, but the unchanged test file packages/@aws-cdk/integ-runner/test/cli.test.ts still calls it synchronously and reads properties straight off the result (const options = parseCliArgs([...]); options.tests, options.proxy, options.roleArn, etc.). Those accesses now land on a Promise, so they are undefined at runtime and fail to type-check under strict mode. The suite also uses expect(() => parseCliArgs([...])).toThrow(...) for argument-validation cases, but an async function no longer throws synchronously — it returns a rejected Promise — so .toThrow() no longer matches (AGENTS.md#testing).
Suggested change: Update cli.test.ts in this PR alongside the signature change: await each call (const options = await parseCliArgs([...])) and convert synchronous throw expectations to await expect(parseCliArgs([...])).rejects.toThrow('...'). While there, add a focused test for the new --to-file write behavior to lock in the round-trip contract with --from-file.
A `--to-file` option can be used to write a list of all files that need updatin' to a text file. That way, if some fail it will be easy to re-run the small subset of tests out of a very large pool without unnecessarily resynthesizing the tests that don't need any work.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2132 +/- ##
==========================================
- Coverage 92.05% 92.04% -0.01%
==========================================
Files 82 82
Lines 13195 13206 +11
Branches 1894 1897 +3
==========================================
+ Hits 12146 12156 +10
- Misses 1011 1012 +1
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| testsSucceeded = success; | ||
|
|
||
| if (options.failedToFile) { | ||
| // At this point, "failed" means "everything that failed its snapshot test that didn't succeed" |
There was a problem hiding this comment.
🟡 Recommended — The new --failed-to-file option adds two distinct write points with deliberate overwrite semantics — the snapshot phase writes the failed snapshot test names, then the deploy phase overwrites with the tests that failed after deployment — but no test exercises any of it; the cli.test.ts changes are purely the mechanical await conversion. An unparsed and untested toggle, especially the layering between phases and the empty-failure case that clears stale content, can regress silently.
Suggested change: Add a parseCliArgs(['--failed-to-file', f]) assertion on options.failedToFile, plus a run-level test (or extract the write into a small testable helper) asserting the file contents for the snapshot-only and deploy phases, including the empty-failure case.
| /** | ||
| * Synthesis purpose | ||
| * | ||
| * - 'snapshot' indicates that the synthesis is for creating a snapshot of the stack. | ||
| * - 'deployment' indicates that the synthesis is for an actual deployment. | ||
| */ | ||
| export type SynthesisPurpose = 'snapshot' | 'deployment'; | ||
|
|
||
| /** | ||
| * Options to use with cdk synth | ||
| */ | ||
| export interface SynthOptions { | ||
| /** | ||
| * What the goal of this synthesis is. | ||
| */ | ||
| readonly purpose: SynthesisPurpose; |
There was a problem hiding this comment.
🟡 Recommended — This PR's stated concern is the --failed-to-file feature, but it also introduces a logically distinct behavior change: a new SynthesisPurpose type and required SynthOptions.purpose field that flips resolveDefaultEnvironment from a hardcoded false to options.purpose === 'deployment', so default account/region resolution now happens for deployment synths but not snapshot synths. That is a real synthesis-behavior change unrelated to recording failed tests, touching several files, which makes the change harder to review and to describe in a single changelog entry (AGENTS.md#pr-conventions).
Suggested change: Split the SynthesisPurpose/resolveDefaultEnvironment change into its own PR, or call it out explicitly in the PR body and confirm it is intended to ship here.
| .options('exclude', { type: 'boolean', desc: 'Run all tests in the directory, except the specified TESTs', default: false }) | ||
| .option('strict', { type: 'boolean', default: false, desc: 'Fail if any specified tests are not found' }) | ||
| .options('from-file', { type: 'string', desc: 'Read TEST names from a file (one TEST per line)' }) | ||
| .options('failed-to-file', { type: 'string', desc: 'Write failed test names to a file, for easy re-running of failed tests' }) |
There was a problem hiding this comment.
🟡 Recommended — The PR title and body refer to a --to-file option, but the option actually registered here is --failed-to-file (failedToFile). The user-facing flag name a reader is told to use does not exist, so anyone following the description would type a flag the tool does not accept.
Suggested change: Pick one name and align the PR body with the code (or rename the flag to --to-file if that is the intended surface), and confirm which name you want to commit to.
A
--to-fileoption can be used to write a list of all files that need updatin' to a text file. That way, if some fail it will be easy to re-run the small subset of tests out of a very large pool without unnecessarily resynthesizing the tests that don't need any work.Should only be merged after #2131 is merged.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license