jest icon indicating copy to clipboard operation
jest copied to clipboard

[bug] duplicate manual mock found in separate directories

Open paularmstrong opened this issue 8 years ago • 75 comments

Do you want to request a feature or report a bug? Bug

What is the current behavior?

Given a file tree:

src/app/modules
├── module1
│   ├── index.js
│   ├── __tests__/
├── module2
│   ├── index.js
│   ├── __tests__/

I use the modules outside of the modules directory by importing them by directory name:

import Module1 from '../modules/module1';
import Module2 from '../modules/module2';

I'd like to be able to mock module1 and module2. However, if I create src/app/modules/module1/__mocks__/index.js and src/app/modules/module2/__mocks__/index.js, I'm given the duplicate manual mock found error from jest-haste-map.

If, however, I try to create src/app/modules/__mocks__/{module1.js,module2.js}, the mocked files are not used.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal repository on GitHub that we can npm install and npm test.

See above behavior.

What is the expected behavior?

I would expect either approach to work, given that the first case uses different paths and the second case uses the pathname of the module.

Run Jest again with --debug and provide the full configuration it prints. Please mention your node and npm version and operating system.

node v6.2.0 npm v3.8.9 OS X 10.11.6

> NODE_ENV=test jest --env jsdom "--debug" "src/app/redux/modules/devices"

jest version = 17.0.0
test framework = jasmine2
config = {
  "moduleFileExtensions": [
    "js",
    "json"
  ],
  "moduleDirectories": [
    "node_modules"
  ],
  "moduleNameMapper": [
    [
      "^.+\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$",
      "/Users/paul/dev/tools/jest/mock-assets.js"
    ],
    [
      "^.+\\.css$",
      "identity-obj-proxy"
    ]
  ],
  "name": "dev",
  "setupTestFrameworkScriptFile": "/Users/paul/dev/tools/jest/setup-framework.js",
  "testPathDirs": [
    "/Users/paul/dev/src"
  ],
  "testRegex": "/__tests__/.*\\.test\\.js$",
  "timers": "fake",
  "rootDir": "/Users/paul/dev",
  "setupFiles": [],
  "testRunner": "/Users/paul/dev/node_modules/jest-jasmine2/build/index.js",
  "testEnvironment": "/Users/paul/dev/node_modules/jest-environment-jsdom/build/index.js",
  "transform": [
    [
      "^.+\\.jsx?$",
      "/Users/paul/dev/node_modules/babel-jest/build/index.js"
    ]
  ],
  "usesBabelJest": true,
  "automock": false,
  "bail": false,
  "browser": false,
  "cacheDirectory": "/var/folders/dm/vt920lmd6tzdq_709zkykwx40000gn/T/jest",
  "coveragePathIgnorePatterns": [
    "/node_modules/"
  ],
  "coverageReporters": [
    "json",
    "text",
    "lcov",
    "clover"
  ],
  "expand": false,
  "globals": {},
  "haste": {
    "providesModuleNodeModules": []
  },
  "mocksPattern": "__mocks__",
  "modulePathIgnorePatterns": [],
  "noStackTrace": false,
  "notify": false,
  "preset": null,
  "resetMocks": false,
  "resetModules": false,
  "snapshotSerializers": [],
  "testPathIgnorePatterns": [
    "/node_modules/"
  ],
  "testURL": "about:blank",
  "transformIgnorePatterns": [
    "/node_modules/"
  ],
  "useStderr": false,
  "verbose": null,
  "watch": false,
  "cache": true,
  "watchman": true,
  "testcheckOptions": {
    "times": 100,
    "maxSize": 200
  }
}
jest-haste-map: duplicate manual mock found:
  Module name: index
  Duplicate Mock path: /Users/paul/dev/src/app/modules/push-notification-manager/__mocks__/index.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in:
/Users/paul/dev/src/app/modules/push-notification-manager/__mocks__/index.js
 Please delete one of the following two files:
 /Users/paul/dev/src/app/modules/image-file/__mocks__/index.js
/Users/paul/dev/src/app/modules/push-notification-manager/__mocks__/index.js


No tests found
  1 file checked.
  testPathDirs: /Users/paul/dev/src - 1 match
  testRegex: /__tests__/.*\.test\.js$ - 0 matches
  testPathIgnorePatterns: /node_modules/ - 1 match

paularmstrong avatar Nov 09 '16 18:11 paularmstrong

+1

yongdamsh avatar Nov 15 '16 07:11 yongdamsh

In my case, after clear the cacheDirectory/var/folders/dm/vt920lmd6tzdq_709zkykwx40000gn/T/jest and re-install npm dependencies, these messages have been disappeared.

yongdamsh avatar Nov 15 '16 08:11 yongdamsh

Here's the offending code:

https://github.com/facebook/jest/blob/cd8976ec50dbed79cfe07f275052cdd80d466e73/packages/jest-haste-map/src/index.js#L98

But it looks like the behavior might be explicitly wanted as there is a test confirming it:

https://github.com/facebook/jest/blob/8de90b320c87a0a36d68f6bd8177620a985df269/packages/jest-haste-map/src/tests/snapshots/index-test.js.snap#L15

Which was added in:

https://github.com/facebook/jest/commit/cfade282fbbe2737b6dd2cee1cf3da3ee8624512

I wonder why we are using basename rather than the whole path as the key?

/cc @flarnie

LegNeato avatar Nov 17 '16 01:11 LegNeato

This means that basenames for modules need to be globally unique in a project when using manual mocks. For my use case, it means I can't do something like:

import { MyWhatever } from 'models/MyWhatever/schema';
import { MyOtherWhatever } from 'models/MyOtherWhatever/schema';

and use manual mocks at the same time. Jest will currently see them both as mocking schema and complain.

Though the workaround is trivial (s/schema/MyWhateverSchema/), it feels like a bug to rename and restructure non-test code to make jest happy 🐞 .

LegNeato avatar Nov 17 '16 01:11 LegNeato

Yes this does suck indeed. The manual mocking system is really not good and I'm happy to accept PRs that will improve the situation, assuming we can make sure we don't break all of FB (but I can take care of that :) )

cpojer avatar Nov 17 '16 01:11 cpojer

Cool. I might find some time tomorrow to cook up a patch, but no promises though 😅

LegNeato avatar Nov 17 '16 01:11 LegNeato

@cpojer is there a particular reason for this behaviour?

Could it have anything to do with the fact that fb uses unique filenames for modules? I don't see otherwise how not allowing two mocks with the same name makes any sense...

juliankrispel avatar Nov 17 '16 14:11 juliankrispel

Yes, mocks are "global" as well. This is a terrible design that we have to live with, unfortunately. At FB we have 4000+ mock files in the wrong location (and often there isn't even a proper location). It is likely we will fix this early next half, so this should improve in Jest. I'm happy to support PRs that improve the behavior in Jest for open source – if we can retain the old behavior for Jest at FB for now.

cpojer avatar Nov 18 '16 03:11 cpojer

@cpojer how about a flag? Would you accept a pr that enables/disables this with a flag?

juliankrispel avatar Nov 18 '16 08:11 juliankrispel

Yeah, it should be a config option. But I'm not just talking about the warning, I'm also talking about the feature in general.

cpojer avatar Nov 18 '16 08:11 cpojer

@cpojer right - which parts of jest does this touch?

juliankrispel avatar Nov 18 '16 08:11 juliankrispel

The resolution code is called from jest-runtime: https://github.com/facebook/jest/blob/master/packages/jest-runtime/src/index.js and is somewhere in jest-resolve and jest-resolve-dependencies.

cpojer avatar Nov 18 '16 08:11 cpojer

@cpojer thanks for the pointers :+1:

juliankrispel avatar Nov 18 '16 08:11 juliankrispel

@cpojer what about some global override, like JEST_USE_BASENAME_FOR_CACHING, to switch this behaviour?

At least, we can enjoy non-unique filenames, and it will not break anything in FB.

Of course, it's a temporary solution.

I mean, this is in some /etc/profile or ~/.bashrc

export JEST_USE_BASENAME_FOR_CACHING="true"

(or some file with env) and then

$ jest

or this one, without env file modifications:

$ JEST_USE_BASENAME_FOR_CACHING="true" jest

What you think? It's a sort of hack or it's ok? :wink:

ColCh avatar Dec 04 '16 17:12 ColCh

I just tried with a new repo, using two versions of jest (^15.0.0 and ^17.0.0) and, although the latter gives the warning, the test behaves as expected.

@ColCh I don't think the issue here is with the cache, probably a more suitable name could be JEST_USE_BASENAME_FOR_MOCKING.

EnoahNetzach avatar Dec 05 '16 13:12 EnoahNetzach

@cpojer if FB code has the restriction on the uniqueness of the names, using the full path as a key for the map of mocks shouldn't bring problems.

Am I right or there is something I'm not seeing?

EnoahNetzach avatar Dec 05 '16 14:12 EnoahNetzach

The two solutions I see are:

  • modify getMockName to accomodate the option to use the basename or the full path
  • remove that function altogether

EnoahNetzach avatar Dec 05 '16 14:12 EnoahNetzach

it would be nice to see @cpojer answer on it

ColCh avatar Dec 05 '16 14:12 ColCh

Hey everyone, sorry for the delay, I'm pretty backed up with a ton of stuff right now.

I think I'm fine if you guys decide to do whatever breaking change in Jest that is necessary to improve this system. Manual mocking is really messed up and doesn't work well. One thing we kind of want to do is limit the scope of "haste modules" (internal FB module system) using a config option, like "haste_modules": ['path/a', 'path/b']" and then only look at haste modules in those folders, including this weird mocking behavior. If anybody wants to make this change, that would be amazing.

One thing to be figured out then, is this: If all manual mocks are local, like __mocks__/a.js maps to a.js, what do we do with node_module mocks? For this there are a couple of ways:

  • Introduce a new __node_modules_mocks__ folder but that is ugly.
  • The top level __mocks__ folder as seen from rootDir (project root) could act as a global folder.

So to summarize:

  • Let's fix manual mocks in Jest!
  • Let's namespace haste modules and limit them to certain folders/regex whatever.
  • Make sure that the current mocking behavior still works for FB, even if that means we have to whitelist certain folders to make haste work (it would just look like ["<rootDir>"] for us for now, I guess)
  • Figure out how to still be able to mock node modules.

What do you think?

cpojer avatar Dec 06 '16 00:12 cpojer

In order:

  • Let's fix manual mocks in Jest!

HURRAY :smile: :tada:

  • Let's namespace haste modules and limit them to certain folders/regex whatever.
  • Make sure that the current mocking behavior still works for FB, even if that means we have to whitelist certain folders to make haste work (it would just look like ["<rootDir>"] for us for now, I guess)

I'm not sure I understand the needs with haste. What you mean is to give the possibility to say "those modules are haste modules"? If we have four modules: /path_1/a, /path_1/b, /path_2/a, /path_2/c, and the setting is

"haste_modules:" ["/path_1/a", "/path_2/c"]

only /path_1/a and /path_1/b are restricted to exist only in /path_1, so /path_2/c is valid, and /path_2/a raises an error/warning.

I'd say, targets could easily be specific files and entire directories, even with single/double *.

  • Figure out how to still be able to mock node modules.

I'd maintain the current behavior:

If the module you are mocking is a node module (eg: fs), the mock should be placed in the same parent directory as the node_modules folder.

EnoahNetzach avatar Dec 06 '16 09:12 EnoahNetzach

My thoughts:

haste modules:

I think , haste_modules is nice, it fits just like collectCoverageFrom and other options: array of globs In case, if you have all src as haste modules, and just one directory is non-haste:

haste_modules: [
  "src",
  "!src/foo"
]

node_modules

@EnoahNetzach what if someone have same names for app module and module from node_modules?


To make it work without haste... hm, I think it can be described like:

given node module project/node_modules/react, mock will be inside of project/__node_modules_mocks__/react.js if you have a file project/react.js, then use project/__mocks__/react.js

(of course, react.js is an example. here can be any filename among of all modules, which can be installed from npm)

Really, mocking node_modules module is a rare case by my experience, so... may be rareness may compensate ugliness in particular case of mocking node_modules ?

Anyone mocking modules inside of node_modules often?

what about to think further

as I noticed, for react-native projects, we often have to mock application modules and leave modules from node_modules unmocked (e.g. lodash)

this means, we have:

  • manual created mock component for each dumb component (dumb component are layout components)
  • a long list of jest.mock call in every test file

what I want to say: it would be very nice to have ability to auto mock modules on some paths.

It can be implemented around well-known data structure array-of-jest-globs in config, and filtering modules upon it.

I will describe that step by step

Given this config entry

"autoMockingPaths": [
  "src/components/dumb/**/*.js",
]

and this code at src/screens/app.js:

import _ from 'lodash';
import Button from '../../components/dumb/button.js';

// blah blah AppScreen implementation skipped

and this test code for screen at src/screens/__tests__/app-test.js:

import AppScreen from '../app.js';

describe('AppScreen', () => /* testing app screen */);

We come to this situation, in context of app-test.js:

  • AppScreen is not mocked
  • lodash is not mocked
  • Button, which is required by AppScreen, is mocked

... You can answer, how it will play with automock config entry?

simply saying, automock: true is equivalent to:

"autoMockingPaths": [
  "<rootDir>"
]

auto mocking... wait!

may be just introduce special value for automock? at least it will not break people's configs

for example, with this config entry:

automock: "app"

jest will auto mock all application modules, and leave actual versions for modules from node_modules

what do you think about app level modules automocking, @cpojer ? I find it very efficient for my particular case.

ColCh avatar Dec 06 '16 10:12 ColCh

I fully agree with "haste_modules".

We personally don't use automocking that much, so I can't say what's better, my wild guess is that the "autoMockingPaths" var could be useful and elastic enough. On the contrary I find "automock": "app" too stiff (jest already disabled automocking by default).

The __node_modules_mocks__ could be an option, I agree that rareness compensate for ugliness (in my particular case, we rarely mock node_modules, and when we have to do it, we use jest.mock(...)). The only caveat is: what happens when you have a nested node_modules folder (e.g. src/node_modules), do you have to mock its modules from the global __node_modules_mocks__, a nested version of it, or normally with __mocks__ co-located?

EnoahNetzach avatar Dec 06 '16 11:12 EnoahNetzach

may be just throw, if someone has same module name in node_modules and app modules

e.g.

app/express.js as app module (may be I'm doing train game) and app/node_modules/express as web server from npm throw new Error("can't mock express.js file - it duplicates one from node_modules")

in this case, __mocks__ can be used for node_modules, but dev have to rename own modules on such collisions

nah... this is more ugly than __node_modules_mocks__, isn't it?

ColCh avatar Dec 06 '16 11:12 ColCh

What I meant is: what if you have an npm installed module x and later, deeper in your codebase define a module x in a nested node_modules folder?

The naming collision is usually handled in node by preferring the nearest, but I don't know how this works in haste.

I'm bringing this up because projects like Create React App are using it, or will do in the near future.

On a side note, @cpojer is this issue somehow related?

EnoahNetzach avatar Dec 07 '16 09:12 EnoahNetzach

Let's keep this focused on changing how haste works (whitelist/blacklist rather than on by default). I do think I'd prefer to maintain that <rootDir>/__mocks__ should be the default for node module mocks. We could make this a configuration option as well: "globalMocks" that defaults to <rootDir>/__mocks__. Is anybody willing to work on this?

cpojer avatar Dec 08 '16 01:12 cpojer

I should be able to work on this no sooner than the next weekend.

EnoahNetzach avatar Dec 09 '16 08:12 EnoahNetzach

I can work on PR this sunday, I'm kinda free

@cpojer just to recap - create globalMocks config entry with default value of <rootDir>/__mocks__. This option regulates use of node-haste within jest by specifying path? Or it will be array of paths?

ColCh avatar Dec 09 '16 08:12 ColCh

I think these are some larger changes on the way to get this resolved but I think we need both the singular globalMocks option (could be a string or array of strings) and the hasteModules option which would be an array of paths of haste modules. Most of this code lives in jest-haste-map and jest-resolve. I'm not 100% sure what the right solution will look like yet.


From: Max Sysoev [email protected] Sent: Friday, December 9, 2016 8:18:44 AM To: facebook/jest Cc: Christoph Pojer; Mention Subject: Re: [facebook/jest] [bug] duplicate manual mock found in separate directories (#2070)

I can work on PR this sunday, I'm kinda free

@cpojerhttps://github.com/cpojer just to recap - create globalMocks config entry with default value of <rootDir>/mocks. This option regulates use of node-haste within jest by specifying path? Or it will be array of paths?

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/facebook/jest/issues/2070#issuecomment-265958606, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAA0KAMFc34iKqBDLHZzgaGHqyc3WkAzks5rGQ7kgaJpZM4Kt2DW.

cpojer avatar Dec 09 '16 08:12 cpojer

Sorry, my workstation became broken, and apparently no way to get it working in couple of months (1-2 months). I even lost my project on this PR :( So, sorry for taking responsibilities.

ColCh avatar Dec 22 '16 17:12 ColCh

~~What about just a config option to change the behavior of getMockName?~~

~~Not too familiar with the internals of jest, but it looks like that is the simplest solution to fix the issue without breaking jest for FB.~~

This is going to be more complicated than I originally thought. To me, manual mocks should replace the file they're closest too, ie. something like this:

{ 'aws-sdk': '/Users/project/__mocks__/aws-sdk.js',
  'slack': '/Users/project/__mocks__/slack.js',
  '/Users/project/db/index': '/Users/project/db/__mocks__/index.js',
  '/Users/project/slack/index': '/Users/projects/slack/__mocks__/index.js' }

require('aws-sdk') should resolve to the /Users/project/__mocks__/aws-sdk.js this is a mock for a node_module.

require('./db') (or any path to db) should resolve to: /Users/project/db/__mocks__/index.js.

My understanding of the way to setup jest manual mocks (and there should probably be more documentation if something like the above is used) was that they should be as close to the file being mocked as possible within a __mocks__ directory.

Manual mocks are defined by writing a module in a mocks/ subdirectory immediately adjacent to the module. (https://facebook.github.io/jest/docs/manual-mocks.html)

Given that, the above behavior makes the most sense to me.

Thoughts?

mwildehahn avatar Jan 15 '17 21:01 mwildehahn