Skip to content

fix(integ-runner): inconsistent and unexpected CWD in test app execution #720

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(integ-runner): inconsistent and unexpected CWD in test app execution #720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 14 additions & 14 deletions packages/@aws-cdk/integ-runner/lib/runner/integ-test-runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { IntegRunner } from './runner-base';
import * as logger from '../logger';
import { chunks, exec, promiseWithResolvers } from '../utils';
import type { DestructiveChange, AssertionResults, AssertionResult } from '../workers/common';
import { DiagnosticReason, formatAssertionResults } from '../workers/common';
import { DiagnosticReason, formatAssertionResults, formatError } from '../workers/common';

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

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

try {
const base = exec(['git', 'merge-base', 'HEAD', baseBranch], {
cwd,
});
exec(['git', 'checkout', base, '--', relativeSnapshotDir], {
cwd,
});
const base = exec([...git, 'merge-base', 'HEAD', baseBranch]);
exec([...git, 'checkout', base, '--', relativeSnapshotDir]);
} catch (e) {
logger.warning('%s\n%s',
`Could not checkout snapshot directory '${this.snapshotDir}'. Please verify the following command completes correctly:`,
`git checkout $(git merge-base HEAD ${baseBranch}) -- ${relativeSnapshotDir}`,
`git -C ${gitCwd} checkout $(git -C ${gitCwd} merge-base HEAD ${baseBranch}) -- ${relativeSnapshotDir}`,
'',
);
logger.warning('error: %s', e);
logger.warning('error: %s', formatError(e));
}
}
}
Expand Down
25 changes: 10 additions & 15 deletions packages/@aws-cdk/integ-runner/lib/runner/integration-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,24 +104,19 @@ export class IntegTest {

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

const parsed = path.parse(this.fileName);
// for consistency, always run the CDK apps under test from the CWD
// this is especially important for languages that use the CWD to discover assets
// @see https://github.com/aws/aws-cdk-cli/issues/638
this.directory = process.cwd();
this.absoluteFileName = path.resolve(info.fileName);
this.fileName = path.relative(this.directory, info.fileName);
this.discoveryRelativeFileName = path.relative(info.discoveryRoot, info.fileName);
// if `--watch` then we need the directory to be the cwd
this.directory = info.watch ? process.cwd() : parsed.dir;

// if we are running in a package directory then just use the fileName
// as the testname, but if we are running in a parent directory with
// multiple packages then use the directory/filename as the testname
//
// Looks either like `integ.mytest` or `package/test/integ.mytest`.
const relDiscoveryRoot = path.relative(process.cwd(), info.discoveryRoot);
this.testName = this.directory === path.join(relDiscoveryRoot, 'test') || this.directory === path.join(relDiscoveryRoot)
? parsed.name
: path.join(path.relative(this.info.discoveryRoot, parsed.dir), parsed.name);

// We treat the discovery root as the base for display names
// Looks either like `integ.mytest` or `package/test/integ.mytest`
const parsed = path.parse(this.fileName);
this.testName = path.join(path.relative(this.info.discoveryRoot, parsed.dir), parsed.name);
this.normalizedTestName = parsed.name;
this.snapshotDir = path.join(parsed.dir, `${parsed.base}.snapshot`);
this.temporaryOutputDir = path.join(parsed.dir, `${CDK_OUTDIR_PREFIX}.${parsed.base}.snapshot`);
Expand Down
8 changes: 5 additions & 3 deletions packages/@aws-cdk/integ-runner/test/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,16 @@ export class MockCdkProvider {
diagnostics: Diagnostic[];
destructiveChanges: DestructiveChange[];
}> {
const actualSnapshotLocation = actualSnapshot ? 'test/test-data/' + actualSnapshot : undefined;

// WHEN
const integTest = new IntegSnapshotRunner({
cdk: this.cdk,
test: new IntegTest({
fileName: 'test/test-data/' + integTestFile,
discoveryRoot: 'test/test-data',
}),
integOutDir: actualSnapshot ? 'test/test-data/' + actualSnapshot : undefined,
integOutDir: actualSnapshotLocation,
});

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

return results;
Expand Down
66 changes: 33 additions & 33 deletions packages/@aws-cdk/integ-runner/test/runner/integ-test-runner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe('IntegTest runIntegTests', () => {
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
app: 'xxxxx.test-with-snapshot.js.snapshot',
app: 'test/test-data/xxxxx.test-with-snapshot.js.snapshot',
requireApproval: 'never',
pathMetadata: false,
assetMetadata: false,
Expand All @@ -76,11 +76,11 @@ describe('IntegTest runIntegTests', () => {
stacks: ['test-stack'],
});
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
app: 'node xxxxx.test-with-snapshot.js',
app: 'node test/test-data/xxxxx.test-with-snapshot.js',
requireApproval: 'never',
pathMetadata: false,
assetMetadata: false,
output: 'cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
profile: undefined,
context: expect.not.objectContaining({
'vpc-provider:account=12345678:filter.isDefault=true:region=test-region:returnAsymmetricSubnets=true': expect.objectContaining({
Expand All @@ -92,7 +92,7 @@ describe('IntegTest runIntegTests', () => {
stacks: ['test-stack', 'new-test-stack'],
});
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
app: 'node xxxxx.test-with-snapshot.js',
app: 'node test/test-data/xxxxx.test-with-snapshot.js',
pathMetadata: false,
assetMetadata: false,
context: expect.not.objectContaining({
Expand All @@ -104,7 +104,7 @@ describe('IntegTest runIntegTests', () => {
profile: undefined,
force: true,
all: true,
output: 'cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
});
});

Expand All @@ -126,7 +126,7 @@ describe('IntegTest runIntegTests', () => {
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
app: 'node xxxxx.integ-test1.js',
app: 'node test/test-data/xxxxx.integ-test1.js',
requireApproval: 'never',
pathMetadata: false,
assetMetadata: false,
Expand All @@ -137,17 +137,17 @@ describe('IntegTest runIntegTests', () => {
}),
lookups: false,
stacks: ['stack1'],
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
});
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
app: 'node xxxxx.integ-test1.js',
app: 'node test/test-data/xxxxx.integ-test1.js',
pathMetadata: false,
assetMetadata: false,
versionReporting: false,
context: expect.any(Object),
force: true,
all: true,
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
});
});

Expand All @@ -169,7 +169,7 @@ describe('IntegTest runIntegTests', () => {
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
app: 'node xxxxx.test-with-snapshot-assets-diff.js',
app: 'node test/test-data/xxxxx.test-with-snapshot-assets-diff.js',
requireApproval: 'never',
pathMetadata: false,
assetMetadata: false,
Expand All @@ -181,11 +181,11 @@ describe('IntegTest runIntegTests', () => {
versionReporting: false,
lookups: true,
stacks: ['test-stack'],
output: 'cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
profile: undefined,
});
expect(cdkMock.mocks.synthFast).toHaveBeenCalledWith({
execCmd: ['node', 'xxxxx.test-with-snapshot-assets-diff.js'],
execCmd: ['node', 'test/test-data/xxxxx.test-with-snapshot-assets-diff.js'],
env: expect.objectContaining({
CDK_INTEG_ACCOUNT: '12345678',
CDK_INTEG_REGION: 'test-region',
Expand All @@ -195,10 +195,10 @@ describe('IntegTest runIntegTests', () => {
vpcId: 'vpc-60900905',
}),
}),
output: 'xxxxx.test-with-snapshot-assets-diff.js.snapshot',
output: 'test/test-data/xxxxx.test-with-snapshot-assets-diff.js.snapshot',
});
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
app: 'node xxxxx.test-with-snapshot-assets-diff.js',
app: 'node test/test-data/xxxxx.test-with-snapshot-assets-diff.js',
pathMetadata: false,
assetMetadata: false,
context: expect.not.objectContaining({
Expand All @@ -209,7 +209,7 @@ describe('IntegTest runIntegTests', () => {
versionReporting: false,
force: true,
all: true,
output: 'cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot-assets-diff.js.snapshot',
});
});

Expand All @@ -231,20 +231,20 @@ describe('IntegTest runIntegTests', () => {
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(1, expect.objectContaining({
app: 'xxxxx.test-with-snapshot.js.snapshot',
app: 'test/test-data/xxxxx.test-with-snapshot.js.snapshot',
context: expect.any(Object),
stacks: ['test-stack'],
}));
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(2, expect.not.objectContaining({
rollback: false,
}));
expect(cdkMock.mocks.deploy).toHaveBeenNthCalledWith(3, expect.objectContaining({
app: 'node xxxxx.test-with-snapshot.js',
app: 'node test/test-data/xxxxx.test-with-snapshot.js',
stacks: ['Bundling/DefaultTest/DeployAssert'],
rollback: false,
}));
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
app: 'node xxxxx.test-with-snapshot.js',
app: 'node test/test-data/xxxxx.test-with-snapshot.js',
pathMetadata: false,
assetMetadata: false,
context: expect.not.objectContaining({
Expand All @@ -255,7 +255,7 @@ describe('IntegTest runIntegTests', () => {
versionReporting: false,
force: true,
all: true,
output: 'cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.test-with-snapshot.js.snapshot',
});
});

Expand Down Expand Up @@ -313,8 +313,8 @@ describe('IntegTest runIntegTests', () => {
// THEN
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledWith({
execCmd: ['node', 'xxxxx.integ-test1.js'],
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
execCmd: ['node', 'test/test-data/xxxxx.integ-test1.js'],
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
env: expect.objectContaining({
CDK_INTEG_ACCOUNT: '12345678',
CDK_INTEG_REGION: 'test-region',
Expand Down Expand Up @@ -342,7 +342,7 @@ describe('IntegTest runIntegTests', () => {
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith({
app: 'node xxxxx.integ-test1.js',
app: 'node test/test-data/xxxxx.integ-test1.js',
requireApproval: 'never',
pathMetadata: false,
assetMetadata: false,
Expand All @@ -355,10 +355,10 @@ describe('IntegTest runIntegTests', () => {
profile: 'test-profile',
lookups: false,
stacks: ['stack1'],
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
});
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith({
app: 'node xxxxx.integ-test1.js',
app: 'node test/test-data/xxxxx.integ-test1.js',
pathMetadata: false,
assetMetadata: false,
versionReporting: false,
Expand All @@ -370,7 +370,7 @@ describe('IntegTest runIntegTests', () => {
profile: 'test-profile',
force: true,
all: true,
output: 'cdk-integ.out.xxxxx.integ-test1.js.snapshot',
output: 'test/test-data/cdk-integ.out.xxxxx.integ-test1.js.snapshot',
});
});

Expand Down Expand Up @@ -442,13 +442,13 @@ describe('IntegTest runIntegTests', () => {
// THEN
expect(spawnSyncMock.mock.calls).toEqual(expect.arrayContaining([
expect.arrayContaining([
'git', ['remote', 'show', 'origin'],
'git', ['-C', expect.any(String), 'remote', 'show', 'origin'],
]),
expect.arrayContaining([
'git', ['merge-base', 'HEAD', 'main'],
'git', ['-C', expect.any(String), 'merge-base', 'HEAD', 'main'],
]),
expect.arrayContaining([
'git', ['checkout', 'abc', '--', 'xxxxx.test-with-snapshot.js.snapshot'],
'git', ['-C', expect.any(String), 'checkout', 'abc', '--', 'xxxxx.test-with-snapshot.js.snapshot'],
]),
]));
});
Expand Down Expand Up @@ -625,13 +625,13 @@ describe('IntegTest runIntegTests', () => {
expect(cdkMock.mocks.destroy).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(2);
expect(cdkMock.mocks.deploy).toHaveBeenCalledWith(expect.objectContaining({
app: 'node --no-warnings xxxxx.test-with-snapshot.js',
app: 'node --no-warnings test/test-data/xxxxx.test-with-snapshot.js',
}));
expect(cdkMock.mocks.synthFast).toHaveBeenCalledWith(expect.objectContaining({
execCmd: ['node', '--no-warnings', 'xxxxx.test-with-snapshot.js'],
execCmd: ['node', '--no-warnings', 'test/test-data/xxxxx.test-with-snapshot.js'],
}));
expect(cdkMock.mocks.destroy).toHaveBeenCalledWith(expect.objectContaining({
app: 'node --no-warnings xxxxx.test-with-snapshot.js',
app: 'node --no-warnings test/test-data/xxxxx.test-with-snapshot.js',
}));
});
});
Expand All @@ -655,7 +655,7 @@ describe('IntegTest watchIntegTest', () => {

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

// THEN
expect(cdkMock.mocks.watch).toHaveBeenCalledWith(expect.objectContaining({
app: 'node --no-warnings xxxxx.test-with-snapshot.js',
app: 'node --no-warnings test/test-data/xxxxx.test-with-snapshot.js',
hotswap: HotswapMode.FALL_BACK,
watch: true,
traceLogs: true,
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/integ-runner/test/runner/snapshot-test-runner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,13 @@ describe('IntegTest runSnapshotTests', () => {
}));
expect(cdkMock.mocks.synthFast).toHaveBeenCalledTimes(1);
expect(cdkMock.mocks.synthFast).toHaveBeenCalledWith({
execCmd: ['node', 'xxxxx.integ-test2.js'],
execCmd: ['node', 'test/test-data/xxxxx.integ-test2.js'],
env: expect.objectContaining({
CDK_INTEG_ACCOUNT: '12345678',
CDK_INTEG_REGION: 'test-region',
}),
context: expect.any(Object),
output: '../../does/not/exist',
output: 'does/not/exist',
});
});
});
Expand Down
Loading
Loading