From 75e04a6b13c025f6583dfdc4a92d02a7560299df Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sat, 23 Jul 2022 17:17:40 -0400 Subject: [PATCH 1/2] refactor(test): heavily simplify the `context` helper - since we're type-casting it anyway, we can heavily simplify this and remove the stubs entirely - they're actually unused in the unit tests, so we don't need them at all - besides the type-checking, which we force with a cast anyway - the `as unknown as` is bad practice, and probably why I didn't use it initially (plus other typing issues), but it's much simpler this way and reflects the intent better -- just making it type-check with the few properties we use - we can also use Jest mocks directly instead of the hacky `contextualLogger` and passing `data` in - `makeContext` now creates the mocks, so we just need to check against `context.error` etc - this is _much_ more familiar as it's what we use in the source and follows idiomatic Jest - rewrite all the checks to test against the mocks instead - I thought this felt too complicated / verbose before, but I had left this as is from brekk's initial test structure - now that I understand all the tests and test intent much better, I could rewrite this to be a good bit simpler - make the `toBeFalsy()` checks more precise by checking that the mock _wasn't_ called - it returns `void` anyway, so `toBeFalsy()` _always_ returns true; it's not much of a test - checking that the low verbosity level didn't trigger the mock to be called actually checks the test's intent --- __tests__/context.spec.ts | 51 ++++++++++++------------ __tests__/fixtures/context.ts | 52 ++++--------------------- __tests__/get-options-overrides.spec.ts | 26 ++++--------- __tests__/parse-tsconfig.spec.ts | 15 ++++--- 4 files changed, 48 insertions(+), 96 deletions(-) diff --git a/__tests__/context.spec.ts b/__tests__/context.spec.ts index e862af8f..f2fc0da0 100644 --- a/__tests__/context.spec.ts +++ b/__tests__/context.spec.ts @@ -1,6 +1,6 @@ import { jest, test, expect } from "@jest/globals"; -import { makeStubbedContext } from "./fixtures/context"; +import { makeContext } from "./fixtures/context"; import { ConsoleContext, RollupContext } from "../src/context"; (global as any).console = { @@ -54,21 +54,20 @@ test("ConsoleContext 0 verbosity", () => { }); test("RollupContext", () => { - const data = {}; - const stubbedContext = makeStubbedContext(data); - const context = new RollupContext(5, false, stubbedContext); + const innerContext = makeContext(); + const context = new RollupContext(5, false, innerContext); context.warn("test"); - expect((data as any).warn).toEqual("test"); + expect(innerContext.warn).toHaveBeenLastCalledWith("test"); context.warn(() => "test2"); - expect((data as any).warn).toEqual("test2"); + expect(innerContext.warn).toHaveBeenLastCalledWith("test2"); context.error("test!"); - expect((data as any).warn).toEqual("test!"); + expect(innerContext.warn).toHaveBeenLastCalledWith("test!"); context.error(() => "test2!"); - expect((data as any).warn).toEqual("test2!"); + expect(innerContext.warn).toHaveBeenLastCalledWith("test2!"); context.info("test3"); expect(console.log).toHaveBeenLastCalledWith("test3"); @@ -84,29 +83,31 @@ test("RollupContext", () => { }); test("RollupContext with 0 verbosity", () => { - const data = {}; - const stubbedContext = makeStubbedContext(data); - const context = new RollupContext(0, false, stubbedContext); - - expect(context.debug("verbosity is too low here")).toBeFalsy(); - expect(context.info("verbosity is too low here")).toBeFalsy(); - expect(context.warn("verbosity is too low here")).toBeFalsy(); + const innerContext = makeContext(); + const context = new RollupContext(0, false, innerContext); + + context.debug("verbosity is too low here"); + expect(innerContext.debug).not.toBeCalled(); + context.info("verbosity is too low here"); + expect(innerContext.debug).not.toBeCalled(); + context.warn("verbosity is too low here") + expect(innerContext.warn).not.toBeCalled(); }); test("RollupContext.error + debug negative verbosity", () => { - const data = {}; - const stubbedContext = makeStubbedContext(data); - const context = new RollupContext(-100, true, stubbedContext); + const innerContext = makeContext(); + const context = new RollupContext(-100, true, innerContext); - expect(context.error("whatever")).toBeFalsy(); - expect(context.debug("whatever")).toBeFalsy(); + context.error("verbosity is too low here"); + expect(innerContext.error).not.toBeCalled(); + context.debug("verbosity is too low here"); + expect(innerContext.debug).not.toBeCalled(); }); test("RollupContext.error with bail", () => { - const data = {}; - const stubbedContext = makeStubbedContext(data); - const context = new RollupContext(5, true, stubbedContext); + const innerContext = makeContext(); + const context = new RollupContext(5, true, innerContext); - expect(context.error("whatever")).toBeFalsy(); - expect((data as any).error).toEqual("whatever"); + context.error("bail"); + expect(innerContext.error).toHaveBeenLastCalledWith("bail"); }); diff --git a/__tests__/fixtures/context.ts b/__tests__/fixtures/context.ts index a00d0d13..cd1501d5 100644 --- a/__tests__/fixtures/context.ts +++ b/__tests__/fixtures/context.ts @@ -1,51 +1,13 @@ +import { jest } from "@jest/globals"; import { PluginContext } from "rollup"; import { IContext } from "../../src/context"; -const stub = (x: any) => x; - -const contextualLogger = (data: any): IContext => { - return { - warn: (x: any) => { - data.warn = x; - }, - error: (x: any) => { - data.error = x; - }, - info: (x: any) => { - data.info = x; - }, - debug: (x: any) => { - data.debug = x; - }, - }; -}; - -export function makeStubbedContext (data: any): PluginContext & IContext { - const { warn, error, info, debug } = contextualLogger(data); +export function makeContext(): PluginContext & IContext { return { - addWatchFile: stub as any, - getWatchFiles: stub as any, - cache: stub as any, - load: stub as any, - resolve: stub as any, - resolveId: stub as any, - isExternal: stub as any, - meta: stub as any, - emitAsset: stub as any, - emitChunk: stub as any, - emitFile: stub as any, - setAssetSource: stub as any, - getAssetFileName: stub as any, - getChunkFileName: stub as any, - getFileName: stub as any, - parse: stub as any, - warn: warn as any, - error: error as any, - info: info as any, - debug: debug as any, - moduleIds: stub as any, - getModuleIds: stub as any, - getModuleInfo: stub as any - }; + error: jest.fn(), + warn: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + } as unknown as PluginContext & IContext; }; diff --git a/__tests__/get-options-overrides.spec.ts b/__tests__/get-options-overrides.spec.ts index cdc141e5..51314e96 100644 --- a/__tests__/get-options-overrides.spec.ts +++ b/__tests__/get-options-overrides.spec.ts @@ -1,11 +1,11 @@ -import { afterAll, test, expect, jest } from "@jest/globals"; +import { afterAll, test, expect } from "@jest/globals"; import * as path from "path"; import * as ts from "typescript"; import { normalizePath as normalize } from "@rollup/pluginutils"; import { remove } from "fs-extra"; import { makeOptions } from "./fixtures/options"; -import { makeStubbedContext } from "./fixtures/context"; +import { makeContext } from "./fixtures/context"; import { getOptionsOverrides, createFilter } from "../src/get-options-overrides"; const local = (x: string) => normalize(path.resolve(__dirname, x)); @@ -100,9 +100,7 @@ test("getOptionsOverrides - with sourceMap", () => { test("createFilter", () => { const config = { ...defaultConfig }; const preParsedTsConfig = { ...defaultPreParsedTsConfig }; - - const stubbedContext = makeStubbedContext({}); - const filter = createFilter(stubbedContext, config, preParsedTsConfig); + const filter = createFilter(makeContext(), config, preParsedTsConfig); expect(filter(filtPath("src/test.ts"))).toBe(true); expect(filter(filtPath("src/test.js"))).toBe(false); @@ -112,14 +110,10 @@ test("createFilter", () => { test("createFilter - context.debug", () => { const config = { ...defaultConfig }; const preParsedTsConfig = { ...defaultPreParsedTsConfig }; + const context = makeContext(); + createFilter(context, config, preParsedTsConfig); - // test context.debug() statements - const debug = jest.fn(x => x()); - const data = { set debug(x: any) { debug(x) } }; - const stubbedContext = makeStubbedContext(data); - - createFilter(stubbedContext, config, preParsedTsConfig); - expect(debug.mock.calls.length).toBe(2); + expect(context.debug).toHaveBeenCalledTimes(2); }); test("createFilter - rootDirs", () => { @@ -130,9 +124,7 @@ test("createFilter - rootDirs", () => { rootDirs: ["src", "lib"] }, }; - - const stubbedContext = makeStubbedContext({}); - const filter = createFilter(stubbedContext, config, preParsedTsConfig); + const filter = createFilter(makeContext(), config, preParsedTsConfig); expect(filter(filtPath("src/test.ts"))).toBe(true); expect(filter(filtPath("src/test.js"))).toBe(false); @@ -155,9 +147,7 @@ test("createFilter - projectReferences", () => { { path: "lib" }, ], }; - - const stubbedContext = makeStubbedContext({}); - const filter = createFilter(stubbedContext, config, preParsedTsConfig); + const filter = createFilter(makeContext(), config, preParsedTsConfig); expect(filter(filtPath("src/test.ts"))).toBe(true); expect(filter(filtPath("src/test.js"))).toBe(false); diff --git a/__tests__/parse-tsconfig.spec.ts b/__tests__/parse-tsconfig.spec.ts index 1f2f7754..b7014709 100644 --- a/__tests__/parse-tsconfig.spec.ts +++ b/__tests__/parse-tsconfig.spec.ts @@ -3,33 +3,32 @@ import * as path from "path"; import { normalizePath as normalize } from "@rollup/pluginutils"; import { makeOptions } from "./fixtures/options"; -import { makeStubbedContext } from "./fixtures/context"; +import { makeContext } from "./fixtures/context"; import { parseTsConfig } from "../src/parse-tsconfig"; const local = (x: string) => normalize(path.resolve(__dirname, x)); const defaultOpts = makeOptions("", ""); -const stubbedContext = makeStubbedContext({}); test("parseTsConfig", () => { - expect(() => parseTsConfig(stubbedContext, defaultOpts)).not.toThrow(); + expect(() => parseTsConfig(makeContext(), defaultOpts)).not.toThrow(); }); test("parseTsConfig - tsconfig errors", () => { - const data = { error: "" }; + const context = makeContext(); // should not throw when the tsconfig is buggy, but should still print an error (below) - expect(() => parseTsConfig(makeStubbedContext(data), { + expect(() => parseTsConfig(context, { ...defaultOpts, tsconfigOverride: { include: "should-be-an-array", }, })).not.toThrow(); - expect(data.error).toMatch("Compiler option 'include' requires a value of type Array"); + expect(context.error).toHaveBeenLastCalledWith(expect.stringContaining("Compiler option 'include' requires a value of type Array")); }); test("parseTsConfig - failed to open", () => { - expect(() => parseTsConfig(stubbedContext, { + expect(() => parseTsConfig(makeContext(), { ...defaultOpts, tsconfig: "non-existent-tsconfig", })).toThrow("rpt2: failed to open 'non-existent-tsconfig'"); @@ -38,7 +37,7 @@ test("parseTsConfig - failed to open", () => { test("parseTsConfig - failed to parse", () => { const notTsConfigPath = local("fixtures/options.ts"); // a TS file should fail to parse - expect(() => parseTsConfig(stubbedContext, { + expect(() => parseTsConfig(makeContext(), { ...defaultOpts, tsconfig: notTsConfigPath, })).toThrow(`rpt2: failed to parse '${notTsConfigPath}'`); From 7e316b8e811990f1362233479dc18f213ab2ba5b Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Mon, 25 Jul 2022 20:59:09 -0400 Subject: [PATCH 2/2] make sure to check funcs in context calls too - `get-options-overrides`'s coverage func % decreased bc funcs passed to `context.debug` weren't being called - took me a bit to notice too since we have no coverage checks - and then another bit to realize _why_ it decreased --- __tests__/fixtures/context.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/__tests__/fixtures/context.ts b/__tests__/fixtures/context.ts index cd1501d5..239114c3 100644 --- a/__tests__/fixtures/context.ts +++ b/__tests__/fixtures/context.ts @@ -3,11 +3,19 @@ import { PluginContext } from "rollup"; import { IContext } from "../../src/context"; +// if given a function, make sure to call it (for code coverage etc) +function returnText (message: string | (() => string)) { + if (typeof message === "string") + return message; + + return message(); +} + export function makeContext(): PluginContext & IContext { return { - error: jest.fn(), - warn: jest.fn(), - info: jest.fn(), - debug: jest.fn(), + error: jest.fn(returnText), + warn: jest.fn(returnText), + info: jest.fn(returnText), + debug: jest.fn(returnText), } as unknown as PluginContext & IContext; };