From 37f42478b8834c188daeb73ec360a41173f29a79 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sun, 10 Jul 2022 19:30:34 -0400 Subject: [PATCH] test: add initial watch mode test suite - 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 --- __tests__/integration/errors.spec.ts | 24 +++--- .../integration/fixtures/no-errors/index.ts | 3 + __tests__/integration/helpers.ts | 51 +++++++++--- __tests__/integration/no-errors.spec.ts | 6 +- __tests__/integration/watch.spec.ts | 79 +++++++++++++++++++ 5 files changed, 139 insertions(+), 24 deletions(-) create mode 100644 __tests__/integration/watch.spec.ts diff --git a/__tests__/integration/errors.spec.ts b/__tests__/integration/errors.spec.ts index f4be8b60..14b8bd9c 100644 --- a/__tests__/integration/errors.spec.ts +++ b/__tests__/integration/errors.spec.ts @@ -11,12 +11,12 @@ 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) { @@ -24,7 +24,7 @@ async function genBundle(relInput: string, extraOpts?: RPT2Options, onwarn?: Moc 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, }); @@ -32,13 +32,13 @@ async function genBundle(relInput: string, extraOpts?: RPT2Options, onwarn?: Moc 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 () => { @@ -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'."); }); diff --git a/__tests__/integration/fixtures/no-errors/index.ts b/__tests__/integration/fixtures/no-errors/index.ts index bd4654bc..9ef3266a 100644 --- a/__tests__/integration/fixtures/no-errors/index.ts +++ b/__tests__/integration/fixtures/no-errors/index.ts @@ -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" diff --git a/__tests__/integration/helpers.ts b/__tests__/integration/helpers.ts index bd72eacb..99b3c19f 100644 --- a/__tests__/integration/helpers.ts +++ b/__tests__/integration/helpers.ts @@ -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 @@ -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((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; +} diff --git a/__tests__/integration/no-errors.spec.ts b/__tests__/integration/no-errors.spec.ts index 283ab76f..a0c93f3b 100644 --- a/__tests__/integration/no-errors.spec.ts +++ b/__tests__/integration/no-errors.spec.ts @@ -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, }); } diff --git a/__tests__/integration/watch.spec.ts b/__tests__/integration/watch.spec.ts new file mode 100644 index 00000000..ce8ced6d --- /dev/null +++ b/__tests__/integration/watch.spec.ts @@ -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(); +});