Skip to content

Commit c5f895e

Browse files
authored
chore(cli): prevent test interference (#32270)
Our CLI unit tests were interfering with each other because they were writing files from and to the current directory, which is shared between all of them. Solve it by making a non-writeable directory before running the tests, so that the tests that do that start throwing errors and we can identify them. Then fix those. I tried papering over the issue by trying to create tempdirs automatically, but that started to introduce all kinds of errors in tests that were already doing tempdir management themselves, and it took too long to go and figure out what was wrong there, so I'm doing this instead. Also in this PR: - Switch off global `silent` mode for the tests. It's very annoying if `console.log` statements don't make it out when you're debugging. - This PR caused a couple of lines in `init.ts` to be useless (they were there for tests), and removing those causes a lack of coverage in `init.ts`'s entirety, so add some tests for that file. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 421d327 commit c5f895e

File tree

6 files changed

+143
-23
lines changed

6 files changed

+143
-23
lines changed

packages/aws-cdk/jest.config.js

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,5 @@ module.exports = {
2020

2121
// We have many tests here that commonly time out
2222
testTimeout: 30_000,
23-
// These tests are too chatty. Shush.
24-
silent: true,
23+
setupFilesAfterEnv: ["<rootDir>/test/jest-setup-after-env.ts"],
2524
};

packages/aws-cdk/lib/init.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -324,13 +324,9 @@ async function initializeProject(
324324
if (migrate) {
325325
await template.addMigrateContext(workDir);
326326
}
327-
if (await fs.pathExists('README.md')) {
328-
const readme = await fs.readFile('README.md', { encoding: 'utf-8' });
329-
// Save the logs!
330-
// Without this statement, the readme of the CLI is printed in every init test
331-
if (!readme.startsWith('# AWS CDK Toolkit')) {
332-
print(chalk.green(readme));
333-
}
327+
if (await fs.pathExists(`${workDir}/README.md`)) {
328+
const readme = await fs.readFile(`${workDir}/README.md`, { encoding: 'utf-8' });
329+
print(chalk.green(readme));
334330
}
335331

336332
if (!generateOnly) {

packages/aws-cdk/test/diff.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
import * as fs from 'fs';
2+
import * as os from 'os';
3+
import * as path from 'path';
24
import { Writable } from 'stream';
35
import { StringDecoder } from 'string_decoder';
46
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
@@ -12,6 +14,22 @@ import { CdkToolkit } from '../lib/cdk-toolkit';
1214
let cloudExecutable: MockCloudExecutable;
1315
let cloudFormation: jest.Mocked<Deployments>;
1416
let toolkit: CdkToolkit;
17+
let oldDir: string;
18+
let tmpDir: string;
19+
20+
beforeAll(() => {
21+
// The toolkit writes and checks for temporary files in the current directory,
22+
// so run these tests in a tempdir so they don't interfere with each other
23+
// and other tests.
24+
oldDir = process.cwd();
25+
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'aws-cdk-test'));
26+
process.chdir(tmpDir);
27+
});
28+
29+
afterAll(() => {
30+
process.chdir(oldDir);
31+
fs.rmSync(tmpDir, { recursive: true, force: true });
32+
});
1533

1634
describe('fixed template', () => {
1735
const templatePath = 'oldTemplate.json';

packages/aws-cdk/test/init.test.ts

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import * as os from 'os';
22
import * as path from 'path';
33
import * as cxapi from '@aws-cdk/cx-api';
44
import * as fs from 'fs-extra';
5-
import { availableInitTemplates, cliInit } from '../lib/init';
5+
import { availableInitLanguages, availableInitTemplates, cliInit, printAvailableTemplates } from '../lib/init';
66

77
describe('constructs version', () => {
88
cliTest('create a TypeScript library project', async (workDir) => {
@@ -17,6 +17,21 @@ describe('constructs version', () => {
1717
expect(await fs.pathExists(path.join(workDir, 'lib'))).toBeTruthy();
1818
});
1919

20+
cliTest('asking for a nonexistent template fails', async (workDir) => {
21+
await expect(cliInit({
22+
type: 'banana',
23+
language: 'typescript',
24+
workDir,
25+
})).rejects.toThrow(/Unknown init template/);
26+
});
27+
28+
cliTest('asking for a template but no language prints and throws', async (workDir) => {
29+
await expect(cliInit({
30+
type: 'app',
31+
workDir,
32+
})).rejects.toThrow(/No language/);
33+
});
34+
2035
cliTest('create a TypeScript app project', async (workDir) => {
2136
await cliInit({
2237
type: 'app',
@@ -237,6 +252,16 @@ test('when no version number is present (e.g., local development), the v2 templa
237252
expect((await availableInitTemplates()).length).toBeGreaterThan(0);
238253
});
239254

255+
test('check available init languages', async () => {
256+
const langs = await availableInitLanguages();
257+
expect(langs.length).toBeGreaterThan(0);
258+
expect(langs).toContain('typescript');
259+
});
260+
261+
test('exercise printing available templates', async () => {
262+
await printAvailableTemplates();
263+
});
264+
240265
function cliTest(name: string, handler: (dir: string) => void | Promise<any>): void {
241266
test(name, () => withTempDir(handler));
242267
}
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
import * as fs from 'fs';
2+
import * as os from 'os';
3+
import * as path from 'path';
4+
import { isPromise } from 'util/types';
5+
6+
/**
7+
* Global test setup for Jest tests
8+
*
9+
* It's easy to accidentally write tests that interfere with each other by
10+
* writing files to disk in the "current directory". To prevent this, the global
11+
* test setup creates a directory in the temporary directory and chmods it to
12+
* being non-writable. That way, whenever a test tries to write to the current
13+
* directory, it will produce an error and we'll be able to find and fix the
14+
* test.
15+
*
16+
* If you see `EACCES: permission denied`, you have a test that creates files
17+
* in the current directory, and you should be sure to do it in a temporary
18+
* directory that you clean up afterwards.
19+
*
20+
* ## Alternate approach
21+
*
22+
* I tried an approach where I would automatically try to create and clean up
23+
* temp directories for every test, but it was introducing too many conflicts
24+
* with existing test behavior (around specific ordering of temp directory
25+
* creation and cleanup tasks that are already present) in many places that I
26+
* didn't want to go and chase down.
27+
*
28+
*/
29+
30+
let tmpDir: string;
31+
let oldDir: string;
32+
33+
beforeAll(() => {
34+
tmpDir = path.join(os.tmpdir(), 'cdk-nonwritable-on-purpose');
35+
fs.mkdirSync(tmpDir, { recursive: true });
36+
fs.chmodSync(tmpDir, 0o500);
37+
oldDir = process.cwd();
38+
process.chdir(tmpDir);
39+
tmpDir = process.cwd(); // This will have resolved symlinks
40+
});
41+
42+
const reverseAfterAll: Array<jest.ProvidesHookCallback> = [];
43+
44+
/**
45+
* We need a cleanup here
46+
*
47+
* 99% of the time, Jest runs the tests in a subprocess and this isn't
48+
* necessary because we would have `chdir`ed in the subprocess.
49+
*
50+
* But sometimes we ask Jest with `-i` to run the tests in the main process,
51+
* or if you only ask for a single test suite Jest runs the tests in the main
52+
* process, and then we `chdir`ed the main process away.
53+
*
54+
* Jest will then try to write the `coverage` directory to the readonly directory,
55+
* and fail. Chdir back to the original dir.
56+
*
57+
* If the test file has an `afterAll()` hook it installed as well, we need to run
58+
* it before our cleanup, otherwise the wrong thing will happen (by default,
59+
* all `afterAll()`s run in call order, but they should be run in reverse).
60+
*/
61+
afterAll(async () => {
62+
for (const aft of reverseAfterAll.reverse()) {
63+
await new Promise<void>((resolve, reject) => {
64+
const response = aft(resolve as any);
65+
if (isPromise(response)) {
66+
response.then(() => { return resolve(); }, reject);
67+
} else {
68+
resolve();
69+
}
70+
});
71+
}
72+
73+
// eslint-disable-next-line no-console
74+
process.stderr.write(`${process.cwd()}, ${tmpDir}\n`);
75+
if (process.cwd() === tmpDir) {
76+
// eslint-disable-next-line no-console
77+
process.stderr.write('chmod\n');
78+
process.chdir(oldDir);
79+
}
80+
});
81+
82+
// Patch afterAll to make later-provided afterAll's run before us (in reverse order even).
83+
afterAll = (after: jest.ProvidesHookCallback) => {
84+
reverseAfterAll.push(after);
85+
};

packages/aws-cdk/test/notices.test.ts

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -455,23 +455,20 @@ describe(CachedDataSource, () => {
455455
});
456456

457457
test('retrieves data from the delegate when the file cannot be read', async () => {
458-
const debugSpy = jest.spyOn(logging, 'debug');
458+
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cdk-test'));
459+
try {
460+
const debugSpy = jest.spyOn(logging, 'debug');
459461

460-
if (fs.existsSync('does-not-exist.json')) {
461-
fs.unlinkSync('does-not-exist.json');
462-
}
463-
464-
const dataSource = dataSourceWithDelegateReturning(freshData, 'does-not-exist.json');
462+
const dataSource = dataSourceWithDelegateReturning(freshData, `${tmpDir}/does-not-exist.json`);
465463

466-
const notices = await dataSource.fetch();
467-
468-
expect(notices).toEqual(freshData);
469-
expect(debugSpy).not.toHaveBeenCalled();
464+
const notices = await dataSource.fetch();
470465

471-
debugSpy.mockRestore();
466+
expect(notices).toEqual(freshData);
467+
expect(debugSpy).not.toHaveBeenCalled();
472468

473-
if (fs.existsSync('does-not-exist.json')) {
474-
fs.unlinkSync('does-not-exist.json');
469+
debugSpy.mockRestore();
470+
} finally {
471+
fs.rmSync(tmpDir, { recursive: true, force: true });
475472
}
476473
});
477474

0 commit comments

Comments
 (0)