From 7b44ca0ab0270fe453a3c84cb1799fcac20993e9 Mon Sep 17 00:00:00 2001 From: cpojer Date: Mon, 22 Feb 2016 21:11:08 -0800 Subject: [PATCH] Deep-unmock transitive dependencies when using `dontMock` only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary:…also fixes a bunch of other transitive unmocking things + simplify `_shouldMock` + cache metadata more thoroughly. This should now work properly. We are deep-unmocking node_modules now which seems like the more sensible solution and works similar to how we deal with npm2. Given that npm2 also dedupes and install order matters (or well, npm is non-deterministic), this should now be the much saner choice for both projects. Closes https://github.com/facebook/jest/pull/733 Reviewed By: kentaromiura Differential Revision: D2959955 fb-gh-sync-id: 7689afe345d71817b47cda01bbaba282df7abc86 shipit-source-id: 7689afe345d71817b47cda01bbaba282df7abc86 --- src/HasteModuleLoader/HasteModuleLoader.js | 172 ++++++++---------- .../HasteModuleLoader-requireModule-test.js | 28 +-- ...teModuleLoader-requireModuleOrMock-test.js | 66 +++++-- .../node_modules/npm3-main-dep/index.js | 3 +- .../node_modules/npm3-transitive-dep/index.js | 5 + .../npm3-transitive-dep/internal-code.js | 11 ++ 6 files changed, 153 insertions(+), 132 deletions(-) create mode 100644 src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-transitive-dep/internal-code.js diff --git a/src/HasteModuleLoader/HasteModuleLoader.js b/src/HasteModuleLoader/HasteModuleLoader.js index d6552101f476..868efa9dedc7 100644 --- a/src/HasteModuleLoader/HasteModuleLoader.js +++ b/src/HasteModuleLoader/HasteModuleLoader.js @@ -27,8 +27,10 @@ const mockParentModule = { const normalizedIDCache = Object.create(null); const moduleNameCache = Object.create(null); +const mockMetaDataCache = Object.create(null); const shouldMockModuleCache = Object.create(null); - +const transitiveShouldMock = Object.create(null); +const shouldUnmockTransitiveDependenciesCache = Object.create(null); const unmockRegExpCache = new WeakMap(); class Loader { @@ -40,7 +42,7 @@ class Loader { this._explicitShouldMock = Object.create(null); this._explicitlySetMocks = Object.create(null); this._isCurrentlyExecutingManualMock = null; - this._mockMetaDataCache = Object.create(null); + this._shouldAutoMock = true; this._extensions = config.moduleFileExtensions.map(ext => '.' + ext); @@ -51,11 +53,11 @@ class Loader { this._CoverageCollector = require(config.coverageCollector); } - this._unmockedList = unmockRegExpCache.get(config); - if (!this._unmockedList && config.unmockedModulePathPatterns) { - this._unmockedList = + this._unmockList = unmockRegExpCache.get(config); + if (!this._unmockList && config.unmockedModulePathPatterns) { + this._unmockList = new RegExp(config.unmockedModulePathPatterns.join('|')); - unmockRegExpCache.set(config, this._unmockedList); + unmockRegExpCache.set(config, this._unmockList); } // Workers communicate the config as JSON so we have to create a regex @@ -305,10 +307,10 @@ class Loader { _generateMock(currPath, moduleName) { const modulePath = this._resolveModuleName(currPath, moduleName); - if (!(modulePath in this._mockMetaDataCache)) { + if (!(modulePath in mockMetaDataCache)) { // This allows us to handle circular dependencies while generating an // automock - this._mockMetaDataCache[modulePath] = moduleMocker.getMetadata({}); + mockMetaDataCache[modulePath] = moduleMocker.getMetadata({}); // In order to avoid it being possible for automocking to potentially // cause side-effects within the module environment, we need to execute @@ -333,12 +335,9 @@ class Loader { if (mockMetadata === null) { throw new Error('Failed to get mock metadata: ' + modulePath); } - this._mockMetaDataCache[modulePath] = mockMetadata; + mockMetaDataCache[modulePath] = mockMetadata; } - - return moduleMocker.generateFromMetadata( - this._mockMetaDataCache[modulePath] - ); + return moduleMocker.generateFromMetadata(mockMetaDataCache[modulePath]); } _resolveModuleName(currPath, moduleName) { @@ -348,7 +347,7 @@ class Loader { } // Otherwise it is likely a node_module. - const key = currPath + ' : ' + moduleName; + const key = currPath + path.delimiter + moduleName; if (moduleNameCache[key]) { return moduleNameCache[key]; } @@ -357,6 +356,9 @@ class Loader { } _resolveNodeModule(currPath, moduleName) { + if (!moduleName) { + return currPath; + } const basedir = path.dirname(currPath); try { return resolve.sync(moduleName, { @@ -401,127 +403,109 @@ class Loader { } _getNormalizedModuleID(currPath, moduleName) { - const key = currPath + ' : ' + moduleName; + const key = currPath + path.delimiter + moduleName; if (normalizedIDCache[key]) { return normalizedIDCache[key]; } let moduleType; - let mockAbsPath = null; - let realAbsPath = null; + let mockPath = null; + let absolutePath = null; if (resolve.isCore(moduleName)) { moduleType = 'node'; - realAbsPath = moduleName; + absolutePath = moduleName; } else { moduleType = 'user'; if ( IS_PATH_BASED_MODULE_NAME.test(moduleName) || (!this._getModule(moduleName) && !this._getMockModule(moduleName)) ) { - realAbsPath = this._resolveModuleName(currPath, moduleName); - if (realAbsPath == null) { - throw new Error( - `Cannot find module '${moduleName}' from '${currPath || '.'}'` - ); - } - + absolutePath = this._resolveModuleName(currPath, moduleName); // Look up if this module has an associated manual mock. const mockModule = this._getMockModule(moduleName); if (mockModule) { - mockAbsPath = mockModule; + mockPath = mockModule; } } - if (realAbsPath === null) { + if (absolutePath === null) { const moduleResource = this._getModule(moduleName); if (moduleResource) { - realAbsPath = moduleResource; + absolutePath = moduleResource; } } - if (mockAbsPath === null) { + if (mockPath === null) { const mockResource = this._getMockModule(moduleName); if (mockResource) { - mockAbsPath = mockResource; + mockPath = mockResource; } } } const delimiter = path.delimiter; - const id = moduleType + delimiter + realAbsPath + delimiter + mockAbsPath; + const id = moduleType + delimiter + absolutePath + delimiter + mockPath; normalizedIDCache[key] = id; return id; } _shouldMock(currPath, moduleName) { + const explicitShouldMock = this._explicitShouldMock; const moduleID = this._getNormalizedModuleID(currPath, moduleName); - if (moduleID in this._explicitShouldMock) { - return this._explicitShouldMock[moduleID]; + const key = currPath + path.delimiter + moduleID; + + if (moduleID in explicitShouldMock) { + return explicitShouldMock[moduleID]; } - if (resolve.isCore(moduleName)) { + if ( + !this._shouldAutoMock || + resolve.isCore(moduleName) || + shouldUnmockTransitiveDependenciesCache[key] + ) { return false; } - if (this._shouldAutoMock) { - // See if the module is specified in the config as a module that should - // never be mocked - if (moduleName in shouldMockModuleCache) { - return shouldMockModuleCache[moduleName]; - } else if (this._unmockedList) { - const manualMockResource = this._getMockModule(moduleName); - let modulePath; - try { - modulePath = this._resolveModuleName(currPath, moduleName); - } catch (e) { - // If there isn't a real module, we don't have a path to match - // against the unmockList regexps. If there is also not a manual - // mock, then we throw because this module doesn't exist anywhere. - // - // However, it's possible that someone has a manual mock for a - // non-existent real module. In this case, we should mock the module - // (because we technically can). - // - // Ideally this should never happen, but we have some odd - // pre-existing edge-cases that rely on it so we need it for now. - // - // I'd like to eliminate this behavior in favor of requiring that - // all module environments are complete (meaning you can't just - // write a manual mock as a substitute for a real module). - if (manualMockResource) { - shouldMockModuleCache[moduleName] = true; - return true; - } - throw e; - } - - const realPath = fs.realpathSync(modulePath); - if (this._unmockedList.test(realPath)) { - shouldMockModuleCache[moduleName] = false; - } else if ( - currPath.includes(NODE_MODULES) && - realPath.includes(NODE_MODULES) && - ( - !shouldMockModuleCache[moduleName] || - this._unmockedList.test(currPath) - ) - ) { - // If the dependency should normally be mocked but the parent - // module is a node module that is not being mocked, the target module - // should be unmocked too. This transitive behavior is useful for flat - // package managers, like npm3. - return false; - } else { - shouldMockModuleCache[moduleName] = true; - } + if (moduleName in shouldMockModuleCache) { + return shouldMockModuleCache[moduleName]; + } - return shouldMockModuleCache[moduleName]; + const manualMockResource = this._getMockModule(moduleName); + let modulePath; + try { + modulePath = this._resolveModuleName(currPath, moduleName); + } catch (e) { + if (manualMockResource) { + shouldMockModuleCache[moduleName] = true; + return true; } - return true; - } else { + throw e; + } + + const realPath = fs.realpathSync(modulePath); + if (this._unmockList && this._unmockList.test(realPath)) { + shouldMockModuleCache[moduleName] = false; return false; } + + // transitive unmocking for package managers that store flat packages (npm3) + const currentModuleID = this._getNormalizedModuleID(currPath); + if ( + currPath.includes(NODE_MODULES) && + realPath.includes(NODE_MODULES) && + ( + (this._unmockList && this._unmockList.test(currPath)) || + explicitShouldMock[currentModuleID] === false || + transitiveShouldMock[currentModuleID] === false + ) + ) { + transitiveShouldMock[moduleID] = false; + shouldUnmockTransitiveDependenciesCache[key] = true; + return false; + } + + return shouldMockModuleCache[moduleName] = true; } _resolveStubModuleName(moduleName) { @@ -551,6 +535,11 @@ class Loader { } _createRuntimeFor(currPath) { + const unmock = moduleName => { + const moduleID = this._getNormalizedModuleID(currPath, moduleName); + this._explicitShouldMock[moduleID] = false; + return runtime; + }; const runtime = { addMatchers: matchers => { const jasmine = this._environment.global.jasmine; @@ -570,11 +559,8 @@ class Loader { clearAllTimers: () => this._environment.fakeTimers.clearAllTimers(), currentTestPath: () => this._environment.testFilePath, - dontMock: moduleName => { - const moduleID = this._getNormalizedModuleID(currPath, moduleName); - this._explicitShouldMock[moduleID] = false; - return runtime; - }, + dontMock: unmock, + unmock, getTestEnvData: () => { const frozenCopy = {}; diff --git a/src/HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js b/src/HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js index dee84b4014cd..fcb7071456f5 100644 --- a/src/HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js +++ b/src/HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js @@ -154,27 +154,13 @@ describe('HasteModuleLoader', function() { }); }); - pit( - 'doesnt override real modules with manual mocks when explicitly ' + - 'marked with .dontMock()', - function() { - return buildLoader().then(function(loader) { - const root = loader.requireModule(rootPath, './root.js'); - root.jest.resetModuleRegistry(); - root.jest.dontMock('ManuallyMocked'); - const exports = loader.requireModule(rootPath, 'ManuallyMocked'); - expect(exports.isManualMockModule).toBe(false); - }); - } - ); - - pit('unmocks transitive dependencies in node_modules by default', () => { - return buildLoader().then(loader => { - const nodeModule = loader.requireModule(rootPath, 'npm3-main-dep'); - expect(nodeModule()).toEqual({ - isMocked: false, - transitiveNPM3Dep: 'npm3-transitive-dep', - }); + pit(`doesn't override real modules with manual mocks when explicitly marked with .unmock()`, () => { + return buildLoader().then(function(loader) { + const root = loader.requireModule(rootPath, './root.js'); + root.jest.resetModuleRegistry(); + root.jest.unmock('ManuallyMocked'); + const exports = loader.requireModule(rootPath, 'ManuallyMocked'); + expect(exports.isManualMockModule).toBe(false); }); }); }); diff --git a/src/HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js b/src/HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js index 358b33a91593..05bcf704e403 100644 --- a/src/HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js +++ b/src/HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js @@ -22,7 +22,7 @@ describe('HasteModuleLoader', function() { const rootDir = path.join(__dirname, 'test_root'); const rootPath = path.join(rootDir, 'root.js'); - const config = utils.normalizeConfig({ + const baseConfig = utils.normalizeConfig({ cacheDirectory: global.CACHE_DIRECTORY, name: 'HasteModuleLoader-requireModuleOrMock-tests', rootDir, @@ -32,7 +32,8 @@ describe('HasteModuleLoader', function() { }, }); - function buildLoader() { + function buildLoader(config) { + config = Object.assign({}, baseConfig, config); const environment = new JSDOMEnvironment(config); const resolver = new HasteResolver(config, {resetCache: false}); return resolver.getHasteMap().then( @@ -56,29 +57,25 @@ describe('HasteModuleLoader', function() { }); }); - pit('doesnt mock modules when explicitly dontMock()ed', function() { + pit(`doesn't mock modules when explicitly unmocked`, function() { return buildLoader().then(function(loader) { const root = loader.requireModule(rootDir, rootPath); - root.jest.dontMock('RegularModule'); + root.jest.unmock('RegularModule'); const exports = loader.requireModuleOrMock(rootPath, 'RegularModule'); expect(exports.isRealModule).toBe(true); }); }); - pit( - 'doesnt mock modules when explicitly dontMock()ed via a different ' + - 'denormalized module name', - function() { - return buildLoader().then(function(loader) { - const root = loader.requireModule(rootDir, rootPath); - root.jest.dontMock('./RegularModule'); - const exports = loader.requireModuleOrMock(rootPath, 'RegularModule'); - expect(exports.isRealModule).toBe(true); - }); - } - ); + pit(`doesn't mock modules when explicitly unmocked via a different denormalized module name`, () => { + return buildLoader().then(function(loader) { + const root = loader.requireModule(rootDir, rootPath); + root.jest.unmock('./RegularModule'); + const exports = loader.requireModuleOrMock(rootPath, 'RegularModule'); + expect(exports.isRealModule).toBe(true); + }); + }); - pit('doesnt mock modules when autoMockOff() has been called', function() { + pit(`doesn't mock modules when autoMockOff() has been called`, function() { return buildLoader().then(function(loader) { const root = loader.requireModule(rootDir, rootPath); root.jest.autoMockOff(); @@ -123,5 +120,40 @@ describe('HasteModuleLoader', function() { expect(exports.isRelativeImageStub).toBe(true); }); }); + + describe('transitive dependencies', () => { + const expectUnmocked = nodeModule => { + const moduleData = nodeModule(); + expect(moduleData.isUnmocked()).toBe(true); + expect(moduleData.transitiveNPM3Dep).toEqual('npm3-transitive-dep'); + expect(moduleData.internalImplementation()) + .toEqual('internal-module-code'); + }; + + pit('unmocks transitive dependencies in node_modules by default', () => { + return buildLoader({ + unmockedModulePathPatterns: ['npm3-main-dep'], + }).then(loader => { + const root = loader.requireModule(rootPath, './root.js'); + expectUnmocked(loader.requireModuleOrMock(rootPath, 'npm3-main-dep')); + + // Test twice to make sure HasteModuleLoader caching works properly + root.jest.resetModuleRegistry(); + expectUnmocked(loader.requireModuleOrMock(rootPath, 'npm3-main-dep')); + }); + }); + + pit('unmocks transitive dependencies in node_modules when using unmock', () => { + return buildLoader().then(loader => { + const root = loader.requireModule(rootPath, './root.js'); + root.jest.unmock('npm3-main-dep'); + expectUnmocked(loader.requireModuleOrMock(rootPath, 'npm3-main-dep')); + + // Test twice to make sure HasteModuleLoader caching works properly + root.jest.resetModuleRegistry(); + expectUnmocked(loader.requireModuleOrMock(rootPath, 'npm3-main-dep')); + }); + }); + }); }); }); diff --git a/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-main-dep/index.js b/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-main-dep/index.js index b45a858b1428..6bac7c991fdb 100644 --- a/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-main-dep/index.js +++ b/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-main-dep/index.js @@ -11,6 +11,7 @@ const transitiveNPM3Dep = require('npm3-transitive-dep'); module.exports = () => ({ - isMocked: false, + isUnmocked: () => true, transitiveNPM3Dep: transitiveNPM3Dep(), + internalImplementation: transitiveNPM3Dep.internalImplementation, }); diff --git a/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-transitive-dep/index.js b/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-transitive-dep/index.js index 53a1f5adb7af..2ba516f0715b 100644 --- a/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-transitive-dep/index.js +++ b/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-transitive-dep/index.js @@ -6,4 +6,9 @@ * of patent rights can be found in the PATENTS file in the same directory. */ +'use strict'; + +const internalImplementation = require('./internal-code'); + module.exports = () => 'npm3-transitive-dep'; +module.exports.internalImplementation = internalImplementation; diff --git a/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-transitive-dep/internal-code.js b/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-transitive-dep/internal-code.js new file mode 100644 index 000000000000..d5fc1bbe8dd0 --- /dev/null +++ b/src/HasteModuleLoader/__tests__/test_root/node_modules/npm3-transitive-dep/internal-code.js @@ -0,0 +1,11 @@ +/** + * Copyright (c) 2014, Facebook, Inc. All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +'use strict'; + +module.exports = () => 'internal-module-code';