Apply suggestions from code review

Co-Authored-By: Romain Marcadier-Muller <[email protected]>

buildspec.yaml

@@ -12,6 +12,7 @@ phases:
   build:
     commands:
       - yarn build && yarn test
+      - yarn test:integ
   post_build:
     commands:
       - '[ ${CODEBUILD_BUILD_SUCCEEDING} = 1 ] && yarn package'

packages/@jsii/integ-test/README.md

@@ -5,6 +5,6 @@ A suite of integration tests for JSII and related modules.
 ## Running
 
 Running the integration tests locally requires a github access token. Copy the
-.env.example file and replace the dummy value with a personal access token.
+`.env.example` file and replace the dummy value with a personal access token.
 
 then run `yarn run test:integ`

packages/@jsii/integ-test/package.json

@@ -1,23 +1,26 @@
 {
-  "name": "integ-test",
-  "version": "1.0.0",
-  "description": "",
-  "main": "index.js",
+  "name": "@jsii/integ-test",
+  "version": "0.21.2",
+  "description": "A suite of integration tests for JSII and related modules.",
   "private": true,
   "scripts": {
-    "build": "tsc",
+    "build": "tsc --build",
     "test:integ": "jest"
   },
   "keywords": [],
-  "author": "",
-  "license": "ISC",
-  "dependencies": {
+  "author": {
+    "name": "Amazon Web Services",
+    "url": "https://aws.amazon.com"
+  },
+  "license": "Apache-2.0",
+  "devDependencies": {
     "@octokit/rest": "^16.36.0",
     "dotenv": "^8.2.0",
+    "fs-extra": "^8.1.0",
     "jest": "^25.1.0",
     "jsii": "^0.21.2",
     "jsii-pacmak": "^0.21.2",
-    "typescript": "^3.7.5"
+    "typescript": "~3.7.5"
   },
   "jest": {
     "errorOnDeprecated": true,

packages/@jsii/integ-test/test/build-cdk.test.ts

@@ -1,15 +1,12 @@
-// import { IncomingMessage } from 'http';
-import * as fs from 'fs';
+import { mkdtemp, remove } from 'fs-extra';
 import * as path from 'path';
 import * as Octokit from '@octokit/rest';
-import { downloadReleaseAsset, minutes, ProcessManager, rmdirRecursive } from '../utils';
+import { downloadReleaseAsset, minutes, ProcessManager, writeFileStream } from '../utils';
 import * as dotenv from 'dotenv';
 
-const { mkdtemp } = fs.promises;
-
 dotenv.config();
-const JSII_DIR = path.resolve(require.resolve('jsii'), '..', '..');
-const JSII_PACMAK_DIR = path.resolve(require.resolve('jsii-pacmak'), '..', '..');
+const JSII_DIR = path.dirname(require.resolve('jsii/package.json'));
+const JSII_PACMAK_DIR = path.dirname(require.resolve('jsii-pacmak/package.json'));
 
 const octokit = new Octokit({
   auth: process.env.GITHUB_TOKEN
@@ -26,10 +23,10 @@ describe('Build CDK', () => {
 
   afterAll(async () => {
     await processes.killAll();
-    await rmdirRecursive(buildDir);
+    await remove(buildDir);
   });
 
-  test('can build latest cdk release', async (done) => {
+  test('can build latest cdk release', async () => {
     // download latest release info
     console.time('cdkbuild');
     const release = await octokit.repos.getLatestRelease({
@@ -41,16 +38,11 @@ describe('Build CDK', () => {
     const fileName = 'cdk.tar.gz';
     const tarFile = path.join(buildDir, fileName);
     const code = await downloadReleaseAsset(`https://api.github.com/repos/aws/aws-cdk/tarball/${release.data.tag_name}`);
-    const codeStream = fs.createWriteStream(tarFile);
 
-    // save to file and wait to finish
-    code.pipe(codeStream);
-    await new Promise(resolve => codeStream.on('close', () => {
-      resolve();
-    }));
+    await writeFileStream(code, tarFile);
 
     // unzip tar archive
-    await processes.spawn('tar', ['-xzvf', fileName], {
+    await processes.spawn('tar', ['-xzf', fileName], {
       cwd: buildDir
     });
 
@@ -70,12 +62,11 @@ describe('Build CDK', () => {
     await processes.spawn('ln', ['-s', JSII_PACMAK_DIR, './node_modules'], { cwd: srcDir });
 
     // build cdk
-    await processes.spawn('./node_modules/.bin/lerna', ['run', 'build', '--stream'], { cwd: srcDir });
+    await processes.spawn('npx', ['lerna', 'run', 'build', '--stream'], { cwd: srcDir });
 
     // package modules
     await processes.spawn('yarn', ['run', 'pack'], { cwd: srcDir });
-    console.timeEnd('cdkbuild');
-    done();
 
+    console.timeEnd('cdkbuild');
   }, minutes(60));
 });

packages/@jsii/integ-test/tsconfig.json

@@ -0,0 +1,8 @@
+{
+  "compilerOptions": {
+    "composite": false,
+    "declaration": false
+  },
+  "extends": "../../../tsconfig-base",
+  "include": ["**/*.ts"]
+}

packages/@jsii/integ-test/utils/index.ts

@@ -1,84 +1,78 @@
+import { Readable } from 'stream';
+import { createWriteStream } from 'fs';
 import * as cp from 'child_process';
 import * as https from 'https';
-import * as path from 'path';
-import { PathLike, promises as fs } from 'fs';
-
-export const minutes = (num: number) => num * 1000 * 60
+import { IncomingMessage } from 'http';
 
 /**
- * rmdirRecursive
+ * @param num a quantity of minutes (could be fractional)
  *
- * recursive directory removal for cleanup after build test. Node10 fs module
- * doesn't support the `recursive` option
+ * @return equivalent number of milliseconds
  */
-export const rmdirRecursive = async (dir: PathLike) => {
-  const contents = await fs.readdir(dir);
-  await Promise.all(contents.map(async (file) => {
-    const currentPath = path.join(dir.toString(), file);
-    if ((await fs.lstat(currentPath)).isDirectory()) {
-      await rmdirRecursive(currentPath);
-    } else {
-      await fs.unlink(currentPath);
-    }
-  }));
-
-  await fs.rmdir(dir);
-};
+export function minutes(num: number): number {
+  return num * 1000 * 60;
+}
 
-/*
- * ProcessManager
- *
+/**
  * Used to track and clean up processes if tests fail or timeout
  */
 export class ProcessManager {
-  processes: {
+  private readonly processes: {
     [pid: string]: {
       proc: cp.ChildProcess,
       promise: Promise<void>
     }
-  };
-
-  constructor() {
-    this.processes = {};
-  }
-
-  async killAll() {
+  } = {};
+
+  /**
+   * kill all still running processes
+   *
+   * @param [signal] - signal sent to terminate process
+   */
+  async killAll(signal?: string) {
     const values = Object.values(this.processes);
-    values.forEach(procObj => procObj.proc.kill());
-    await Promise.all(values.map(proc => proc.promise));
-    this.processes = {};
+    await Promise.all(values.map(({ proc, promise }) => async() => {
+      proc.kill(signal);
+      await promise;
+      this.remove(proc);
+    }));
   }
 
   private add(proc: cp.ChildProcess, promise: Promise<void>) {
-    const { pid } = proc;
-    this.processes[pid] = { proc, promise };
+    this.processes[proc.pid] = { proc, promise };
   }
 
   private remove(proc: cp.ChildProcess) {
     delete this.processes[proc.pid];
   }
 
-  spawn(cmd: string, args: string[], opts: any) {
-    const proc = cp.spawn(cmd, args, opts);
-    proc.stdout.pipe(process.stdout);
-    proc.stderr.pipe(process.stderr);
-
-    const promise: Promise<void> = new Promise((resolve, reject) => {
-      proc.on('exit', code => {
+  /**
+   * spawn new child process
+   *
+   * @param shell command being called
+   * @param arguments passed to command
+   * @param options passed to child process spawn
+   */
+  spawn(cmd: string, args: string[], opts: any = {}): Promise<void> {
+    const proc = cp.spawn(cmd, args, { stdio: 'inherit', ...opts });
+
+    const promise = new Promise<void>((ok, ko) => {
+      proc.once('exit', code => {
         const message = `child process exited with code: ${code}`;
-        if (code !== 0) {
-          process.stderr.write(message);
-          reject(new Error(message));
-        } else {
+        if (code === 0) {
           process.stdout.write(message);
-          resolve();
+          ok();
+        } else {
+          process.stderr.write(message);
+          ko(new Error(message));
         }
 
         this.remove(proc);
       });
 
-      proc.on('error', error => {
+      proc.once('error', error => {
         process.stderr.write(`Process ${proc.pid} error: ${error}`);
+        ko();
       });
     });
 
@@ -88,31 +82,53 @@ export class ProcessManager {
 }
 
 /**
- * downloadReleaseAsset
+ * write downloaded asset to file
  *
+ * @param source stream
+ * @param destination of saved file
+ */
+export function writeFileStream(source: Readable, destination: string) {
+  return new Promise((ok, ko) => {
+    const destStream = createWriteStream(destination);
+    destStream.once('close', ok);
+    destStream.once('error', ko);
+    source.once('error', ko);
+
+    source.pipe(destStream);
+  });
+}
+
+/**
  * Wrap http calls to download release asset in a promise. Github responds with
  * a 302 sometimes which is required to be handled. Returns the buffer to be
  * streamed to destination fs stream.
+ *
+ * @param url of downloadable asset
+ *
+ * @returns readable stream of asset data
  */
-export const downloadReleaseAsset = (url: string): Promise<any> => new Promise((resolve, reject) => {
-  const config = {
-    headers: {
-      'User-Agent': 'aws-cdk',
-      Authorization: `token ${process.env.GITHUB_TOKEN}`,
-      Accept: 'application/json'
-    }
-  };
+export function downloadReleaseAsset(url: string): Promise<Readable> {
+  return new Promise((ok, ko) => {
+    const config = {
+      headers: {
+        'User-Agent': '@jsii/integ-test',
+        Authorization: `token ${process.env.GITHUB_TOKEN}`
+      }
+    };
+
+    https.get(url, config, (response: IncomingMessage) => {
+      if (response.statusCode === 302) {
+
+        if (!response.headers.location) {
+          throw new Error('Bad redirect, no location header found');
+        }
 
-  https.get(url, config, response => {
-    if (response.statusCode! < 200 && response.statusCode !== 302) {
-      reject(new Error(`Status Code: ${response.statusCode}`));
-    }
-    if (response.statusCode === 302 && response.headers.location) {
-      return https.get(response.headers.location, config, response => {
-        return resolve(response);
-      });
-    }
+        return https.get(response.headers.location, config, ok);
+      } else if (response.statusCode && (response.statusCode < 200 || response.statusCode > 300)) {
+        return ko(new Error(`Status Code: ${response.statusCode}`));
+      }
 
-    return resolve(response);
+      return ok(response);
+    });
   });
-});
+};

yarn.lock

@@ -8491,7 +8491,7 @@ typescript-json-schema@^0.42.0:
     typescript "^3.5.3"
     yargs "^14.0.0"
 
-typescript@^3.5.3, typescript@^3.7.5, typescript@~3.7.5:
+typescript@^3.5.3, typescript@~3.7.5:
   version "3.7.5"
   resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.7.5.tgz#0692e21f65fd4108b9330238aac11dd2e177a1ae"
   integrity sha512-/P5lkRXkWHNAbcJIiHPfRoKqyd7bsyCma1hZNUGfn20qm64T6ZBlrzprymeu918H+mB/0rIg2gGK/BXkhhYgBw==