Skip to content

Commit 742ccfc

Browse files
mrgrainmrgrain
authored
fix(integ-runner): inconsistent and unexpected CWD in test app execution (#720)
Fixes an issue with the working directory being inconsistent and unexpected during execution of the test app. Previously we had two different behaviors: For regular execution we would execute the app from the directory of the test file, but for watch we would run it in the actual user's CWD. This is inconsistent behavior making it difficult to write integ test cases than can be executed across all modes. Additionally as reported in #638, languages likes python may rely on the working directory to import modules. The working directory changing compared to library code and modes, makes it very difficult to use integ-runner in these languages. This change aligns the working directory for test app execution to always be the CWD integ-runner is executed from. Given the two existing choices, this is the less arbitrary one and aligns more with how running a test app directly (`node integ.app-under-test.test.js`) would work. The practical implications of this are limited. Tests are already discovered and executed using relative paths. In case someone really dependent on this behavior, they can simply execute integ-runner from a different working dir. Fixes #638 --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
1 parent 779352d commit 742ccfc

File tree

6 files changed

+57
-51
lines changed

6 files changed

+57
-51
lines changed

‎packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts‎

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -104,24 +104,19 @@ export class IntegTest {
104104

105105
constructor(public readonly info: IntegTestInfo) {
106106
this.appCommand = info.appCommand ?? 'node {filePath}';
107-
this.absoluteFileName = path.resolve(info.fileName);
108-
this.fileName = path.relative(process.cwd(), info.fileName);
109107

110-
const parsed = path.parse(this.fileName);
108+
// for consistency, always run the CDK apps under test from the CWD
109+
// this is especially important for languages that use the CWD to discover assets
110+
// @see https://github.com/aws/aws-cdk-cli/issues/638
111+
this.directory = process.cwd();
112+
this.absoluteFileName = path.resolve(info.fileName);
113+
this.fileName = path.relative(this.directory, info.fileName);
111114
this.discoveryRelativeFileName = path.relative(info.discoveryRoot, info.fileName);
112-
// if `--watch` then we need the directory to be the cwd
113-
this.directory = info.watch ? process.cwd() : parsed.dir;
114-
115-
// if we are running in a package directory then just use the fileName
116-
// as the testname, but if we are running in a parent directory with
117-
// multiple packages then use the directory/filename as the testname
118-
//
119-
// Looks either like `integ.mytest` or `package/test/integ.mytest`.
120-
const relDiscoveryRoot = path.relative(process.cwd(), info.discoveryRoot);
121-
this.testName = this.directory === path.join(relDiscoveryRoot, 'test') || this.directory === path.join(relDiscoveryRoot)
122-
? parsed.name
123-
: path.join(path.relative(this.info.discoveryRoot, parsed.dir), parsed.name);
124115

116+
// We treat the discovery root as the base for display names
117+
// Looks either like `integ.mytest` or `package/test/integ.mytest`
118+
const parsed = path.parse(this.fileName);
119+
this.testName = path.join(path.relative(this.info.discoveryRoot, parsed.dir), parsed.name);
125120
this.normalizedTestName = parsed.name;
126121
this.snapshotDir = path.join(parsed.dir, `${parsed.base}.snapshot`);
127122
this.temporaryOutputDir = path.join(parsed.dir, `${CDK_OUTDIR_PREFIX}.${parsed.base}.snapshot`);

‎packages/@aws-cdk/integ-runner/test/helpers.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,14 +78,16 @@ export class MockCdkProvider {
7878
diagnostics: Diagnostic[];
7979
destructiveChanges: DestructiveChange[];
8080
}> {
81+
const actualSnapshotLocation = actualSnapshot ? 'test/test-data/' + actualSnapshot : undefined;
82+
8183
// WHEN
8284
const integTest = new IntegSnapshotRunner({
8385
cdk: this.cdk,
8486
test: new IntegTest({
8587
fileName: 'test/test-data/' + integTestFile,
8688
discoveryRoot: 'test/test-data',
8789
}),
88-
integOutDir: actualSnapshot ? 'test/test-data/' + actualSnapshot : undefined,
90+
integOutDir: actualSnapshotLocation,
8991
});
9092

9193
const results = await integTest.testSnapshot();
@@ -98,8 +100,8 @@ export class MockCdkProvider {
98100
CDK_INTEG_REGION: 'test-region',
99101
}),
100102
context: expect.any(Object),
101-
execCmd: ['node', integTestFile],
102-
output: actualSnapshot ?? `cdk-integ.out.${integTestFile}.snapshot`,
103+
execCmd: ['node', 'test/test-data/' + integTestFile],
104+
output: actualSnapshotLocation ?? `test/test-data/cdk-integ.out.${integTestFile}.snapshot`,
103105
});
104106

105107
return results;

‎packages/@aws-cdk/integ-runner/test/runner/integ-test-runner.test.ts‎

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ describe('IntegTest runIntegTests', () => {
6161
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
6262
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
6363
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
64-
app: 'xxxxx.test-with-snapshot.js.snapshot',
64+
app: 'test/test-data/xxxxx.test-with-snapshot.js.snapshot',
6565
requireApproval: 'never',
6666
pathMetadata: false,
6767
assetMetadata: false,
@@ -76,11 +76,11 @@ describe('IntegTest runIntegTests', () => {
7676
stacks: ['test-stack'],
7777
});
7878
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
79-
app: 'node xxxxx.test-with-snapshot.js',
79+
app: 'node test/test-data/xxxxx.test-with-snapshot.js',
8080
requireApproval: 'never',
8181
pathMetadata: false,
8282
assetMetadata: false,
83-
output: 'cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
83+
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
8484
profile: undefined,
8585
context: expect.not.objectContaining({
8686
'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({
@@ -92,7 +92,7 @@ describe('IntegTest runIntegTests', () => {
9292
stacks: ['test-stack', 'new-test-stack'],
9393
});
9494
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
95-
app: 'node xxxxx.test-with-snapshot.js',
95+
app: 'node test/test-data/xxxxx.test-with-snapshot.js',
9696
pathMetadata: false,
9797
assetMetadata: false,
9898
context: expect.not.objectContaining({
@@ -104,7 +104,7 @@ describe('IntegTest runIntegTests', () => {
104104
profile: undefined,
105105
force: true,
106106
all: true,
107-
output: 'cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
107+
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
108108
});
109109
});
110110

@@ -126,7 +126,7 @@ describe('IntegTest runIntegTests', () => {
126126
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
127127
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
128128
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
129-
app: 'node xxxxx.integ-test1.js',
129+
app: 'node test/test-data/xxxxx.integ-test1.js',
130130
requireApproval: 'never',
131131
pathMetadata: false,
132132
assetMetadata: false,
@@ -137,17 +137,17 @@ describe('IntegTest runIntegTests', () => {
137137
}),
138138
lookups: false,
139139
stacks: ['stack1'],
140-
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
140+
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
141141
});
142142
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
143-
app: 'node xxxxx.integ-test1.js',
143+
app: 'node test/test-data/xxxxx.integ-test1.js',
144144
pathMetadata: false,
145145
assetMetadata: false,
146146
versionReporting: false,
147147
context: expect.any(Object),
148148
force: true,
149149
all: true,
150-
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
150+
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
151151
});
152152
});
153153

@@ -169,7 +169,7 @@ describe('IntegTest runIntegTests', () => {
169169
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
170170
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
171171
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
172-
app: 'node xxxxx.test-with-snapshot-assets-diff.js',
172+
app: 'node test/test-data/xxxxx.test-with-snapshot-assets-diff.js',
173173
requireApproval: 'never',
174174
pathMetadata: false,
175175
assetMetadata: false,
@@ -181,11 +181,11 @@ describe('IntegTest runIntegTests', () => {
181181
versionReporting: false,
182182
lookups: true,
183183
stacks: ['test-stack'],
184-
output: 'cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
184+
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
185185
profile: undefined,
186186
});
187187
expect(cdkMock.mocks.synthFast).toHaveBeenCalledWith({
188-
execCmd: ['node', 'xxxxx.test-with-snapshot-assets-diff.js'],
188+
execCmd: ['node', 'test/test-data/xxxxx.test-with-snapshot-assets-diff.js'],
189189
env: expect.objectContaining({
190190
CDK_INTEG_ACCOUNT: '12345678',
191191
CDK_INTEG_REGION: 'test-region',
@@ -195,10 +195,10 @@ describe('IntegTest runIntegTests', () => {
195195
vpcId: 'vpc-60900905',
196196
}),
197197
}),
198-
output: 'xxxxx.test-with-snapshot-assets-diff.js.snapshot',
198+
output: 'test/test-data/xxxxx.test-with-snapshot-assets-diff.js.snapshot',
199199
});
200200
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
201-
app: 'node xxxxx.test-with-snapshot-assets-diff.js',
201+
app: 'node test/test-data/xxxxx.test-with-snapshot-assets-diff.js',
202202
pathMetadata: false,
203203
assetMetadata: false,
204204
context: expect.not.objectContaining({
@@ -209,7 +209,7 @@ describe('IntegTest runIntegTests', () => {
209209
versionReporting: false,
210210
force: true,
211211
all: true,
212-
output: 'cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
212+
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
213213
});
214214
});
215215

@@ -231,20 +231,20 @@ describe('IntegTest runIntegTests', () => {
231231
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
232232
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
233233
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(1, expect.objectContaining({
234-
app: 'xxxxx.test-with-snapshot.js.snapshot',
234+
app: 'test/test-data/xxxxx.test-with-snapshot.js.snapshot',
235235
context: expect.any(Object),
236236
stacks: ['test-stack'],
237237
}));
238238
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(2, expect.not.objectContaining({
239239
rollback: false,
240240
}));
241241
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(3, expect.objectContaining({
242-
app: 'node xxxxx.test-with-snapshot.js',
242+
app: 'node test/test-data/xxxxx.test-with-snapshot.js',
243243
stacks: ['Bundling/DefaultTest/DeployAssert'],
244244
rollback: false,
245245
}));
246246
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
247-
app: 'node xxxxx.test-with-snapshot.js',
247+
app: 'node test/test-data/xxxxx.test-with-snapshot.js',
248248
pathMetadata: false,
249249
assetMetadata: false,
250250
context: expect.not.objectContaining({
@@ -255,7 +255,7 @@ describe('IntegTest runIntegTests', () => {
255255
versionReporting: false,
256256
force: true,
257257
all: true,
258-
output: 'cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
258+
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
259259
});
260260
});
261261

@@ -313,8 +313,8 @@ describe('IntegTest runIntegTests', () => {
313313
// THEN
314314
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(1);
315315
expect(cdkMock.mocks.synthFast).toHaveBeenCalledWith({
316-
execCmd: ['node', 'xxxxx.integ-test1.js'],
317-
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
316+
execCmd: ['node', 'test/test-data/xxxxx.integ-test1.js'],
317+
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
318318
env: expect.objectContaining({
319319
CDK_INTEG_ACCOUNT: '12345678',
320320
CDK_INTEG_REGION: 'test-region',
@@ -342,7 +342,7 @@ describe('IntegTest runIntegTests', () => {
342342
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
343343
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
344344
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
345-
app: 'node xxxxx.integ-test1.js',
345+
app: 'node test/test-data/xxxxx.integ-test1.js',
346346
requireApproval: 'never',
347347
pathMetadata: false,
348348
assetMetadata: false,
@@ -355,10 +355,10 @@ describe('IntegTest runIntegTests', () => {
355355
profile: 'test-profile',
356356
lookups: false,
357357
stacks: ['stack1'],
358-
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
358+
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
359359
});
360360
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
361-
app: 'node xxxxx.integ-test1.js',
361+
app: 'node test/test-data/xxxxx.integ-test1.js',
362362
pathMetadata: false,
363363
assetMetadata: false,
364364
versionReporting: false,
@@ -370,7 +370,7 @@ describe('IntegTest runIntegTests', () => {
370370
profile: 'test-profile',
371371
force: true,
372372
all: true,
373-
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
373+
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
374374
});
375375
});
376376

@@ -625,13 +625,13 @@ describe('IntegTest runIntegTests', () => {
625625
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
626626
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
627627
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith(expect.objectContaining({
628-
app: 'node --no-warnings xxxxx.test-with-snapshot.js',
628+
app: 'node --no-warnings test/test-data/xxxxx.test-with-snapshot.js',
629629
}));
630630
expect(cdkMock.mocks.synthFast).toHaveBeenCalledWith(expect.objectContaining({
631-
execCmd: ['node', '--no-warnings', 'xxxxx.test-with-snapshot.js'],
631+
execCmd: ['node', '--no-warnings', 'test/test-data/xxxxx.test-with-snapshot.js'],
632632
}));
633633
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith(expect.objectContaining({
634-
app: 'node --no-warnings xxxxx.test-with-snapshot.js',
634+
app: 'node --no-warnings test/test-data/xxxxx.test-with-snapshot.js',
635635
}));
636636
});
637637
});
@@ -655,7 +655,7 @@ describe('IntegTest watchIntegTest', () => {
655655

656656
// THEN
657657
expect(cdkMock.mocks.watch).toHaveBeenCalledWith(expect.objectContaining({
658-
app: 'node --no-warnings xxxxx.test-with-snapshot.js',
658+
app: 'node --no-warnings test/test-data/xxxxx.test-with-snapshot.js',
659659
hotswap: HotswapMode.FALL_BACK,
660660
watch: true,
661661
traceLogs: false,
@@ -683,7 +683,7 @@ describe('IntegTest watchIntegTest', () => {
683683

684684
// THEN
685685
expect(cdkMock.mocks.watch).toHaveBeenCalledWith(expect.objectContaining({
686-
app: 'node --no-warnings xxxxx.test-with-snapshot.js',
686+
app: 'node --no-warnings test/test-data/xxxxx.test-with-snapshot.js',
687687
hotswap: HotswapMode.FALL_BACK,
688688
watch: true,
689689
traceLogs: true,

‎packages/@aws-cdk/integ-runner/test/runner/integration-tests.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,4 +138,13 @@ describe('IntegrationTests Discovery', () => {
138138
expect(integTests[2].fileName).toEqual(expect.stringMatching(new RegExp('^.*test3\\.js$')));
139139
});
140140
});
141+
142+
describe('IntegTest directory is always cwd', () => {
143+
test('directory is set to process.cwd()', async () => {
144+
const integTests = await tests.fromCliOptions({ language: ['javascript'] });
145+
146+
expect(integTests.length).toBeGreaterThan(0);
147+
expect(integTests[0].directory).toEqual(process.cwd());
148+
});
149+
});
141150
});

‎packages/@aws-cdk/integ-runner/test/runner/snapshot-test-runner.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,13 +219,13 @@ describe('IntegTest runSnapshotTests', () => {
219219
}));
220220
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(1);
221221
expect(cdkMock.mocks.synthFast).toHaveBeenCalledWith({
222-
execCmd: ['node', 'xxxxx.integ-test2.js'],
222+
execCmd: ['node', 'test/test-data/xxxxx.integ-test2.js'],
223223
env: expect.objectContaining({
224224
CDK_INTEG_ACCOUNT: '12345678',
225225
CDK_INTEG_REGION: 'test-region',
226226
}),
227227
context: expect.any(Object),
228-
output: '../../does/not/exist',
228+
output: 'does/not/exist',
229229
});
230230
});
231231
});

‎packages/@aws-cdk/integ-runner/test/workers/integ-worker.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ describe('test runner', () => {
102102

103103
expect(spawnSyncMock).toHaveBeenCalledWith(
104104
expect.stringMatching(/node/),
105-
['xxxxx.integ-test1.js'],
105+
['test/test-data/xxxxx.integ-test1.js'],
106106
expect.objectContaining({
107107
env: expect.objectContaining({
108108
CDK_INTEG_ACCOUNT: '12345678',

0 commit comments

Comments
 (0)