Skip to content

Commit

Permalink
refactor(test): heavily simplify the context helper
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
agilgur5 committed Aug 24, 2022
1 parent bba2f19 commit 75e04a6
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 96 deletions.
51 changes: 26 additions & 25 deletions __tests__/context.spec.ts
Original file line number Diff line number Diff line change
@@ -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 = {
Expand Down Expand Up @@ -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");
Expand All @@ -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");
});
52 changes: 7 additions & 45 deletions __tests__/fixtures/context.ts
Original file line number Diff line number Diff line change
@@ -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;
};
26 changes: 8 additions & 18 deletions __tests__/get-options-overrides.spec.ts
Original file line number Diff line number Diff line change
@@ -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));
Expand Down Expand Up @@ -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);
Expand All @@ -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", () => {
Expand All @@ -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);
Expand All @@ -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);
Expand Down
15 changes: 7 additions & 8 deletions __tests__/parse-tsconfig.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'");
Expand All @@ -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}'`);
Expand Down

0 comments on commit 75e04a6

Please sign in to comment.