From e0d6c57e29061eb261462352171e35c4bb8e4fe3 Mon Sep 17 00:00:00 2001 From: Huafu Gandon Date: Wed, 26 Sep 2018 09:04:38 +0200 Subject: [PATCH] fix(import): wrong error message when a module exists but fails The importer is responsible of importing and patching modules with configurable fallbacks. If one is missing, it goes to the next one. Previously if a module existed but was failing when loading it with `require`, it'd report as missing instead of failing. Now it bubbles the error correctly. --- src/config/config-set.ts | 2 +- src/types.ts | 7 --- src/util/importer.spec.ts | 83 ++++++++++++++++++++++++++++-------- src/util/importer.ts | 89 +++++++++++++++++++++++++++++++++------ src/util/messages.ts | 1 + 5 files changed, 143 insertions(+), 39 deletions(-) diff --git a/src/config/config-set.ts b/src/config/config-set.ts index 7642850e92..16cc661c61 100644 --- a/src/config/config-set.ts +++ b/src/config/config-set.ts @@ -354,7 +354,7 @@ export class ConfigSet { let hooksFile = process.env.TS_JEST_HOOKS if (hooksFile) { hooksFile = resolve(this.cwd, hooksFile) - return importer.tryThese(hooksFile) || {} + return importer.tryTheseOr(hooksFile, {}) } return {} } diff --git a/src/types.ts b/src/types.ts index 6feff23c1c..48c3c3f783 100644 --- a/src/types.ts +++ b/src/types.ts @@ -126,13 +126,6 @@ export interface CreateJestPresetOptions { */ export type ModulePatcher = (module: T) => T -/** - * @internal - */ -export interface TsJestImporter { - tryThese(moduleName: string, ...fallbacks: string[]): any -} - /** * Common TypeScript interfaces between versions. */ diff --git a/src/util/importer.spec.ts b/src/util/importer.spec.ts index 2b8faf8f44..9cfe9ac74a 100644 --- a/src/util/importer.spec.ts +++ b/src/util/importer.spec.ts @@ -3,22 +3,21 @@ import * as fakers from '../__helpers__/fakers' import { Importer, __requireModule } from './importer' -const requireModule = jest.fn( - mod => - mod in modules - ? modules[mod]() - : (() => { - const err: any = new Error(`Module not found: ${mod}.`) - err.code = 'MODULE_NOT_FOUND' - throw err - })(), -) -__requireModule(requireModule as any) +const moduleNotFound = (mod: string) => { + const err: any = new Error(`Module not found: ${mod}.`) + err.code = 'MODULE_NOT_FOUND' + throw err +} +const fakeFullPath = (mod: string) => `/root/${mod}.js` +const requireModule = jest.fn(mod => (mod in modules ? modules[mod]() : moduleNotFound(mod))) +const resolveModule = jest.fn(mod => (mod in modules ? fakeFullPath(mod) : moduleNotFound(mod))) +__requireModule(requireModule as any, resolveModule) let modules!: { [key: string]: () => any } beforeEach(() => { modules = {} requireModule.mockClear() + resolveModule.mockClear() }) describe('instance', () => { @@ -30,12 +29,60 @@ describe('instance', () => { }) }) -describe('tryTheese', () => { - it('tries until it find one not failing', () => { +describe('tryThese', () => { + it('should try until it finds one existing', () => { modules = { - success: () => 'success', + success: () => 'ok', } - expect(new Importer().tryThese('fail1', 'fail2', 'success')).toBe('success') + expect(new Importer().tryThese('missing1', 'missing2', 'success')).toMatchInlineSnapshot(` +Object { + "exists": true, + "exports": "ok", + "given": "success", + "path": "/root/success.js", +} +`) + }) + it('should return the error when one is failing', () => { + modules = { + fail1: () => { + throw new Error('foo') + }, + success: () => 'ok', + } + const res = new Importer().tryThese('missing1', 'fail1', 'success') + expect(res).toMatchObject({ + exists: true, + error: expect.any(Error), + given: 'fail1', + path: fakeFullPath('fail1'), + }) + expect(res).not.toHaveProperty('exports') + expect((res as any).error.message).toMatch(/\bfoo\b/) + }) +}) + +describe('tryTheseOr', () => { + it('should try until it find one not failing', () => { + expect(new Importer().tryTheseOr(['fail1', 'fail2', 'success'])).toBeUndefined() + expect(new Importer().tryTheseOr(['fail1', 'fail2', 'success'], 'foo')).toBe('foo') + modules = { + success: () => 'ok', + } + expect(new Importer().tryTheseOr(['fail1', 'fail2', 'success'])).toBe('ok') + modules.fail2 = () => { + throw new Error('foo') + } + expect(new Importer().tryTheseOr(['fail1', 'fail2', 'success'], 'bar', true)).toBe('bar') + }) + it('should fail if one is throwing', () => { + modules = { + success: () => 'ok', + fail2: () => { + throw new Error('foo') + }, + } + expect(() => new Importer().tryTheseOr(['fail1', 'fail2', 'success'], 'bar')).toThrow(/\bfoo\b/) }) }) @@ -49,10 +96,10 @@ describe('patcher', () => { foo: () => ({ foo: true }), bar: () => ({ bar: true }), } - expect(imp.tryThese('foo')).toEqual({ foo: true, p1: true, p2: true }) - expect(imp.tryThese('foo')).toEqual({ foo: true, p1: true, p2: true }) + expect(imp.tryTheseOr('foo')).toEqual({ foo: true, p1: true, p2: true }) + expect(imp.tryTheseOr('foo')).toEqual({ foo: true, p1: true, p2: true }) - expect(imp.tryThese('bar')).toEqual({ bar: true }) + expect(imp.tryTheseOr('bar')).toEqual({ bar: true }) // ensure cache has been used expect(patch1).toHaveBeenCalledTimes(1) diff --git a/src/util/importer.ts b/src/util/importer.ts index 07717293af..e2f224ac20 100644 --- a/src/util/importer.ts +++ b/src/util/importer.ts @@ -1,4 +1,4 @@ -import { ModulePatcher, TBabelCore, TBabelJest, TTypeScript, TsJestImporter } from '../types' +import { ModulePatcher, TBabelCore, TBabelJest, TTypeScript } from '../types' import * as hacks from './hacks' import { rootLogger } from './logger' @@ -26,7 +26,7 @@ const passThru = (action: () => void) => (input: any) => { /** * @internal */ -export class Importer implements TsJestImporter { +export class Importer { @Memoize() static get instance() { logger.debug('creating Importer singleton') @@ -70,22 +70,51 @@ export class Importer implements TsJestImporter { } @Memoize((...args: string[]) => args.join(':')) - tryThese(moduleName: string, ...fallbacks: string[]): any { + tryThese(moduleName: string, ...fallbacks: string[]) { let name: string - let loaded: any + let loaded: RequireResult | undefined const tries = [moduleName, ...fallbacks] // tslint:disable-next-line:no-conditional-assignment while ((name = tries.shift() as string) !== undefined) { - try { - loaded = requireModule(name) - logger.debug('loaded module', name) + const req = requireWrapper(name) + + // remove exports from what we're going to log + const contextReq: any = { ...req } + delete contextReq.exports + + if (req.exists) { + // module exists + loaded = req as RequireResult + if (loaded.error) { + // require-ing it failed + logger.error({ requireResult: contextReq }, `failed loading module '${name}'`, loaded.error.message) + } else { + // it has been loaded, let's patch it + logger.debug({ requireResult: contextReq }, 'loaded module', name) + loaded.exports = this._patch(name, loaded.exports) + } break - } catch (err) { - logger.debug('fail loading module', name) + } else { + // module does not exists in the path + logger.debug({ requireResult: contextReq }, `module '${name}' not found`) + continue } } - return loaded && this._patch(name, loaded) + // return the loaded one, could be one that has been loaded, or one which has failed during load + // but not one which does not exists + return loaded + } + + tryTheseOr(moduleNames: [string, ...string[]] | string, missingResult: T, allowLoadError?: boolean): T + tryTheseOr(moduleNames: [string, ...string[]] | string, missingResult?: T, allowLoadError?: boolean): T | undefined + tryTheseOr(moduleNames: [string, ...string[]] | string, missingResult?: T, allowLoadError = false): T | undefined { + const args: [string, ...string[]] = Array.isArray(moduleNames) ? moduleNames : [moduleNames] + const result = this.tryThese(...args) + if (!result) return missingResult + if (!result.error) return result.exports as T + if (allowLoadError) return missingResult + throw result.error } @Memoize(name => name) @@ -105,8 +134,10 @@ export class Importer implements TsJestImporter { // try to load any of the alternative after trying main one const res = this.tryThese(moduleName, ...alternatives) // if we could load one, return it - if (res) { - return res + if (res && res.exists) { + if (!res.error) return res.exports + // it could not load because of a failure while importing, but it exists + throw new Error(interpolate(Errors.LoadingModuleFailed, { module: res.given, error: res.error.message })) } // if it couldn't load, build a nice error message so the user can fix it by himself @@ -136,11 +167,43 @@ export class Importer implements TsJestImporter { */ export const importer = Importer.instance +/** + * @internal + */ +export interface RequireResult { + exists: E + given: string + path?: string + exports?: any + error?: Error +} + +function requireWrapper(moduleName: string): RequireResult { + let path: string + let exists = false + try { + path = resolveModule(moduleName) + exists = true + } catch (error) { + return { error, exists, given: moduleName } + } + const result: RequireResult = { exists, path, given: moduleName } + try { + result.exports = requireModule(moduleName) + } catch (error) { + result.error = error + } + return result +} + let requireModule = (mod: string) => require(mod) +let resolveModule = (mod: string) => require.resolve(mod) + /** * @internal */ // so that we can test easier -export function __requireModule(localRequire: typeof requireModule) { +export function __requireModule(localRequire: typeof requireModule, localResolve: typeof resolveModule) { requireModule = localRequire + resolveModule = localResolve } diff --git a/src/util/messages.ts b/src/util/messages.ts index 8c5345275a..b46d448abe 100644 --- a/src/util/messages.ts +++ b/src/util/messages.ts @@ -4,6 +4,7 @@ * @internal */ export enum Errors { + LoadingModuleFailed = 'Loading module {{module}} failed with error: {{error}}', UnableToLoadOneModule = 'Unable to load the module {{module}}. {{reason}} To fix it:\n{{fix}}', UnableToLoadAnyModule = 'Unable to load any of these modules: {{module}}. {{reason}}. To fix it:\n{{fix}}', TypesUnavailableWithoutTypeCheck = 'Type information is unavailable with "isolatedModules"',