Skip to content

Commit

Permalink
test: add initial watch mode test suite
Browse files Browse the repository at this point in the history
- test starting watch mode, changing a file, and adding a semantic error
  - put this in a separate file as it has its own complexity to deal with (see below)
    - initially put it inside `no-errors`, but it got pretty confusing; this structure is a good bit simpler
    - refactored this a couple times actually

- add two watch mode helpers
  - `watchBundle` is very similar to `genBundle` but with some watch mode nuances (like returning a watcher)
  - `watchEnd` is a bit complicated async wrapper around a listener interface to make imperative testing simpler
  - refactor: abstract out `createInput` and `createOutput` to be used in both `genBundle` and `watchBundle`
    - refactor: make sure `dist` output gets put into a temp test dir
      - the previous config made it output into rpt2's `dist` dir, since `cwd` is project root (when running tests from project root)
      - the Rollup API's `watch` func always writes out; can't just test in-memory like with `bundle.generate`
        - so the `dist` dir becomes relevant as such
      - refactor: pass in a temp `testDir` instead of the `cacheRoot`
        - we can derive the `cacheRoot` and the `dist` output from `testDir`
        - also improve test clean-up by removing `testDir` at the end, not just the `cacheRoot` dir inside it
        - `testDir` is the same var used in the unit tests to define a temp dir for testing

- change the `no-errors` fixture a tiny bit so that changing the import causes it to change too

- this ended up being fairly complex due to having to handle lots of async and parallelism
  - parallel testing means fixtures have to be immutable, but watch mode needs to modify files
    - ended up copying fixtures to a temp dir where I could modify them
  - async events are all over here
    - had to convert a callback listener interface to async too, which was pretty confusing
      - and make sure the listener and bundles got properly closed too so no leaky tests
    - apparently `expect.rejects.toThrow` can return a Promise too, so missed this error for a while
      - refactor: make sure the error spec awaits too (though I think the errors _happen_ to throw synchronously there)
  - and also found a number of bugs while attempting to test cache invalidation within watch mode
    - thought it was a natural place to test since watch mode testing needs to modify files anyway
    - had to trace a bunch to figure out why some code paths weren't being covered; will discuss this separately from this commit
  - testing Rollup watch within Jest watch also causes Jest to re-run a few times
    - I imagine double, overlapping watchers are confusing each other
    - might need to disable these tests when using Jest watch
  - high complexity async + parallel flows makes it pretty confusing to debug, so this code needs to be "handled with care"
    - also this high complexity was even harder to debug when I'm having migraines (which is most of the time, unfortunately)
      - hence why it took me a bit to finally make a PR for this small amount of code; the code was ok, the debugging was not too fun
  • Loading branch information
agilgur5 committed Jul 21, 2022
1 parent e5e3ccd commit 37f4247
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 24 deletions.
24 changes: 12 additions & 12 deletions __tests__/integration/errors.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,34 +11,34 @@ import * as helpers from "./helpers";
jest.setTimeout(15000);

const local = (x: string) => normalize(path.resolve(__dirname, x));
const cacheRoot = local("__temp/errors/rpt2-cache"); // don't use the one in node_modules
const testDir = local("__temp/errors");

afterAll(async () => {
// workaround: there seems to be some race condition causing fs.remove to fail, so give it a sec first (c.f. https://github.com/jprichardson/node-fs-extra/issues/532)
await new Promise(resolve => setTimeout(resolve, 1000));
await fs.remove(cacheRoot);
await fs.remove(testDir);
});

async function genBundle(relInput: string, extraOpts?: RPT2Options, onwarn?: Mock) {
const input = local(`fixtures/errors/${relInput}`);
return helpers.genBundle({
input,
tsconfig: local("fixtures/errors/tsconfig.json"),
cacheRoot,
testDir,
extraOpts: { include: [input], ...extraOpts }, // only include the input itself, not other error files (to only generate types and type-check the one file)
onwarn,
});
}

test("integration - tsconfig errors", async () => {
// TODO: move to parse-tsconfig unit tests?
expect(genBundle("semantic.ts", {
await expect(genBundle("semantic.ts", {
tsconfig: "non-existent-tsconfig",
})).rejects.toThrow("rpt2: failed to open 'non-existent-tsconfig'");
});

test("integration - semantic error", async () => {
expect(genBundle("semantic.ts")).rejects.toThrow("Type 'string' is not assignable to type 'number'.");
await expect(genBundle("semantic.ts")).rejects.toThrow("Type 'string' is not assignable to type 'number'.");
});

test("integration - semantic error - abortOnError: false / check: false", async () => {
Expand All @@ -55,21 +55,21 @@ test("integration - semantic error - abortOnError: false / check: false", async
expect(onwarn).toBeCalledTimes(1);
});

test("integration - syntax error", () => {
expect(genBundle("syntax.ts")).rejects.toThrow("';' expected.");
test("integration - syntax error", async () => {
await expect(genBundle("syntax.ts")).rejects.toThrow("';' expected.");
});

test("integration - syntax error - abortOnError: false / check: false", () => {
test("integration - syntax error - abortOnError: false / check: false", async () => {
const onwarn = jest.fn();
const err = "Unexpected token (Note that you need plugins to import files that are not JavaScript)";
expect(genBundle("syntax.ts", { abortOnError: false }, onwarn)).rejects.toThrow(err);
expect(genBundle("syntax.ts", { check: false }, onwarn)).rejects.toThrow(err);
await expect(genBundle("syntax.ts", { abortOnError: false }, onwarn)).rejects.toThrow(err);
await expect(genBundle("syntax.ts", { check: false }, onwarn)).rejects.toThrow(err);
});

const typeOnlyIncludes = ["**/import-type-error.ts", "**/type-only-import-with-error.ts"];

test("integration - type-only import error", () => {
expect(genBundle("import-type-error.ts", {
test("integration - type-only import error", async () => {
await expect(genBundle("import-type-error.ts", {
include: typeOnlyIncludes,
})).rejects.toThrow("Property 'nonexistent' does not exist on type 'someObj'.");
});
Expand Down
3 changes: 3 additions & 0 deletions __tests__/integration/fixtures/no-errors/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ export function sum(a: number, b: number) {
return a + b;
}

import { difference } from "./some-import";
export const diff2 = difference; // add an alias so that this file has to change when the import does (to help test cache invalidation etc)

export { difference } from "./some-import"
export type { num } from "./type-only-import"

Expand Down
51 changes: 42 additions & 9 deletions __tests__/integration/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,31 +1,39 @@
import { rollup, RollupOptions, OutputAsset } from "rollup";
import { rollup, watch, RollupOptions, OutputOptions, OutputAsset, RollupWatcher } from "rollup";
import * as path from "path";

import rpt2, { RPT2Options } from "../../src/index";

type Params = {
input: string,
tsconfig: string,
cacheRoot: string,
testDir: string,
extraOpts?: RPT2Options,
onwarn?: RollupOptions['onwarn'],
};

export async function genBundle ({ input, tsconfig, cacheRoot, extraOpts, onwarn }: Params) {
const bundle = await rollup({
function createInput ({ input, tsconfig, testDir, extraOpts, onwarn }: Params) {
return {
input,
plugins: [rpt2({
tsconfig,
cacheRoot,
cacheRoot: `${testDir}/rpt2-cache`, // don't use the one in node_modules
...extraOpts,
})],
onwarn,
});
}
}

const esm = await bundle.generate({
file: "./dist/index.js",
function createOutput (testDir: string): OutputOptions {
return {
file: path.resolve(`${testDir}/dist/index.js`), // put outputs in temp test dir
format: "esm",
exports: "named",
});
}
}

export async function genBundle (inputArgs: Params) {
const bundle = await rollup(createInput(inputArgs));
const esm = await bundle.generate(createOutput(inputArgs.testDir));

// Rollup has some deprecated properties like `get isAsset`, so enumerating them with, e.g. `.toEqual`, causes a bunch of warnings to be output
// delete the `isAsset` property for (much) cleaner logs
Expand All @@ -39,3 +47,28 @@ export async function genBundle ({ input, tsconfig, cacheRoot, extraOpts, onwarn

return esm;
}

/** wrap Listener interface in a Promise */
export function watchEnd (watcher: RollupWatcher) {
return new Promise<void>((resolve, reject) => {
watcher.on("event", event => {
if ("result" in event)
event.result?.close(); // close all bundles

if (event.code === "END")
resolve();
else if (event.code === "ERROR")
reject(event.error);
});
});
}

export async function watchBundle (inputArgs: Params) {
const watcher = watch({
...createInput(inputArgs),
output: createOutput(inputArgs.testDir),
});

await watchEnd(watcher); // wait for build to end before returning, similar to genBundle
return watcher;
}
6 changes: 3 additions & 3 deletions __tests__/integration/no-errors.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ import * as helpers from "./helpers";
jest.setTimeout(15000);

const local = (x: string) => path.resolve(__dirname, x);
const cacheRoot = local("__temp/no-errors/rpt2-cache"); // don't use the one in node_modules
const testDir = local("__temp/no-errors");

afterAll(() => fs.remove(cacheRoot));
afterAll(() => fs.remove(testDir));

async function genBundle(relInput: string, extraOpts?: RPT2Options) {
return helpers.genBundle({
input: local(`fixtures/no-errors/${relInput}`),
tsconfig: local("fixtures/no-errors/tsconfig.json"),
cacheRoot,
testDir,
extraOpts,
});
}
Expand Down
79 changes: 79 additions & 0 deletions __tests__/integration/watch.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { jest, beforeAll, afterAll, test, expect } from "@jest/globals";
import * as path from "path";
import * as fs from "fs-extra";

import { RPT2Options } from "../../src/index";
import * as helpers from "./helpers";

// increase timeout to 15s for whole file since CI occassionally timed out -- these are integration and cache tests, so longer timeout is warranted
jest.setTimeout(15000);

const local = (x: string) => path.resolve(__dirname, x);
const testDir = local("__temp/watch");
const fixtureDir = `${testDir}/fixtures`;

beforeAll(async () => {
await fs.ensureDir(fixtureDir);
// copy the dir to not interfere with other parallel tests since we need to change files for watch mode
// note we're copying the root fixture dir bc we need the _base_ tsconfig too. maybe optimize in the future or use the other fixtures?
await fs.copy(local("fixtures"), fixtureDir);
});
afterAll(() => fs.remove(testDir));

async function watchBundle(input: string, extraOpts?: RPT2Options) {
return helpers.watchBundle({
input,
tsconfig: `${path.dirname(input)}/tsconfig.json`, // use the tsconfig of whatever fixture we're in
testDir,
extraOpts,
});
}

test("integration - watch", async () => {
const srcPath = `${fixtureDir}/no-errors/index.ts`;
const importPath = `${fixtureDir}/no-errors/some-import.ts`;
const distDir = `${testDir}/dist`;
const distPath = `${testDir}/dist/index.js`;
const decPath = `${distDir}/index.d.ts`;
const decMapPath = `${decPath}.map`;
const filesArr = [
"index.js",
"index.d.ts",
"index.d.ts.map",
"some-import.d.ts",
"some-import.d.ts.map",
"type-only-import.d.ts",
"type-only-import.d.ts.map",
];

const watcher = await watchBundle(srcPath);

const files = await fs.readdir(distDir);
expect(files).toEqual(expect.arrayContaining(filesArr));
expect(files.length).toBe(filesArr.length); // no other files

// save content to test against later
const dist = await fs.readFile(distPath, "utf8");
const dec = await fs.readFile(decPath, "utf8");
const decMap = await fs.readFile(decMapPath, "utf8");

// modify an imported file -- this should cause it and index to change
await fs.writeFile(importPath, "export const difference = 2", "utf8");
await helpers.watchEnd(watcher);

// should have same structure, since names haven't changed and dist hasn't been cleaned
const files2 = await fs.readdir(distDir);
expect(files2).toEqual(expect.arrayContaining(filesArr));
expect(files2.length).toBe(filesArr.length); // no other files

// should have different content now though
expect(dist).not.toEqual(await fs.readFile(distPath, "utf8"));
expect(dec).not.toEqual(await fs.readFile(decPath, "utf8"));
expect(decMap).not.toEqual(await fs.readFile(decMapPath, "utf8"));

// modify an imported file to cause a semantic error
await fs.writeFile(importPath, "export const difference = nonexistent", "utf8")
await expect(helpers.watchEnd(watcher)).rejects.toThrow("Cannot find name 'nonexistent'.");

await watcher.close();
});

0 comments on commit 37f4247

Please sign in to comment.