Skip to content

Commit d83463b

Browse files
mrgrainmrgrain
committed
fix(integ-runner): improve git directory handling for snapshot operations
Fixes issues with git operations by using the -C flag to specify the correct git repository directory. This ensures integ-runner works correctly when executed from outside the repository under test. Also standardizes the directory used for running CDK apps to always be the current working directory.
1 parent d71960e commit d83463b

File tree

6 files changed

+68
-80
lines changed

6 files changed

+68
-80
lines changed

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

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { IntegRunner } from './runner-base';
1111
import * as logger from '../logger';
1212
import { chunks, exec, promiseWithResolvers } from '../utils';
1313
import type { DestructiveChange, AssertionResults, AssertionResult } from '../workers/common';
14-
import { DiagnosticReason, formatAssertionResults } from '../workers/common';
14+
import { DiagnosticReason, formatAssertionResults, formatError } from '../workers/common';
1515

1616
export interface CommonOptions {
1717
/**
@@ -114,15 +114,19 @@ export class IntegTestRunner extends IntegRunner {
114114
* all branches and we then search for one that starts with `HEAD branch: `
115115
*/
116116
private checkoutSnapshot(): void {
117-
const cwd = this.directory;
117+
// We use the directory that contains the snapshot to run git commands in
118+
// We don't change the cwd for executing git, but instead use the -C flag
119+
// @see https://git-scm.com/docs/git#Documentation/git.txt--Cltpathgt
120+
// This way we are guaranteed to operate under the correct git repo, even
121+
// when executing integ-runner from outside the repo under test.
122+
const gitCwd = path.dirname(this.snapshotDir);
123+
const git = ['git', '-C', gitCwd];
118124

119125
// https://git-scm.com/docs/git-merge-base
120126
let baseBranch: string | undefined = undefined;
121127
// try to find the base branch that the working branch was created from
122128
try {
123-
const origin: string = exec(['git', 'remote', 'show', 'origin'], {
124-
cwd,
125-
});
129+
const origin: string = exec([...git, 'remote', 'show', 'origin']);
126130
const originLines = origin.split('\n');
127131
for (const line of originLines) {
128132
if (line.trim().startsWith('HEAD branch: ')) {
@@ -141,22 +145,18 @@ export class IntegTestRunner extends IntegRunner {
141145
// if we found the base branch then get the merge-base (most recent common commit)
142146
// and checkout the snapshot using that commit
143147
if (baseBranch) {
144-
const relativeSnapshotDir = path.relative(this.directory, this.snapshotDir);
148+
const relativeSnapshotDir = path.relative(gitCwd, this.snapshotDir);
145149

146150
try {
147-
const base = exec(['git', 'merge-base', 'HEAD', baseBranch], {
148-
cwd,
149-
});
150-
exec(['git', 'checkout', base, '--', relativeSnapshotDir], {
151-
cwd,
152-
});
151+
const base = exec([...git, 'merge-base', 'HEAD', baseBranch]);
152+
exec([...git, 'checkout', base, '--', relativeSnapshotDir]);
153153
} catch (e) {
154154
logger.warning('%s\n%s',
155155
`Could not checkout snapshot directory '${this.snapshotDir}'. Please verify the following command completes correctly:`,
156-
`git checkout $(git merge-base HEAD ${baseBranch}) -- ${relativeSnapshotDir}`,
156+
`git -C ${gitCwd} checkout $(git -C ${gitCwd} merge-base HEAD ${baseBranch}) -- ${relativeSnapshotDir}`,
157157
'',
158158
);
159-
logger.warning('error: %s', e);
159+
logger.warning('error: %s', formatError(e));
160160
}
161161
}
162162
}

‎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: 33 additions & 33 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

@@ -442,13 +442,13 @@ describe('IntegTest runIntegTests', () => {
442442
// THEN
443443
expect(spawnSyncMock.mock.calls).toEqual(expect.arrayContaining([
444444
expect.arrayContaining([
445-
'git', ['remote', 'show', 'origin'],
445+
'git', ['-C', expect.any(String), 'remote', 'show', 'origin'],
446446
]),
447447
expect.arrayContaining([
448-
'git', ['merge-base', 'HEAD', 'main'],
448+
'git', ['-C', expect.any(String), 'merge-base', 'HEAD', 'main'],
449449
]),
450450
expect.arrayContaining([
451-
'git', ['checkout', 'abc', '--', 'xxxxx.test-with-snapshot.js.snapshot'],
451+
'git', ['-C', expect.any(String), 'checkout', 'abc', '--', 'xxxxx.test-with-snapshot.js.snapshot'],
452452
]),
453453
]));
454454
});
@@ -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/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
});

0 commit comments

Comments
 (0)